Creating an MVC router

Question

I created an MVC router based on knowledge of an MVC design pattern.

It will simply take a URL as a string (class/method/params...), then it will instantiate a requested class controller and call its method.

In my class, I call a controller method and then include the view at the same time to display data.

My questions are:

Is this a valid MVC router?
Is it good practice to make this class call include views for our application?

class FrontController {
    //default view
    private $view = 'index';
    //default methode
    private $method = 'indexMethode';
    //default controller
    private $controller = 'Page';
    //this attribute to store data returned
    private $data;
    //to store methode parameters from url.
    private $param;
    public function __construct() {
        //getting user url request
        $url = $this->getUrl();
        //checking user input
        if (isset($url[0])) {
              //getting class name from user input
              $this->controller = ucwords($url[0]);
              unset($url[0]);
        }
        //checking if class file exsiste if yes we include it
        if(file_exists('controllers/'.$this->controller.'.php')){
              require 'controllers/'.$this->controller.'.php';
        }
        //instantiating the demanded class
        $this->controller = new $this->controller;

        //getting demanded class method if it is demanded by user
        if (isset($url[1])) {
              if(method_exists($this->controller, $url[1]))
                  $this->method = $url[1];

        }

        //checking if there is a view for our request
        if(file_exists('view/'.$url[1].'.php')){
            //setting the sutable template for the methode
            $this->view = $url[1];
            unset($url[1]);
        }

        //Getting methode parameters from user input if exsist
        $this->param = $url ? array_values($url) : [];

        //Calling the methode and getting the result
        $this->data = call_user_func_array([$this->controller,
        $this->method], $this->param);

        //Getting the sutable page for  the responde to display it.
        include 'view/' . $this->view . '.php';
    }
    //To get any private var in our class
    public function __get($name) {
        return $this->$name;
    }
    //Here we are gona take user input and devide it as array element
    public function getUrl() {
      //checking user input and filtring it for unwanted charecters
        if (!empty($_GET['url'])) {
            // remove / char from url
            $url = rtrim($_GET['url'], '/');
            //Checking url for unwanted chars
            $url = filter_var($url, FILTER_SANITIZE_URL);
            //forming array from url
            $url = explode('/', $url);
            //Sending back url array
            return $url;
        }
    }
}

One problem I can see is that of security. Given this code anybody can call any of your public controller methods with any arguments. You will have to be very dissiplined and properly filter all incoming argument in all public controller methods. You might want to have public controller methods that are not callable through an url. — KIKO Software
– KIKO Software, Commented Jul 2, 2018 at 8:09

Greg Burghardt · Accepted Answer · 2018-07-06 02:13:55Z

This implementation of the Front Controller Pattern has knowledge over too many concerns. The Single Responsibility Principal states that a class should only have one reason to change. From what I can see if any of the following things change, this class will be affectetd:

The location of controller class files
The name of the controller classes
How controller classes are instantiated
The pattern used to match URLs
How view files are loaded
How controller methods are invoked
Which views correspond to which methods on a controller

All of these concerns mixed into one class means it is near impossible to unit test a very critical part of your application.

To be honest, if this is a small application it isn't too bad. If your application grows in size and complexity this will quickly show its limits.

The answer?

Split each concern into it's own class (Single Responsibility Principle)
Have each class interact with the others through an interface (Interface Segregation Principle)
Pass these objects to each other through their constructors (Dependency Inversion Principle)

Going with a minimum of 4 classes and interfaces gives you a lot more flexibility for your application to grow.

First, the interfaces:

interface IUrlRouter {
    function getRoute(Request $request) : UrlRoute;
    function getErrorRoute() : UrlRoute;
}

interface IControllerFactory {
    function createController($controllerName);
}

interface IActionResultExecutor {
    function executeResult($result);
}

Then your "front controller" really just becomes "the application" which handles the Big Picture steps of the process:

class MvcApplication {
    private $controllerFactory;
    private $resultExecutor;
    private $router;

    public function __construct(IUrlRouter $router, IControllerFactory $controllerFactory, IActionResultExecutor $resultExecutor) {
        $this->router = $router;
        $this->controllerFactory = $controllerFactory;
        $this->resultExecutor = $resultExecutor;
    }

    public function handleRequest(Request $request) {
        try {
            $route = $this->router->getRoute($request);
            $controllerName = $route->getControllerName();
            $actionName = $route->getActionName();
            $controller = $this->controllerFactory->createController($route->getControllerName());
            $result = $controller->$actionName($route->getParams());

            $this->resultExecutor->executeResult($result);
        }
        catch (Exception $ex) {
            $route = $this->router->getErrorRoute();
            $controller = $this->controllerFactory->createController($route->getControllerName());
            $controller->error($request, $ex);
        }
    }
}

And then just a smidgen of boiler plate code to kick start the process:

$app = new MvcApplication(new UrlRouter(), new ControllerFactory(), new ActionResultExecutor());
$request = new Request($_SESSION, $_SERVER, $_GET, $_POST);
$app->handleRequest($request);

And this doesn't even touch on user input validation, or resolving a view and then rendering it.

Dan · Accepted Answer · 2018-07-02 09:13:07Z

Dependency Injection

You instantiate all the controllers with new $this->controller what if my controller has dependencies ? Instead of your controllers looking like this;

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

They should look like this

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

and your router should be able to inject the dependencies I need.

call_user_func_array

Your use of call_user_func_array assumes the the client will send the correct order of parameters expected by the method,

You take the input array and order them to match what is expected by the method (look at reflection for getting the methods params)

All controllers are callable

You should be checking to see if the controller is callable by the router either implementing an interface like PublicController or using some sort of docBlock

This is just a small summary of the problems I see with this router others include;

Autoloading
Non absolute filepaths

Stack Exchange Network

Creating an MVC router

2 Answers 2

Dependency Injection

call_user_func_array

All controllers are callable

You must log in to answer this question.

Hot Network Questions

Creating an MVC router

2 Answers 2

Dependency Injection

call_user_func_array

All controllers are callable

You must log in to answer this question.

Related

Hot Network Questions