Simple router for updating users

Question

I need to know if I am going in the right direction with my code so far. I am really trying hard to transition from procedural, page-based programming into OOP MVC. So far I have written a simple router, and MVC for updating users.

First, my index.php file acts as my bootstrapper.

index.php:

// start session
session_start();

// include config constants
include __DIR__.'/config.php';

// autoload classes via composer
include __DIR__.'/vendor/autoload.php';

// route user based on url path
$router = new router();
$router->set_properties();
$router->route();

Any URL that is visited will be turned into http://localhost/myapp/?path=controller/method/param etc. by my .htaccess file.

For example, for the update user URL, you visit http://localhost/myapp/user/update/12 and this would point to $user_controller->update(12).

router.php:

class router {
    public $controller;
    public $method;
    public $parameters = [];

    function set_properties () {
        if (isset($_GET['path'])) {
            // set controller from path
            $path = explode('/', $_GET['path']);
            $this->controller = $path[0].'_controller';
            unset($path[0]);

            // set method from path if it exists, otherwise use "index"
            if (isset($path[1])) {
                $this->method = $path[1];
                unset($path[1]);
            }
            else {
                $this->method = 'index';
            }

            // set parameters to remaining path elements
            $this->parameters = $path;
        }
        else {
            // path not set, show app index
            die('app index');
        }
    }

    function route () {
        if (method_exists($this->controller, $this->method)) {
            // controller method exists
            $controller = new $this->controller();
            call_user_func_array([$controller, $this->method], $this->parameters);
        }
        else {
            // controller method does not exist, show 404
            die('404');
        }
    }
}

I am trying to keep the controller as light as possible. Here is a sample of my controller code, which includes the update method referenced before.

controller.php:

class controller {
    public $sanitizer;
    public $redirector;

    function __construct () {
        $this->sanitizer = new sanitizer();
        $this->redirector = new redirector();
    }

    function view ($filename) {
        include __DIR__.'/views/'.$filename.'.php';
    }
}

user_controller.php:

class user_controller extends controller {
    public $user;

    function __construct () {
        parent::__construct();
        $this->user = new user();
    }

    function update ($id) {
        if ($this->user->exists_by_id($id)) {
            // user exists by id, fetch and show update form
            $this->user->get_by_id($id);

            if (!empty($_POST) && $this->user->validate($_POST)) {
                // update form posted, update user if input is valid
                $this->user->update($id);
            }

            $this->view('user_update');
        }
        else {
            // user does not exist by id, redirect to user index
            $this->redirector->redirect('user');
        }
    }
}

As you can see, the controller is only handling views and redirects. The rest of the business logic (like validation, checking existence) is done by the model.

Now here is where I begin to get frustrated. Is this controller light enough? Is the model doing what it should be?

model.php:

class model {
    public $pdo;
    public $validator;

    function __construct () {
        $this->pdo = new PDO('mysql:host='.APP_DATABASE_HOST.';dbname='.APP_DATABASE_NAME, APP_DATABASE_USERNAME, APP_DATABASE_PASSWORD);
        $this->validator = new validator();
    }
}

user.php:

class user extends model {
    public $id;
    public $username;
    public $email_address;

    function exists_by_id ($id) {
        // check if user exists by id
        $stm = $this->pdo->prepare('select count(*) from `user` where `id`=:id');
        $stm->bindParam(':id', $id);
        $stm->execute();

        return $stm->fetchColumn();
    }

    function get_by_id ($id) {
        // set user properties by id
        $stm = $this->pdo->prepare('select * from `user` where `id`=:id');
        $stm->bindParam(':id', $id);
        $stm->execute();
        $res = $stm->fetch();

        foreach ($res as $key => $value) {
            $this->$key = $value;
        }
    }

    function validate ($input) {
        // set properties with input
        foreach ($input as $key => $value) {
            $this->$key = $value;
        }

        // validate form input
        $this->validator->set_rule('username', 'Username', $this->username, ['required']);
        $this->validator->set_rule('email_address', 'Email Address', $this->email_address, ['required', 'email_address']);

        // return boolean result of validation attempt
        return $this->validator->validate();
    }

    function update ($id) {
        // update user by id
        $stm = $this->pdo->prepare('update `user` set `username`=:username, `email_address`=:email_address  where `id`=:id');
        $stm->bindParam(':username', $this->username);
        $stm->bindParam(':email_address', $this->email_address);
        $stm->bindParam(':id', $id);
        $stm->execute();
    }
}

I know that I should probably use an ORM or something, but I really prefer just using PDO queries directly. Also, should I be creating a method for setting the class properties via an array within the model class?

Looking at this code, am I doing everything correctly? I know I didn't show the view because it isn't coded yet, but the view will have access to the sanitizer and do checks for any session messages (such as errors and successes). That is it aside from HTML output.

Also, is this actually OOP or am I just infinitely stuck in this procedural pattern I can't get out of my head?

Answer 1

A few thoughts:

1)

Any URL that is visited will be turned into http://localhost/myapp/?path=controller/method/param etc. by my .htaccess file.

I know it is common to use .htaccess for this sort of thing, but when designing a true front-controller type of pattern like you are doing, why put part of the routing logic into an Apache configuration layer? You should probably have your main Apache .conf file simply route all requests to index.php in your web directory and turn off all .htaccess files to save your server the overhead. You then parse apart the URI in your router, validating each of the URI components to match what the router expects just as you are doing with the path parameter right now. Right now, using .htaccess brings you zero value.

2)

In index.php,

$router->set_properties();

Why require the caller to call this method? The caller is not passing any information to the router class at all, so I don't see why all the set_properties() activity is just not happening in the constructor when the class is instantiated.

3)

All of your code is very "happy path" oriented. You just assume everything is going to work and you do a generally poor job of dealing with executional failures in your code. For example:

• When you get a mal-formed URL or routes that don't exist, you just have the application die, no 400 series HTTP response code is sent to tell the caller they did something wrong.

• When interacting with the database, you assume everything is going to work. You are not checking the results of statement preparations, prepared statement executions, etc.

• You don't make any use of exception throwing/handling nor of any error logging.

• Most every method works on whatever parameter is passed without performing any validation. This is especially critical when passing parameters in on public methods.

4)

You might consider dependency injection. Right now you regularly use the approach of instantiating dependencies in class constructors. If you actually did take some above advice and properly handle all the possible things that could go wrong everytime you need to instantiate some resource or object, you will find yourself having a lot of dependency-specific logic in your classes.

With a dependency injection model, to can pass in valid instance of say a DB connection or validator class, with the class receiving that dependency knowing that it has a valid instance and not having to worry about all the instantiation handling for the object (or even knowing how to instantiate it at all).

You model class constructor could be rewritten like this:

function __construct (PDO $pdo, validator $validator) {
    $this->pdo = $pdo;
    $this->validator = $validator;
}

Much simpler right? You enforce the fact that there are proper dependencies passed right there in the method signature. You also don't end up needing to create new PDO object for each class in that you can now just pass around a valid PDO object (DB connection) to any class that needs it.

This example could also be applied directly to your controller class.

5)

function exists_by_id ($id) {
    // check if user exists by id
    $stm = $this->pdo->prepare('select count(*) from `user` where `id`=:id');
    $stm->bindParam(':id', $id);
    $stm->execute();

    return $stm->fetchColumn();
}

The above function could be problematic in it's return. I would suggest returning boolean true/false vs. an integer-like (0 or greater) string value like you are passing now. This could be very problematic with loose comparisons.

6)

function get_by_id ($id) {
    // set user properties by id
    $stm = $this->pdo->prepare('select * from `user` where `id`=:id');
    $stm->bindParam(':id', $id);
    $stm->execute();
    $res = $stm->fetch();

    foreach ($res as $key => $value) {
        $this->$key = $value;
    }
}

Consider getting away from SELECT * type queries. These can be problematic and end up sendig a lot more fields from the database to this application layer than needed. Think about a typical database that may have timestamp fields or similar fields that are not mean to be used by the outside world. I would actually specific name all the fields you are setting on the object rather the looping over a (possibly unknown) set of fields and automatically set them as public properties on the object.

Also, should this method have a return value? Possibly even return $this to allow for chaining of methods. Just seems odd to not give caller any response to indicate that the operation was successful.

7)

I would be explicit in defining accessibility of your methods (public, protected, private).