Don't immediatly see the point behind this(php, school)

Question

I'm a 2nd year ICT student. I never did PHP before this year and our instructor gave us the basics and at the end of the semester, gave us a project that would combine what we learned in his course and the databases course. We were to use the classic AMP setup on windows.

Now, our instructor told us to make a profile-site, based on how we made smaller ones before in class.

I don't see the point behind the somewhat weird method of entering the user into the database.

First, we do some PHP formchecking to make sure the entered data is safe and somewhat realistic(for instance, zip-codes over here are 4 numbers, never more and no letters or other symbols).

When everything checks out fine, we do the following:

$sql = new SqlObject(); //from SqlObject.class.php
$newUser = new User(login,passw,mail,...,...,...); //from User.class.php
$sql->addUser($newUser);

The SqlObject class is a class that contains all the SQL commands we need update, insert and generally alter data in the database. We never write SQL in our normal pages. But that's not what I'm confused about. It's the User.class.php file.

This file contains only a constructor and exactly the same amount of fields as needs to be entered into the database. For instance:

<?php

  class User {
    // members
    var $id;
    var $name;
    var $password;

    // constructor
    function User($id=-1,$name='',$password='') {
        $this->id = $id;
        $this->name = $name;
        $this->password = $password;
    }
}

?>

That's it. The SqlObject.class.php file requires the User.class.php file on the first line.

The function addUser($user) in the SqlObject.class.php file looks like this:

function addUser($user) {
  $insQuery = 'INSERT INTO users(name,password)';
  $insQuery.= " VALUES ('".$user->name."', '".$user->password."')";
  @mysql_query($insQuery) or showError("user insert failed");
}

Why make such a detour via the User.class.php file? Security reason of some kind?

I'll repeat: It's my first year using PHP and I'm still a student.

EDIT: People are complaining that there is no checks on SQL injection before inserting the data.

At the beginning of this post, I mentioned "formchecking". The register.php file does all the escaping and checking of input. This includes several Regex tests, mysql_real_escape_string() and some simpler tests. Once all tests are passed and all input is escaped, only then will this happen:

$sql = new SqlObject(); //from SqlObject.class.php
$newUser = new User(login,passw,mail,...,...,...); //from User.class.php
$sql->addUser($newUser);

That code is never executed if the input doesn't receive the treatment that I see some people wanting to give it inside the SqlObject.class.php file.

EDIT2: as promised, blog posted: http://webdevhobo.blogspot.com/2009/08/prepared-statements-in-php.html

Answer 1

So, you're asking why not just pass the user and password in directly

 function addUser($name='',$password='') { etc

My guess is that the code you show is an example of future-proofing, the idea being that the User class might do something a lot more detailed in the future, or get the credentials from some other source, rather than having name and password passed to its constructor.

It's an idea of separation of concerns, some bit if code is responsible for assembling the suer data, the addUer function merely uses the stuff it needs. In large programs it really helps to have these kinds of organisation - on the surface it might appear to be adding complexity but when you start to think like this it enables you limit the number of pieces you need to keep in your head. Also you might imagine breaking up the overall programming task so one person looks at the User class, another does the addUser. They can work independently. Silly in this tiny case, but in the real world very beneficial.

Answer 2

First of all, the addUser() method is awful, allows sql injections because it doesn't use prepared statements nor escapes the contents. It should read something like

function addUser($user) {
  $insQuery = 'INSERT INTO users(name,password)';
  $insQuery.= " VALUES ('".mysql_real_escape_string($user->name)."',";
  $insQuery.= " '".mysql_real_escape_string($user->password)."')";
  @mysql_query($insQuery) or showError("user insert failed");
}

This is awful because it teaches you bad practices, your teacher should teach you about SQL injections from day one, and not simplify them just for academic sake.

That aside, the idea is to future proof the code, using abstractions.

Abstractions allow you to think in a higher level. At this current level the User class might seem overkill, because it acts only as a storage facility, but it helps you think in term of domain instances instead of SQL sentences which then will help you make modifications shall such a need arise.

These abstractions also create a single place to complexify the code without having the changes spread all over, for example if you'd then need something else about a user you need only change the User class, for example by adding a, say, printUser() method or whatever.

function printUser() {
    return "Mr. ".$this->name;
}

But this being an easy and artificial academic example, it might not make much sense. You'd need a bigger scenario to really see the benefit of abstractions.

About your edits:

I'm glad that you are told about SQL injections, even if not in the right place :)

The proper place to do the escaping for SQL injection is not in a previous 'formchecking' step, but to do it when sending the data to the SQL server. That 'formchecking' is the proper place to validate as you say, length of zipcodes, or empty fields and so on, but not to escape the strings to be SQL safe.

A simple example should show why:

$name = formcheck("John O'Donnell"); //Where you do all your checking step,
                                     //including mysql_real_escape_string
$user = new User($name,"gandalf");

But,

mysql_real_escape_string("John O'Donnell") = "John O\'Donnell";

Now you use your User instance to display the name on the screen:

 echo "Welcome ".$user->printUser();

and you get:

Welcome Mr. John O\'Donell

Answer 3

At first, I thought it was to prevent SQL-Injections, but as there is no validation of the input, this isn't the case either. So only for future addititions to the functionality is a possibility.

Did you learn to use mysql_real_escape_string in the database class? Because this is really needed to protect your database from attacks!

Update to reflect OP's edit: I still dont feel comfortable about the sanatizing of the input. An UI or GUI is for one purpose only: Display data. The businesslayer (this would be your User class) cares about the sanatizing, before it sends the data to the datalayer (your sqlobject class). Please think about it and maybe talk to it with your instructors.

Answer 4

It may not make as much sense since the project is small. But you now have a user class in which you could add more functions to use everywhere else. Such as a function to check if a username exists, update, delete ect.

It also makes it so that you can reuse this code in the next project since its more robust. Say in the next project you need to write something bigger such as a full user management system. You would then realize how nice it is to split things up.

Answer 5

If this is a sample of OO in PHP, then it's a bad one. Why? Because the SqlObject contains a method AddUser(), which suggests the class is directly linked to the User class. You could derive an SqlUser class from SqlObject and use that class to add users instead. This new class would handle the communications with the database and would also be responsible to prevent any SQL injections. Even though you can escape any attempts of code injection in other locations, it should still be included in this class, just in case you forget to add a call somewhere else. (Don't think that won't happen, because it will!)

However, there is a reason for keeping things a bit simpler, even though it costs reliability in the final code. This is a school project and therefore an example of how to write code. It's reasonable robust for a simple project, even though there's a lot of room for improvements. Also, keep in mind that you're learning in a controlled environment. Once you start to work for a commercial company, things should look a lot better. (And unfortunately, at quite a few companies, things are just far worse.)

Answer 6

if the AddUser class is inside of the SqlClass that means that only the SqlClass can add a user. this would mean that the sql class must depend on the user class to know what a user is before it can add one. (you couldn't add apples and oranges, especially if you didnt know what an orange was)