Framework to drive a small personal site

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:

```
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 al

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.

StackExchange Code Review Q#10538, answer score: 2