PHP Router For MVC with strict routing requirements

Question

I had previously asked the question here.

The response was pretty much "wow this is bad".

So I learned everything I could and wrote what I believe to be better using TDD.

The strict requirement is that the url will be parsed as /controller/action/everything/else/is/a/parameter.

So here is what I wrote:

The router class

<?php declare(strict_types=1); namespace xxxx\xxxx\Router; use xxxx\xxxx\Util\StorageUtils; class Router{ public array $defaultRoute; public array $notFoundRoute; public array $urlSegments; public $controller; public $action; public $parameters; public $controllerName; public $view; public function setDefaultRoute($controller, $action): void{ $this->defaultRoute = [$controller, $action]; } public function setNotFoundRoute($controller, $action): void{ $this->notFoundRoute = [$controller, $action]; } public function execute($url): void{ $this->getUrlSegments($url) ->determineController() ->createController() ->determineParameters() ->determineAction() ->executeAction() ->determineView(); } public function executeView(){ $this->controller->initData(); if($this->controller->data != null){ extract($this->controller->data, EXTR_PREFIX_SAME, 'wddx'); } if($this->view != null){ $viewPath = StorageUtils::getFullPath($this->getViewFile($this->controllerName, $this->view)); if($this->controller->useTemplate){ $templatePath = StorageUtils::getFullPath($this->getViewFile('Template', $this->controller->template)); require_once($templatePath); }else{ require_once($viewPath); } } $this->controller->init(); } public function executeAction(): self{ $action = $this->action; $this->controller->$action($this->parameters); return $this; } public function getUrlSegments($url): self{ $url = explode('/', ltrim($url,'/')); $this->urlSegments = $url; return $this; } public function determineController(): self{ if(count($this->urlSegments) > 0){ $controller = ucfirst($this->urlSegments[0]); if(empty($controller)){ $this->controllerName = $this->defaultRoute[0]; return $this; } if($this->controllerExist($controller)){ $this->controllerName = $controller; return $this; } } $this->controllerName = $this->notFoundRoute[0]; return $this; } public function createController(): self{ $controllerName = $this->controllerName; $this->requireFile($this->getControllerFile($controllerName)); $this->controller = new $controllerName(); return $this; } public function determineParameters(): self{ $this->parameters = array_slice($this->urlSegments, 2); return $this; } public function determineAction(): self{ $action = 'index'; if(count($this->urlSegments) > 1){ if(strlen($this->urlSegments[1]) > 0){ $action = $this->urlSegments[1]; } } if(!$this->actionExist($action)){ $this->controllerName = 'ErrorPages'; $this->createController(); $action = $this->notFoundRoute[1]; } $this->action = $action; return $this; } public function determineView(): self{ $this->view = $this->controller->view; if(!is_string($this->view)){ $this->view = null; } if(!$this->viewExist($this->controllerName, $this->view) && $this->view != null){ $this->controllerName = 'ErrorPages'; $this->createController(); $this->action = $this->notFoundRoute[1]; $this->executeAction(); $this->view = '404'; } return $this; } public function controllerExist($controllerName): bool{ return StorageUtils::fileExist(StorageUtils::getFullPath($this->getControllerFile($controllerName))); } public function actionExist($action): bool{ return method_exists($this->controller, $action); } public function viewExist($controllerName, $view): bool{ return StorageUtils::fileExist(StorageUtils::getFullPath($this->getViewFile($controllerName, $view))); } public function getControllerFile($controllerName): string{ return "/src/Application/$controllerName/Controller/$controllerName.php"; } public function getViewFile($controllerName, $viewName): string{ return "/src/Application/$controllerName/View/$viewName.php"; } private function requireFile($route): void{ require_once(StorageUtils::getFullPath($route)); } } ?> 

The corresponding test class

<?php namespace xxxx\xxxx\Test; use PHPUnit\Framework\TestCase; use xxxx\xxxx\Router\Router; class RouterTest extends TestCase{ private $router; public function setUp(): void{ $this->router = new Router(); $this->router->setDefaultRoute('Home', 'index'); $this->router->setNotFoundRoute('ErrorPages', 'notFound'); } /** * @covers getUrlSegments */ public function testGetUrlSegments(){ $returnValue = $this->router->getUrlSegments('/test/index/111/222'); $this->assertNotEmpty($this->router->urlSegments); $this->assertIsArray($this->router->urlSegments); $this->assertEquals('test', $this->router->urlSegments[0]); $this->assertEquals('index', $this->router->urlSegments[1]); $this->assertEquals('111', $this->router->urlSegments[2]); $this->assertEquals('222', $this->router->urlSegments[3]); $this->assertEquals(Router::class, $returnValue::class); } /** * @covers setDefaultRoute */ public function testSetDefaultRoute(){ $this->assertEquals('Home', $this->router->defaultRoute[0]); $this->assertEquals('index', $this->router->defaultRoute[1]); } /** * @covers setNotFoundRoute */ public function testSetNotFoundRoute(){ $this->assertEquals('ErrorPages', $this->router->notFoundRoute[0]); $this->assertEquals('notFound', $this->router->notFoundRoute[1]); } public function routeProvider(){ return [ ['/test', 'Test'], ['/test/sdf24', 'Test'], ['/home', 'Home'], ['/', 'Home'], ['', 'Home'], ['/home/', 'Home'], ['adafeadsdas', 'ErrorPages'], ['ad/asd/efwefd/qed', 'ErrorPages'], ['!@#@!#@', 'ErrorPages'], ['/!@@$2323', 'ErrorPages'], ['/.html', 'ErrorPages'], ['/someNonExistentController', 'ErrorPages'], ['/%', 'ErrorPages'], ['/@@$%$%%5E', 'ErrorPages'] ]; } /** * @covers determineController * @dataProvider routeProvider */ public function testDetermineController($url, $expectedResult){ $this->router->getUrlSegments($url) ->determineController(); $this->assertEquals($expectedResult, $this->router->controllerName); } /** * @covers createController * @dataProvider routeProvider */ public function testCreateController($url, $expected){ $this->router->getUrlSegments($url) ->determineController() ->createController(); $this->assertEquals($expected, $this->router->controller::class); } public function actionDataProvider(){ return [ ['/test', 'index'], ['/test/doesNotExist', 'notFound'], ['/test/test', 'test'], ['/test/!#$@#$ad', 'notFound'], ['/test/', 'index'] ]; } /** * @covers determineAction * @dataProvider actionDataProvider */ public function testDetermineAction($url, $expected){ $this->router->getUrlSegments($url) ->determineController() ->createController() ->determineAction(); $this->assertEquals($expected, $this->router->action); } /** * @covers determineAction */ public function testDetermineActionChangesControllerOnActionNotExist(){ $this->router->getUrlSegments('/test/doesNotExist') ->determineController() ->createController() ->determineAction(); $this->assertEquals($this->router->notFoundRoute[0], $this->router->controllerName); $this->assertEquals($this->router->notFoundRoute[1], $this->router->action); $this->assertEquals(\ErrorPages::class, $this->router->controller::class); } public function parameterDataProvider(){ return [ ['/test', []], ['/test/index', []], ['/test/index/one', ['one']], ['/test/index/one/two/three', ['one', 'two', 'three']], ['/test/index/3,d,/!#$/', ['3,d,', '!#$', '']], ['/text/index/index.html', ['index.html']] ]; } /** * @covers determineParameters * @dataProvider parameterDataProvider */ public function testDetermineParameters($url, $expected){ $this->router->getUrlSegments($url) ->determineParameters(); $this->assertEquals($expected, $this->router->parameters); } public function viewDataProvider(){ return [ ['/test/validView', 'ValidView'], ['/test/invalidView', '404'] ]; } /** * @covers determineView * @dataProvider viewDataProvider */ public function testDetermineView($url, $expected){ $this->router->execute($url); $this->assertEquals($expected, $this->router->view); } public function printedReturnDataProvider(){ return [ ['/test/printedReturnView', null], ['/test/printRReturnView', null] ]; } /** * @covers determineView * @dataProvider printedReturnDataProvider */ public function testDetermineViewWithPrintedReturnData($url, $expected){ $this->router->execute($url); $this->expectOutputString('{"randomData":"dsdsddd"}'); $this->assertEquals($expected, $this->router->view); } /** * @covers determineView */ public function testDetermineViewChangesControllerOnViewNotExist(){ $this->router->execute('/test/invalidView'); $this->assertEquals('ErrorPages', $this->router->controllerName); } } 

I realize that there is probably a lot of room to grow here and I'm curious to know how I did this time around and where improvements can be made.

Almost forgot to add what the index.php file looks like now.

use xxxx\xxxx\Util\StorageUtils; use xxxx\xxxx\Router\Router; if(isset($argv)){ $options = getopt('', ['uri::']); $url = $options['uri']; }else{ if($engine == 'nginx'){ $url = $_SERVER['REQUEST_URI']; }else if($engine == 'apache'){ $url = $_SERVER['PATH_INFO']; } } $router = new Router(); $router->setDefaultRoute('Home', 'index'); $router->setNotFoundRoute('ErrorPages', 'notFound'); $router->execute($url); $router->executeView(); 

Answer 1

Your router is doing too much.

Router should do one thing and one thing only. It should map request uri to a specific request handler (controller action).

You may get a better architecture if you think of router as being stateless. Actually this principle will serve you well far beyond just router.

Router should not instantiate controllers. That's job of controller factory. Furthermore controllers usually need some services to act upon. These services should be injected to the controller via constructor injection (there are other means of injection, but constructor injection is by far the best). Even if you don't see the need now, believe me it will show itself later. Unless you're doing something like calling everything via static methods, which is kinda bad on its own.

Router should not render views. That's job of your rendering engine to execute it. And it's job of controllers to invoke the execution.

Router also should not try to be the entire application. There usually is yet another class that uses a router to determine controller, but it is the application which then asks controller factory to instantiate controller and then it executes it but this happens after the router's job is already done.

Another way to think of router is just that it is a request handler itself. It is the controller for all routes and if receives a specific route for which there is a specific handler/controller, it delegates to it.

When I say "request handler" I mean specifically the RequestHandlerInterface of PSR-15 https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-15-request-handlers.md#21-psrhttpserverrequesthandlerinterface.

Consider implementing router such that it implements this interface and such that the handle method is the only public method of the router. To achieve configurabiloty of the router, you should split the implementation into two objects - router builder and the router itself. The builder has methods for configuration and creating of the router. Router is the RequestHandlerInterface implementation and all configuration is passed to it via constructor (inside the RouterBuilder::build() method).

$builder = new RouterBuilder($controllerFactory); $builder->get('/test', 'Test', 'index'); $router = $builder->build(); $request = ServerRequest::fromGlobals(); $response = $router->handle($request); sendResponse($response); 

Answer 2

Code styling (refer to PSR-12 guidelines)

• Mind your class bracing and spacing: https://www.php-fig.org/psr/psr-12/#41-extends-and-implements
• Mind your method bracing and spacing: https://www.php-fig.org/psr/psr-12/#44-methods-and-functions
• elseif is one word in PHP: https://www.php-fig.org/psr/psr-12/#51-if-elseif-else

Avoid making loose comparisons whenever appropriate. != null will return true for a number of falsey values. If you want "not null", then use === null. If you are happy to greedily/loosely evaluate falsey values the simply use !$var.

Unless you have a good, sound reason to the contrary, avoid making single-use variable declarations like $url = explode.... Instead write $this->urlSegments = explode... in a single line.

if(count($this->urlSegments) > 0) can lose the > 0 and maintain the same logic because an array cannot have an negative count and an empty array will evaluate as falsey.

It doesn't make sense to check if(empty($controller)){ if the variable is guaranteed to be declared. Instead, you should should check if the first segment is a zero length string before converting its first character to uppercase (you might potentially want to look into title_casing which would make the whole word lowercase and the first letter uppercase).

if(count($this->urlSegments) > 1){ if(strlen($this->urlSegments[1]) > 0){ seems reasonably condensed into one statement with &&. It also feels reasonable to just check !empty($this->urlSegments[1]) since the action can be presumed to be a "word".

Overall, I see a lot of sensible method naming and the single-responsibility principle looking fairly implemented. While it is possibile that I've overlooked something in the "big picture", I'd say this latest code isn't: "wow this is bad".