3
\$\begingroup\$

I would like to know if my Routing system respects the solid principle ?

The system is simple, Router class contains the routes and returns the correct route and Route class represents a route, contains a path and an action to call. Router takes the Request class as a parameter in its constructor to try to respect the solid principle.

I would like to know what's good and what isn't.

class Router
{
    private $request;
    private $routes = [];

    public function __construct(Request $request)
    {
        $this->request = $request;
    }
  
    public function get(string $path, string $action)
    {
        return $this->addRoute($path, $action, 'GET');
    }

    private function addRoute(string $path, string $action, string $method)
    {
        $route = new Route($path, $action);
        $this->routes[$method][] = $route;
        return $route;
    }

    public function run()
    {
        if(!isset($this->routes[$this->request->getMETHOD()])) {
            throw new RouterException('REQUEST_METHOD does not exist');
        }
        foreach ($this->routes[$this->request->getMETHOD()] as $route) {
            if ($route->match($this->request->getUri())) {
                return $route->call();
            }
        }
        throw new RouterException("La page demandée est introuvable.");
    }
}
class Route
{
    private $path;   
    private $action; 
    private $matches; 
    private $parameters = [];

    public function __construct(string $path, string $action)
    {
        $this->path = trim($path, '/');
        $this->action = $action;
    }

    public function where(string $param, string $regex)
    {
        $this->parameters[$param] = str_replace('(', '(?:', $regex);
        return $this; 
    }

    public function match(string $requestUri)
    {
        $requestUri = trim($requestUri, '/');
        $path = preg_replace_callback('#:([\w]+)#', [$this, 'paramMatch'], $this->path); 
        $regex = "#^$path$#";
        if (preg_match($regex, $requestUri, $matches)) {
            array_shift($matches);
            $this->matches = $matches;
            return true;
        } else {
            return false;
        }
    }

    private function paramMatch(array $match)
    {
        if (isset($this->parameters[$match[1]])) {
            return '(' . $this->parameters[$match[1]] . ')';
        }
        return '([^/]+)';
    }

    public function call()
    {
        $parameters = explode('@', $this->action);
        $controller = "Src\\Controller\\" . $parameters[0];
        $controller = new $controller();
        $method = $parameters[1];
        return call_user_func_array([$controller, $method], $this->matches);
    }
}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Router should not accept request object in constructor. You will need a new router for each request. I know PHP spawns a new process for each request so it gets created for each request anyway. But in tests it may be different... And anyway it doesn't make much sense to hide the request inside the router from the logical point of view. Hiding a constructor dependency means that the dependency is an implementation detail. And sure you agree that request is not just an implementation detail of a router. It is the input for routing.

Also the route should not be aware of how to determine controller class and surely it doesn't want to know or, and thats even worse, force no constructor arguments for all controllers. There should be a controller factory that knows the details.

And you don't want to pass controller factory to every route. So maybe instead, route should just define path mapping and controller factory should take a route as an input. Router would then pass the matched route to the controller factory.

Another thing is that the addRoute method doesn't really belong on a router, it belong on a router builder. You register routes on the builder and finalize by calling it's createRouter method.

What's entirely missing in your code is an "application" class - that's the one where run method belongs, not the router, router should really have a method that accepts request object and returns response object. Application might have a request factory, the router and some kind of response emitter which takes the response object returned by router and send it's representation to the client.

\$\endgroup\$
2
\$\begingroup\$

As a general best practice rule, I would advise that any time you are going to be feeding a variable into a regex pattern string AND that variable isn't 100% guaranteed to be free of characters with special meaning to the regex engine -- you should always bake in a preg_quote() call for stability/security.


Focusing on match() and paramMatch()...

Fundamentally, I don't see any re-usable utility in declaring paramMatch() as a single-use method. I think it should be declared as an anonymous function within match(). The lookup can be simplified using a null coalescing operator. I would also declare the method's return type and try to minimize the number of single-use variables declared. Trivially, \w doesn't not benefit from being packaged in a character class -- you can lose the square braces.

public function match(string $requestUri): bool
{
    $path = preg_replace_callback(
        '#:(\w+)#',
        function ($m) {
            return '(' . ($this->parameters[$m[1]] ?? '[^/]+') . ')';
        },
        $this->path
    );
    if (preg_match("#^" . $path . "$#", trim($requestUri, '/'), $matches)) {
        array_shift($matches);
        $this->matches = $matches;
        return true;
    } else {
        return false;
    }
}
    

If you want to have declarative variable names in call(), you might consider the following alternative style. I think prepending the namespace path to $this->action before exploding is tolerable to make the preparation more direct. Also, as a matter of habit, when I know the exact number of expected elements to be generated by an explode() call, I always declare the limit parameter so that the function can immediately stop assessing the string when satisfied.

[$controller, $method] = explode('@', "Src\\Controller\\" . $this->action, 2);
return call_user_func_array([new $controller(), $method], $this->matches);
\$\endgroup\$
1
  • \$\begingroup\$ I will see all this, make the modifications and I will post my corrected code to morrow. thank you for your explanation, is helpful !! \$\endgroup\$
    – user237529
    Feb 15, 2021 at 21:49

Your Answer

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