Skip to content

Refactor to move router logic to new RouteHandler and improve middleware execution #42

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
SimonFrings merged 2 commits into from
Sep 20, 2021
Merged

Refactor to move router logic to new RouteHandler and improve middleware execution #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b32983a
Refactor to move router logic to new `RouteHandler`
clue Sep 18, 2021
a568d96
Improve instantiation of middleware handler to happen only once
clue Sep 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 15 additions & 53 deletions src/App.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand Down
73 changes: 73 additions & 0 deletions src/RouteHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
}
39 changes: 19 additions & 20 deletions tests/AppMiddlewareTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down
Loading