Framework to drive a small personal site

Question

I have been writing a very slim MVC framework to drive a small personal website. It's nothing intricate, just mostly for fun and a learning experience.

Specifically, this is the initial part of the MVC and just the entry point. I don't want to develop all this logic for the models and views if my dispatcher is not going to work right in a production site. Again, this is just the entry of the project and handles requests to load the controllers via the URL. This is not the entire MVC project.

When I am looking at other frameworks out there, they seem quite complex in their handling of the routing and controller dispatching, and my final code is just a fraction of the size.

I have the server redirect all requests back to index.php using an .htaccess file or via a Nginx config file. I have tried both.

I use the index.php file as the entry point (this is the relevant part):

/* using "Example Implementation" of the PSR-0 standards supporting both '\' and '_' namespace seperations
 * https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-0.md
 * using file_exists to deal with otherwise unhandled errors 
 */
function autoload($className) {
    $className = ltrim($className, '\\');
    $fileName  = '';
    $namespace = '';

    if($lastNsPos = strripos($className, '\\')) {
        $namespace = substr($className, 0, $lastNsPos);
        $className = substr($className, $lastNsPos + 1);
        $fileName  = str_replace('\\', DS, $namespace) . DS;
    }

    $fileName .= str_replace('_', DS, $className) . '.php';

    $fileName = ROOT . DS . $fileName;  
    if(file_exists($fileName)) {
        require($fileName);
    }
}

$test = new framework\core\Router();

Router.php:

<?php

namespace framework\core {

class Router {      

    function __construct() {            
        //get the raw url
        $urlRaw = $_SERVER['REQUEST_URI'];

        //do the route :)
        $this->route($urlRaw);
    }

    private function route($urlRaw) {
        //do our best to split the url at the most obvious parts (/, ?, $, =)
        $urlRawParts = preg_split('/[\/?&=]+/', $urlRaw, -1, PREG_SPLIT_NO_EMPTY);

        //do a simple sanitization on the parts (allow alphanumeric and dash while stripping common web extensions)
        $urlRawParts = preg_replace('/[^A-Za-z0-9\-]|\bhtm\b|\bphp\b|\bhtml\b/', '', $urlRawParts);

        //set our controller and action or use defaults
        $controller =   ((!empty($urlRawParts[0])) ? $urlRawParts[0] : 'Index');
        $action =       ((!empty($urlRawParts[1])) ? $urlRawParts[1] : 'index');            
        $arguments =    ((count($urlRawParts) > 2) ? array_slice($urlRawParts, 2) : array());           

        $this->dispatch($controller, $action, $arguments);
    }

    private function dispatch($controller, $action, $arguments) {           
        //format the call to our class
        $controller = 'application\\controllers\\' . ucfirst($controller) . 'Controller';

        //load our class or show the appropriate error page 
        $controller = ((class_exists($controller) ? new $controller() : new Error('404')));

        //use the default class method unless a valid one exists
        $method = ((method_exists($controller, $action)) ? $action : 'index');

        //call our class method
        call_user_func_array(array($controller, $method), $arguments);      
    }
}
}

?>

This works for any URL that I throw at it and will always default to a known file (Error.php which is just a simplified controller and view in one).

Am I missing anything huge here? Is my logic way off or not complete? Is this utilization slow or wasteful on resources? I'm just looking for a general idea on if I am on the right track.

Answer 1

All the basic parts are here, but there's still room for improvement.

First of all, I wouldn't have the constructor call route and route then call dispatch because it makes the second method less discoverable when the code is being read. Contrast this with:

function __construct() {            
    //get the raw url
    $urlRaw = $_SERVER['REQUEST_URI'];

    $route = $this->route($urlRaw); // don't care what $route is exactly
    $this->dispatch($route);
}

Here it is much more obvious what you method you should be looking for. It's true that you, as creator of the code, do not need this -- but others might, and it's a totally free lunch.

Second: this line bakes an application configuration decision inside the router. You don't want to do this, as it's totally reasonable to want to change this configuration most of the time while the router would stay mostly untouched.

$controller =   ((!empty($urlRawParts[0])) ? $urlRawParts[0] : 'Index');

It's much better to make the default controller name part of the application configuration and have the router take it from there, because it's reasonable to want your "home" page to be equivalent to the "/something" url instead of "/index".

The same goes for

$action =       ((!empty($urlRawParts[1])) ? $urlRawParts[1] : 'index');

It would be better if you allowed the controller writer to pick which action is the default:

$action = ((!empty($urlRawParts[1])) ? 'action'.$urlRawParts[1] : 'defaultAction');

But why also rename index to defaultAction and add the "action" prefix to method names? Consider also your dispatch code:

$method = ((method_exists($controller, $action)) ? $action : 'index');

This check with method_exists would allow any url that maps to a public function to be successfully dispatched, even if that function does not map to a "real" action (it can simply be a helper method). That's not really a security issue (you can easily make sensitive functions private) but it's not very consistent: targeting nonExistentAction will get you to the index page, while targeting e.g. helperMethod will show a blank page as helperMethod most likely does not directly produce content.

By demanding that actions are implemented in functions with names starting with "action" you can make sure that this never happens. The default action is a special exception to this rule, as it allows the controller writer to effectively specify which url routes to the default action and what the default action should do at the same time.

Third: It's not consistent to show a 404 for a missing controller but not do that for a missing action and use the default one instead. You should do the same in both cases, and the right choice would be the 404.

Fourth: Parameter handling needs immediate attention.

To begin with, you are sanitizing parameters to an unreasonably restricted set. What if there's a search action somewhere and the user wants to type in a character like * or a non-alphanum string? Clearly parameter sanitization needs to simply be removed.

Apart from that, you are passing GET parameters to the controller action positionally. The positional part is not a dealbreaker by itself (although it would be a dealbreaker in a framework, or if the URL format were configurable), but the GET restriction is a bit ugly. However, this cannot be fixed easily because there's no good way to know where parameters from $_POST should be inserted, or what their order relative to one another should be.

To address this, you need to make the code reflect into the controller method and look for argument names and default values, pull these out from your list of GET parameters and $_POST, place them in an array ordered by the position of each named parameter in the function signature and call the function with that. You should also probably return an HTTP 400 if the action turns out to have a non-optional parameter the value of which was not provided.

All in all not the end of the world, but a speed bump and a non-trivial amount of code.

Answer 2

You are basically on the right track though you could use:

$controller->$method($arguments);

instead of call_user_func_array.

The reason that other routers are more complicated is because they do a lot more. For example, you have a direct map between url and controller which means you can't change your urls without changing your code.

But again, if your functionality suffices for your applications then go with it. You can always refactor later.