Merge pull request #42 from clue/route-handler

Refactor to move router logic to new `RouteHandler` and improve middleware execution
clue · Sep 20, 2021 · 9172322 · 9172322
2 parents 25c44b0 + a568d96
commit 9172322
Show file tree

Hide file tree

Showing 5 changed files with 187 additions and 99 deletions.
diff --git a/src/App.php b/src/App.php
@@ -2,10 +2,6 @@
 
 namespace FrameworkX;
 
-use FastRoute\DataGenerator\GroupCountBased as RouteGenerator;
-use FastRoute\Dispatcher\GroupCountBased as RouteDispatcher;
-use FastRoute\RouteCollector;
-use FastRoute\RouteParser\Std as RouteParser;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
 use React\EventLoop\Loop;
@@ -21,12 +17,12 @@
 class App
 {
     private $loop;
-    private $middleware;
-    private $router;
-    private $routeDispatcher;
 
-    /** @var ErrorHandler */
-    private $errorHandler;
+    /** @var MiddlewareHandler */
+    private $handler;
+
+    /** @var RouteHandler */
+    private $router;
 
     /**
      * Instantiate new X application
@@ -55,17 +51,21 @@ class App
      */
     public function __construct($loop = null, callable ...$middleware)
     {
-        $this->errorHandler = new ErrorHandler();
+        $errorHandler = new ErrorHandler();
         if (\is_callable($loop)) {
             \array_unshift($middleware, $loop);
             $loop = null;
         } elseif (\func_num_args() !== 0 && !$loop instanceof LoopInterface) {
-            throw new \TypeError('Argument 1 ($loop) must be callable|' . LoopInterface::class . ', ' . $this->errorHandler->describeType($loop) . ' given');
+            throw new \TypeError('Argument 1 ($loop) must be callable|' . LoopInterface::class . ', ' . $errorHandler->describeType($loop) . ' given');
         }
 
         $this->loop = $loop ?? Loop::get();
-        $this->middleware = $middleware;
-        $this->router = new RouteCollector(new RouteParser(), new RouteGenerator());
+        $this->router = new RouteHandler();
+
+        // new MiddlewareHandler([$errorHandler, ...$middleware, $routeHandler])
+        \array_unshift($middleware, $errorHandler);
+        $middleware[] = $this->router;
+        $this->handler = new MiddlewareHandler($middleware);
     }
 
     public function get(string $route, callable $handler, callable ...$handlers): void
@@ -110,12 +110,7 @@ public function any(string $route, callable $handler, callable ...$handlers): vo
 
     public function map(array $methods, string $route, callable $handler, callable ...$handlers): void
     {
-        if ($handlers) {
-            $handler = new MiddlewareHandler(array_merge([$handler], $handlers));
-        }
-
-        $this->routeDispatcher = null;
-        $this->router->addRoute($methods, $route, $handler);
+        $this->router->map($methods, $route, $handler, ...$handlers);
     }
 
     public function redirect($route, $target, $code = 302)
@@ -303,12 +298,7 @@ private function sendResponse(ServerRequestInterface $request, ResponseInterface
      */
     private function handleRequest(ServerRequestInterface $request)
     {
-        $handler = function (ServerRequestInterface $request) {
-            return $this->routeRequest($request);
-        };
-        $handler = new MiddlewareHandler(\array_merge([$this->errorHandler], $this->middleware, [$handler]));
-
-        $response = $handler($request);
+        $response = ($this->handler)($request);
         if ($response instanceof \Generator) {
             if ($response->valid()) {
                 $response = $this->coroutine($response);
@@ -320,34 +310,6 @@ private function handleRequest(ServerRequestInterface $request)
         return $response;
     }
 
-    private function routeRequest(ServerRequestInterface $request)
-    {
-        if (\strpos($request->getRequestTarget(), '://') !== false || $request->getMethod() === 'CONNECT') {
-            return $this->errorHandler->requestProxyUnsupported($request);
-        }
-
-        if ($this->routeDispatcher === null) {
-            $this->routeDispatcher = new RouteDispatcher($this->router->getData());
-        }
-
-        $routeInfo = $this->routeDispatcher->dispatch($request->getMethod(), $request->getUri()->getPath());
-        switch ($routeInfo[0]) {
-            case \FastRoute\Dispatcher::NOT_FOUND:
-                return $this->errorHandler->requestNotFound($request);
-            case \FastRoute\Dispatcher::METHOD_NOT_ALLOWED:
-                return $this->errorHandler->requestMethodNotAllowed($routeInfo[1]);
-            case \FastRoute\Dispatcher::FOUND:
-                $handler = $routeInfo[1];
-                $vars = $routeInfo[2];
-
-                foreach ($vars as $key => $value) {
-                    $request = $request->withAttribute($key, rawurldecode($value));
-                }
-
-                return $handler($request);
-        }
-    } // @codeCoverageIgnore
-
     private function coroutine(\Generator $generator): PromiseInterface
     {
         $next = null;

diff --git a/src/RouteHandler.php b/src/RouteHandler.php
@@ -0,0 +1,73 @@
+<?php
+
+namespace FrameworkX;
+
+use FastRoute\DataGenerator\GroupCountBased as RouteGenerator;
+use FastRoute\Dispatcher\GroupCountBased as RouteDispatcher;
+use FastRoute\RouteCollector;
+use FastRoute\RouteParser\Std as RouteParser;
+use Psr\Http\Message\ResponseInterface;
+use Psr\Http\Message\ServerRequestInterface;
+use React\Promise\PromiseInterface;
+
+/**
+ * @internal
+ */
+class RouteHandler
+{
+    /** @var RouteCollector */
+    private $routeCollector;
+
+    /** @var ?RouteDispatcher */
+    private $routeDispatcher;
+
+    /** @var ErrorHandler */
+    private $errorHandler;
+
+    public function __construct()
+    {
+        $this->routeCollector = new RouteCollector(new RouteParser(), new RouteGenerator());
+        $this->errorHandler = new ErrorHandler();
+    }
+
+    public function map(array $methods, string $route, callable $handler, callable ...$handlers): void
+    {
+        if ($handlers) {
+            $handler = new MiddlewareHandler(array_merge([$handler], $handlers));
+        }
+
+        $this->routeDispatcher = null;
+        $this->routeCollector->addRoute($methods, $route, $handler);
+    }
+
+    /**
+     * @return ResponseInterface|PromiseInterface<ResponseInterface>|\Generator
+     */
+    public function __invoke(ServerRequestInterface $request)
+    {
+        if (\strpos($request->getRequestTarget(), '://') !== false || $request->getMethod() === 'CONNECT') {
+            return $this->errorHandler->requestProxyUnsupported($request);
+        }
+
+        if ($this->routeDispatcher === null) {
+            $this->routeDispatcher = new RouteDispatcher($this->routeCollector->getData());
+        }
+
+        $routeInfo = $this->routeDispatcher->dispatch($request->getMethod(), $request->getUri()->getPath());
+        switch ($routeInfo[0]) {
+            case \FastRoute\Dispatcher::NOT_FOUND:
+                return $this->errorHandler->requestNotFound($request);
+            case \FastRoute\Dispatcher::METHOD_NOT_ALLOWED:
+                return $this->errorHandler->requestMethodNotAllowed($routeInfo[1]);
+            case \FastRoute\Dispatcher::FOUND:
+                $handler = $routeInfo[1];
+                $vars = $routeInfo[2];
+
+                foreach ($vars as $key => $value) {
+                    $request = $request->withAttribute($key, rawurldecode($value));
+                }
+
+                return $handler($request);
+        }
+    } // @codeCoverageIgnore
+}
diff --git a/tests/AppMiddlewareTest.php b/tests/AppMiddlewareTest.php
@@ -2,9 +2,8 @@
 
 namespace FrameworkX\Tests;
 
-use FastRoute\RouteCollector;
 use FrameworkX\App;
-use FrameworkX\MiddlewareHandler;
+use FrameworkX\RouteHandler;
 use PHPUnit\Framework\TestCase;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
@@ -22,8 +21,8 @@ public function testGetMethodWithMiddlewareAddsGetRouteOnRouter()
         $middleware = function () {};
         $controller = function () { };
 
-        $router = $this->createMock(RouteCollector::class);
-        $router->expects($this->once())->method('addRoute')->with(['GET'], '/', new MiddlewareHandler([$middleware, $controller]));
+        $router = $this->createMock(RouteHandler::class);
+        $router->expects($this->once())->method('map')->with(['GET'], '/', $middleware, $controller);
 
         $ref = new \ReflectionProperty($app, 'router');
         $ref->setAccessible(true);
@@ -39,8 +38,8 @@ public function testHeadMethodWithMiddlewareAddsHeadRouteOnRouter()
         $middleware = function () {};
         $controller = function () { };
 
-        $router = $this->createMock(RouteCollector::class);
-        $router->expects($this->once())->method('addRoute')->with(['HEAD'], '/', new MiddlewareHandler([$middleware, $controller]));
+        $router = $this->createMock(RouteHandler::class);
+        $router->expects($this->once())->method('map')->with(['HEAD'], '/', $middleware, $controller);
 
         $ref = new \ReflectionProperty($app, 'router');
         $ref->setAccessible(true);
@@ -56,8 +55,8 @@ public function testPostMethodWithMiddlewareAddsPostRouteOnRouter()
         $middleware = function () {};
         $controller = function () { };
 
-        $router = $this->createMock(RouteCollector::class);
-        $router->expects($this->once())->method('addRoute')->with(['POST'], '/', new MiddlewareHandler([$middleware, $controller]));
+        $router = $this->createMock(RouteHandler::class);
+        $router->expects($this->once())->method('map')->with(['POST'], '/', $middleware, $controller);
 
         $ref = new \ReflectionProperty($app, 'router');
         $ref->setAccessible(true);
@@ -73,8 +72,8 @@ public function testPutMethodWithMiddlewareAddsPutRouteOnRouter()
         $middleware = function () {};
         $controller = function () { };
 
-        $router = $this->createMock(RouteCollector::class);
-        $router->expects($this->once())->method('addRoute')->with(['PUT'], '/', new MiddlewareHandler([$middleware, $controller]));
+        $router = $this->createMock(RouteHandler::class);
+        $router->expects($this->once())->method('map')->with(['PUT'], '/', $middleware, $controller);
 
         $ref = new \ReflectionProperty($app, 'router');
         $ref->setAccessible(true);
@@ -90,8 +89,8 @@ public function testPatchMethodWithMiddlewareAddsPatchRouteOnRouter()
         $middleware = function () {};
         $controller = function () { };
 
-        $router = $this->createMock(RouteCollector::class);
-        $router->expects($this->once())->method('addRoute')->with(['PATCH'], '/', new MiddlewareHandler([$middleware, $controller]));
+        $router = $this->createMock(RouteHandler::class);
+        $router->expects($this->once())->method('map')->with(['PATCH'], '/', $middleware, $controller);
 
         $ref = new \ReflectionProperty($app, 'router');
         $ref->setAccessible(true);
@@ -107,8 +106,8 @@ public function testDeleteMethodWithMiddlewareAddsDeleteRouteOnRouter()
         $middleware = function () {};
         $controller = function () { };
 
-        $router = $this->createMock(RouteCollector::class);
-        $router->expects($this->once())->method('addRoute')->with(['DELETE'], '/', new MiddlewareHandler([$middleware, $controller]));
+        $router = $this->createMock(RouteHandler::class);
+        $router->expects($this->once())->method('map')->with(['DELETE'], '/', $middleware, $controller);
 
         $ref = new \ReflectionProperty($app, 'router');
         $ref->setAccessible(true);
@@ -124,8 +123,8 @@ public function testOptionsMethodWithMiddlewareAddsOptionsRouteOnRouter()
         $middleware = function () {};
         $controller = function () { };
 
-        $router = $this->createMock(RouteCollector::class);
-        $router->expects($this->once())->method('addRoute')->with(['OPTIONS'], '/', new MiddlewareHandler([$middleware, $controller]));
+        $router = $this->createMock(RouteHandler::class);
+        $router->expects($this->once())->method('map')->with(['OPTIONS'], '/', $middleware, $controller);
 
         $ref = new \ReflectionProperty($app, 'router');
         $ref->setAccessible(true);
@@ -141,8 +140,8 @@ public function testAnyMethodWithMiddlewareAddsAllHttpMethodsOnRouter()
         $middleware = function () {};
         $controller = function () { };
 
-        $router = $this->createMock(RouteCollector::class);
-        $router->expects($this->once())->method('addRoute')->with(['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS'], '/', new MiddlewareHandler([$middleware, $controller]));
+        $router = $this->createMock(RouteHandler::class);
+        $router->expects($this->once())->method('map')->with(['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS'], '/', $middleware, $controller);
 
         $ref = new \ReflectionProperty($app, 'router');
         $ref->setAccessible(true);
@@ -158,8 +157,8 @@ public function testMapMethodWithMiddlewareAddsGivenMethodsOnRouter()
         $middleware = function () {};
         $controller = function () { };
 
-        $router = $this->createMock(RouteCollector::class);
-        $router->expects($this->once())->method('addRoute')->with(['GET', 'POST'], '/', new MiddlewareHandler([$middleware, $controller]));
+        $router = $this->createMock(RouteHandler::class);
+        $router->expects($this->once())->method('map')->with(['GET', 'POST'], '/', $middleware, $controller);
 
         $ref = new \ReflectionProperty($app, 'router');
         $ref->setAccessible(true);