4
\$\begingroup\$

I'm learning the mvc pattern and today I finished my first mvc routing class. Hope you can give me some tipps and improvements.

Router.php

<?php

namespace System\Libraries;

class Router
{
    /**
     * Store the given parameters
     *
     * @var array
     */
    private $_param = [];

    /**
     * Default action
     *
     * @var bool
     */
    private $_action = 'index';

    /**
     * Store the given request
     *
     * @var array
     */
    private $_request = [];

    /**
     * Default controller
     *
     * @var bool
     */
    private $_controller = 'index';

    /**
     * Constructor
     */
    public function __construct()
    {
        $this->splitUrl();
        $this->setRoute();
    }

    /**
     * Split the url
     *
     * @return void
     */
    public function splitUrl()
    {
        if (isset($_GET['url']) && !empty($_GET['url'])) {
            $this->_request = trim(filter_var(strtolower($_GET['url']), FILTER_SANITIZE_URL));
            $this->_request = explode('/', rtrim($this->_request, '/'));
        } else {
            $this->_request = null;
        }
    }

    /**
     * Check the request and load the controller
     *
     * @return void
     */
    public function setRoute()
    {
        if (isset($this->_request[0])) {
            if (file_exists(ROOT . '/app/controllers/' . $this->_request[0] . '.php')) {
                $this->_controller = $this->_request[0];
            } else {
                $this->_controller = 'error';
            }
        }

        $this->_controller = 'App\Controllers\\' . ucfirst($this->_controller);
        $this->_controller = new $this->_controller;

        if (isset($this->_request[1])) {
            if (method_exists($this->_controller, $this->_request[1] . 'Action')) {
                $this->_action = $this->_request[1];
            } else {
                $this->_action = 'error';
            }
        }

        unset($this->_request[0], $this->_request[1]);

        if (isset($this->_request[2])) {
            $this->_param = array_values($this->_request);
        }

        call_user_func_array([$this->_controller, $this->_action . 'Action'], $this->_param);
    }

    /**
     * Destory the variables
     *
     * @return void
     */
    public function __destruct()
    {
        $this->_param      = null;
        $this->_model      = null;
        $this->_action     = null;
        $this->_request    = null;
        $this->_controller = null;
    }
}

?>
\$\endgroup\$
1
  • \$\begingroup\$ A router doesnt have anything to do with MVC. It's just a piece of software that translates 'var A' to 'action for A'. Maybe look into the symfony HTTPKernel documentation: symfony.com/doc/current/components/http_kernel/… \$\endgroup\$
    – Pinoniq
    Dec 11, 2015 at 12:18

2 Answers 2

3
\$\begingroup\$

At first glance, this router is doing too much:

  1. Extracts relevant information from the HTTP request
  2. Instantiates a controller object
  3. Executes a method on the controller
  4. Does a little error handling if no controller exists

Really a router should only be responsible for item #1: Extracting relevant information from the HTTP request, and return that information in the form of a concrete object. What you have is the name of a controller, the name of an action, and an array of values to pass into the controller action:

class RouteData
{
    private $controllerName;
    private $controllerAction;
    private $arguments;

    public function __construct($controllerName, $controllerAction, $arguments) {
        $this->controllerName = $controllerName;
        $this->controllerAction = $controllerAction;
        $this->arguments = isset($arguments) ? $arguments : array();
    }

    public function getControllerAction() {
        return $this->controllerAction;
    }

    public function getControllerName() {
        return $this->controllerName;
    }

    public function getArguments() {
        return $this->arguments;
    }
}

You should provide all values in the constructor so the object is immutable. The route data should not change once it has been created.

Now that we've encapsulated the route data, the constructor of the Router class also does too much. It should be refactored so that it doesn't do anything but take in the data it needs to do its job. Offhand, the super global $_GET variable is used. This would be better if it were passed in to the method that creates the route data:

class Router
{
    public function getRouteData($getParams) {
        // Extract info, return a new RouteData object
    }
}

Your router doesn't really need persistent state. The splitUrl method should return an array of values instead of setting properties on the Router object. In fact, renaming this to createRouteValues and passing in the $_GET array would be better:

private function createRouteValues($getParams) {
    $routeValues = array();

    if (isset($getParams['url']) && !empty($getParams['url'])) {
        $url = trim(filter_var(strtolower($getParams['url']), FILTER_SANITIZE_URL));
        $routeValues = explode('/', rtrim($url, '/'));
    }

    return $routeValues;
}

Now it's just a matter of pulling out the necessary pieces of the array and creating the RouteData object:

public function getRouteData($getParams) {
    $routeValues = $this->createRouteValues($getParams);
    $controllerName = isset($routeValues[0]) ? $routeValues[0] : 'error';
    $controllerAction = isset($routeValues[1]) ? $routeValues[1] : 'index';
    $arguments = count($routeValues) > 2 ? array_slice($routeValues, 2) : array();

    return new RouteData($controllerName, $controllerAction, $arguments);
}

Notice that the Router does not do any error handling, nor does it try to resolve a controller name to a class, create the controller or execute the action. It's only responsible for turning an HTTP request into a RouteData object. Something else in the system needs to take the RouteData, get a controller object, execute an action on the controller and perform error handling. For that, we need two more classes. One that creates the controller object, and another that invokes an action on the controller and does some error handling.

First, the controller factory:

class ControllerFactory
{
    private $rootDirectory;

    public function __construct($rootDirectory) {
        $this->rootDirectory = $rootDirectory;
    }

    public function createController(RouteData route) {
        $filePath = $this->createFilePath(route->getControllerName());

        if (!file_exists($filePath)) {
            return new ErrorController();
        }

        $class = 'App\Controllers\\' . ucfirst($route->getControllerName());
        $controller = new $class();

        return $controller;
    }

    public function getErrorController() {
        return new ErrorController();
    }

    private function createFilePath($controllerName) {
        return $this->rootDirectory . '/app/controllers/' . $controllerName . '.php';
    }
}

The controller factory does a little error handling, in that it will return an ErrorController object if the route does not resolve to a controller class. It also provides a public getErrorController method that will be used to return a generic "error controller object" if one is needed.

The last thing we will do is create an "application" class that glues all the pieces together:

class Application
{
    private $controllerFactory;
    private $router;

    public function __construct(ControllerFactory $controllerFactory, Router $router) {
        $this->controllerFactory = $controllerFactory;
        $this->router = $router;
    }

    public function handleRequest() {
        try {
            $route = $this->router->getRouteData($_GET);
            $controller = $this->controllerFactory->createController($route);
            $method = $route->getControllerAction() . 'Action';

            if (!method_exists($controller, $method)) {
                throw new Exception("Method $method does not exist for controller {$route->getControllerName()}");
            }

            call_user_func_array([$controller, $method], $route->getArguments());
        } catch (Exception $ex) {
            $controller = $this->controllerFactory->getErrorController();
            $controller->indexAction($ex);
            // Log the exception some place
        }
    }
}

The Application class has two dependencies: a controller factory and a router. Both of these are passed in to the constructor, and have PHP Type Hints ensuring that you pass the correct kind of object. Everything happens in the handleRequest method, which orchestrates the interaction between the router and controller factory, and is responsible for invoking an action on the controller. The "Application" has a big picture view of things, and is an ideal place to do error handling, hence the try-catch block in the handleRequest method.

Lastly, let's write a quick PHP file (index.php) for your site:

<?php

$controllerFactory = new ControllerFactory(dirname(__FILE__) . '..');
$router = new Router();
$app = new Application($controllerFactory, $router);
$app->handleRequest();

A full MVC application in 4 lines of code.

\$\endgroup\$
3
  • \$\begingroup\$ this is a monolithic approach. A modern MVC framework should be modeled after the Request Response pattern, and all file loading should occur via an autoloader. Other than that the controller factory is fine, however i still feel like that is something the application class should handle internally. ideally the index.php for an automatic MVC, like what is posed in the OP would contain only (new Application)->handleRequest(), keeping all of the concerns contained in the Application's handleRequest method. \$\endgroup\$
    – r3wt
    Dec 10, 2015 at 15:28
  • \$\begingroup\$ @r3wt: I completely agree. This was an attempt at introducing some looser coupling. The original behavior of the OP's router is pretty simple, so I was trying to keep this as simple as possible without diving down the hole of a full blown, properly decoupled MVC framework. I +1'd your answer, BTW. \$\endgroup\$ Dec 10, 2015 at 15:31
  • \$\begingroup\$ That's very true. I +1'd your answer as well. A full blown MVC wouldn't fit in a single answer. \$\endgroup\$
    – r3wt
    Dec 10, 2015 at 15:36
1
\$\begingroup\$

A couple observations:

setRoute() can result in a fatal exception if $this->_request = null. you could fix this by utilizing short circuiting to eliminate the nested loop in your first condition check:

if (isset($this->_request[0]) && file_exists(ROOT . '/app/controllers/' . $this->_request[0] . '.php')) {
    $this->_controller = $this->_request[0];
} else {
    $this->_controller = 'error';
}

I've never seen anyone use implicit destructors in PHP, since objects are destroyed automatically as soon as they leave scope. It would make sense if you were building some sort of long running script as a best practice to avoid potential leakage, but for simple RR patterns, its pointless in my opinion.

Final Remarks: the source code of Slim Framework 2.6.x branch is required reading for studying the optimal design of a RMVC, and there is a ton of useful knowledge to be gleaned from studying how the router works.

If you're into framework portability for the code you will potentially write, you may want to get up to speed with PSR-7, and then delve into Slim's new 3.x branch, though i have no interest in it myself as i think it is a bad move for PHP-FIG.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.