From 27718cfea72da8d249f98d55ec6395cdc39636b7 Mon Sep 17 00:00:00 2001 From: Austin Collier Date: Thu, 16 May 2024 23:03:36 -0600 Subject: [PATCH] added ability to throw a method not found instead of 404 --- flight/Engine.php | 49 ++++++++------ flight/commands/RouteCommand.php | 18 ++--- flight/net/Router.php | 53 +++++++++------ tests/EngineTest.php | 43 ++++++++++++ tests/commands/ControllerCommandTest.php | 66 +++++++++--------- tests/commands/RouteCommandTest.php | 85 ++++++++++++------------ 6 files changed, 186 insertions(+), 128 deletions(-) diff --git a/flight/Engine.php b/flight/Engine.php index 47e84f77..2559810c 100644 --- a/flight/Engine.php +++ b/flight/Engine.php @@ -388,15 +388,14 @@ protected function processMiddleware(Route $route, string $eventName): bool { $atLeastOneMiddlewareFailed = false; - // Process things normally for before, and then in reverse order for after. - $middlewares = $eventName === Dispatcher::FILTER_BEFORE - ? $route->middleware - : array_reverse($route->middleware); + // Process things normally for before, and then in reverse order for after. + $middlewares = $eventName === Dispatcher::FILTER_BEFORE + ? $route->middleware + : array_reverse($route->middleware); $params = $route->params; foreach ($middlewares as $middleware) { - - // Assume that nothing is going to be executed for the middleware. + // Assume that nothing is going to be executed for the middleware. $middlewareObject = false; // Closure functions can only run on the before event @@ -426,12 +425,12 @@ protected function processMiddleware(Route $route, string $eventName): bool } } - // If nothing was resolved, go to the next thing + // If nothing was resolved, go to the next thing if ($middlewareObject === false) { continue; } - // This is the way that v3 handles output buffering (which captures output correctly) + // This is the way that v3 handles output buffering (which captures output correctly) $useV3OutputBuffering = $this->response()->v2_output_buffering === false && $route->is_streamed === false; @@ -441,16 +440,16 @@ protected function processMiddleware(Route $route, string $eventName): bool } // Here is the array callable $middlewareObject that we created earlier. - // It looks bizarre but it's really calling [ $class, $method ]($params) - // Which loosely translates to $class->$method($params) + // It looks bizarre but it's really calling [ $class, $method ]($params) + // Which loosely translates to $class->$method($params) $middlewareResult = $middlewareObject($params); if ($useV3OutputBuffering === true) { $this->response()->write(ob_get_clean()); } - // If you return false in your middleware, it will halt the request - // and throw a 403 forbidden error by default. + // If you return false in your middleware, it will halt the request + // and throw a 403 forbidden error by default. if ($middlewareResult === false) { $atLeastOneMiddlewareFailed = true; break; @@ -579,7 +578,13 @@ public function _start(): void if ($failedMiddlewareCheck === true) { $this->halt(403, 'Forbidden', empty(getenv('PHPUNIT_TEST'))); } elseif ($dispatched === false) { - $this->notFound(); + // Get the previous route and check if the method failed, but the URL was good. + $lastRouteExecuted = $router->executedRoute; + if ($lastRouteExecuted !== null && $lastRouteExecuted->matchUrl($request->url) === true && $lastRouteExecuted->matchMethod($request->method) === false) { + $this->halt(405, 'Method Not Allowed', empty(getenv('PHPUNIT_TEST'))); + } else { + $this->notFound(); + } } } @@ -670,8 +675,8 @@ public function _group(string $pattern, callable $callback, array $group_middlew * @param string $pattern URL pattern to match * @param callable|string $callback Callback function or string class->method * @param bool $pass_route Pass the matching route object to the callback - * - * @return Route + * + * @return Route */ public function _post(string $pattern, $callback, bool $pass_route = false, string $route_alias = ''): Route { @@ -684,8 +689,8 @@ public function _post(string $pattern, $callback, bool $pass_route = false, stri * @param string $pattern URL pattern to match * @param callable|string $callback Callback function or string class->method * @param bool $pass_route Pass the matching route object to the callback - * - * @return Route + * + * @return Route */ public function _put(string $pattern, $callback, bool $pass_route = false, string $route_alias = ''): Route { @@ -698,12 +703,12 @@ public function _put(string $pattern, $callback, bool $pass_route = false, strin * @param string $pattern URL pattern to match * @param callable|string $callback Callback function or string class->method * @param bool $pass_route Pass the matching route object to the callback - * - * @return Route + * + * @return Route */ public function _patch(string $pattern, $callback, bool $pass_route = false, string $route_alias = ''): Route { - return $this->router()->map('PATCH ' . $pattern, $callback, $pass_route, $route_alias); + return $this->router()->map('PATCH ' . $pattern, $callback, $pass_route, $route_alias); } /** @@ -712,8 +717,8 @@ public function _patch(string $pattern, $callback, bool $pass_route = false, str * @param string $pattern URL pattern to match * @param callable|string $callback Callback function or string class->method * @param bool $pass_route Pass the matching route object to the callback - * - * @return Route + * + * @return Route */ public function _delete(string $pattern, $callback, bool $pass_route = false, string $route_alias = ''): Route { diff --git a/flight/commands/RouteCommand.php b/flight/commands/RouteCommand.php index 6ba661af..a34b821b 100644 --- a/flight/commands/RouteCommand.php +++ b/flight/commands/RouteCommand.php @@ -41,10 +41,10 @@ public function execute() { $io = $this->app()->io(); - if(isset($this->config['index_root']) === false) { - $io->error('index_root not set in .runway-config.json', true); - return; - } + if (isset($this->config['index_root']) === false) { + $io->error('index_root not set in .runway-config.json', true); + return; + } $io->bold('Routes', true); @@ -69,12 +69,12 @@ public function execute() return preg_match("/^class@anonymous/", end($middleware_class_name)) ? 'Anonymous' : end($middleware_class_name); }, $route->middleware); } catch (\TypeError $e) { - $middlewares[] = 'Bad Middleware'; + $middlewares[] = 'Bad Middleware'; } finally { - if(is_string($route->middleware) === true) { - $middlewares[] = $route->middleware; - } - } + if (is_string($route->middleware) === true) { + $middlewares[] = $route->middleware; + } + } } $arrayOfRoutes[] = [ diff --git a/flight/net/Router.php b/flight/net/Router.php index 88df038e..a43b5baf 100644 --- a/flight/net/Router.php +++ b/flight/net/Router.php @@ -32,7 +32,7 @@ class Router /** * The current route that is has been found and executed. */ - protected ?Route $executedRoute = null; + public ?Route $executedRoute = null; /** * Pointer to current route. @@ -42,21 +42,21 @@ class Router /** * When groups are used, this is mapped against all the routes */ - protected string $group_prefix = ''; + protected string $groupPrefix = ''; /** * Group Middleware * * @var array */ - protected array $group_middlewares = []; + protected array $groupMiddlewares = []; /** * Allowed HTTP methods * * @var array */ - protected array $allowed_methods = ['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'HEAD', 'OPTIONS']; + protected array $allowedMethods = ['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'HEAD', 'OPTIONS']; /** * Gets mapped routes. @@ -93,7 +93,7 @@ public function map(string $pattern, $callback, bool $pass_route = false, string // Flight::route('', function() {}); // } // Keep the space so that it can execute the below code normally - if ($this->group_prefix !== '') { + if ($this->groupPrefix !== '') { $url = ltrim($pattern); } else { $url = trim($pattern); @@ -113,14 +113,14 @@ public function map(string $pattern, $callback, bool $pass_route = false, string } // And this finishes it off. - if ($this->group_prefix !== '') { - $url = rtrim($this->group_prefix . $url); + if ($this->groupPrefix !== '') { + $url = rtrim($this->groupPrefix . $url); } $route = new Route($url, $callback, $methods, $pass_route, $route_alias); // to handle group middleware - foreach ($this->group_middlewares as $gm) { + foreach ($this->groupMiddlewares as $gm) { $route->addMiddleware($gm); } @@ -197,20 +197,20 @@ public function delete(string $pattern, $callback, bool $pass_route = false, str /** * Group together a set of routes * - * @param string $group_prefix group URL prefix (such as /api/v1) + * @param string $groupPrefix group URL prefix (such as /api/v1) * @param callable $callback The necessary calling that holds the Router class - * @param array $group_middlewares + * @param array $groupMiddlewares * The middlewares to be applied to the group. Example: `[$middleware1, $middleware2]` */ - public function group(string $group_prefix, callable $callback, array $group_middlewares = []): void + public function group(string $groupPrefix, callable $callback, array $groupMiddlewares = []): void { - $old_group_prefix = $this->group_prefix; - $old_group_middlewares = $this->group_middlewares; - $this->group_prefix .= $group_prefix; - $this->group_middlewares = array_merge($this->group_middlewares, $group_middlewares); + $oldGroupPrefix = $this->groupPrefix; + $oldGroupMiddlewares = $this->groupMiddlewares; + $this->groupPrefix .= $groupPrefix; + $this->groupMiddlewares = array_merge($this->groupMiddlewares, $groupMiddlewares); $callback($this); - $this->group_prefix = $old_group_prefix; - $this->group_middlewares = $old_group_middlewares; + $this->groupPrefix = $oldGroupPrefix; + $this->groupMiddlewares = $oldGroupMiddlewares; } /** @@ -221,9 +221,14 @@ public function group(string $group_prefix, callable $callback, array $group_mid public function route(Request $request) { while ($route = $this->current()) { - if ($route->matchMethod($request->method) && $route->matchUrl($request->url, $this->case_sensitive)) { + $urlMatches = $route->matchUrl($request->url, $this->case_sensitive); + $methodMatches = $route->matchMethod($request->method); + if ($urlMatches === true && $methodMatches === true) { $this->executedRoute = $route; return $route; + // capture the route but don't execute it. We'll use this in Engine->start() to throw a 405 + } elseif ($urlMatches === true && $methodMatches === false) { + $this->executedRoute = $route; } $this->next(); } @@ -299,12 +304,20 @@ public function current() return $this->routes[$this->index] ?? false; } + /** + * Gets the previous route. + */ + public function previous(): void + { + --$this->index; + } + /** * Gets the next route. */ public function next(): void { - $this->index++; + ++$this->index; } /** @@ -312,6 +325,6 @@ public function next(): void */ public function reset(): void { - $this->index = 0; + $this->rewind(); } } diff --git a/tests/EngineTest.php b/tests/EngineTest.php index ee6ad762..3c24c882 100644 --- a/tests/EngineTest.php +++ b/tests/EngineTest.php @@ -899,4 +899,47 @@ public function testContainerPsr11MethodNotFound() { $engine->start(); } + + public function testRouteFoundButBadMethod() { + $engine = new class extends Engine { + public function getLoader() + { + return $this->loader; + } + }; + // doing this so we can overwrite some parts of the response + $engine->getLoader()->register('response', function () { + return new class extends Response { + public function setRealHeader( + string $header_string, + bool $replace = true, + int $response_code = 0 + ): self { + return $this; + } + }; + }); + + $engine->route('POST /path1/@id', function ($id) { + echo 'OK' . $id; + }); + + $engine->route('GET /path2/@id', function ($id) { + echo 'OK' . $id; + }); + + $engine->route('PATCH /path3/@id', function ($id) { + echo 'OK' . $id; + }); + + $engine->request()->url = '/path1/123'; + $engine->request()->method = 'GET'; + + $engine->start(); + + $this->expectOutputString('Method Not Allowed'); + $this->assertEquals(405, $engine->response()->status()); + $this->assertEquals('Method Not Allowed', $engine->response()->getBody()); + } + } diff --git a/tests/commands/ControllerCommandTest.php b/tests/commands/ControllerCommandTest.php index cee5c837..82fb0c1c 100644 --- a/tests/commands/ControllerCommandTest.php +++ b/tests/commands/ControllerCommandTest.php @@ -11,8 +11,7 @@ class ControllerCommandTest extends TestCase { - - protected static $in = __DIR__ . '/input.test'; + protected static $in = __DIR__ . '/input.test'; protected static $ou = __DIR__ . '/output.test'; public function setUp(): void @@ -31,49 +30,48 @@ public function tearDown(): void unlink(static::$ou); } - if (file_exists(__DIR__.'/controllers/TestController.php')) { - unlink(__DIR__.'/controllers/TestController.php'); - } + if (file_exists(__DIR__ . '/controllers/TestController.php')) { + unlink(__DIR__ . '/controllers/TestController.php'); + } - if (file_exists(__DIR__.'/controllers/')) { - rmdir(__DIR__.'/controllers/'); - } - } + if (file_exists(__DIR__ . '/controllers/')) { + rmdir(__DIR__ . '/controllers/'); + } + } - protected function newApp(string $name, string $version = '') + protected function newApp(string $name, string $version = '') { $app = new Application($name, $version ?: '0.0.1', fn () => false); return $app->io(new Interactor(static::$in, static::$ou)); } - public function testConfigAppRootNotSet() - { - $app = $this->newApp('test', '0.0.1'); - $app->add(new ControllerCommand([])); - $app->handle(['runway', 'make:controller', 'Test']); - - $this->assertStringContainsString('app_root not set in .runway-config.json', file_get_contents(static::$ou)); - } + public function testConfigAppRootNotSet() + { + $app = $this->newApp('test', '0.0.1'); + $app->add(new ControllerCommand([])); + $app->handle(['runway', 'make:controller', 'Test']); - public function testControllerAlreadyExists() - { - $app = $this->newApp('test', '0.0.1'); - mkdir(__DIR__.'/controllers/'); - file_put_contents(__DIR__.'/controllers/TestController.php', 'add(new ControllerCommand(['app_root' => 'tests/commands/'])); - $app->handle(['runway', 'make:controller', 'Test']); + $this->assertStringContainsString('app_root not set in .runway-config.json', file_get_contents(static::$ou)); + } - $this->assertStringContainsString('TestController already exists.', file_get_contents(static::$ou)); - } + public function testControllerAlreadyExists() + { + $app = $this->newApp('test', '0.0.1'); + mkdir(__DIR__ . '/controllers/'); + file_put_contents(__DIR__ . '/controllers/TestController.php', 'add(new ControllerCommand(['app_root' => 'tests/commands/'])); + $app->handle(['runway', 'make:controller', 'Test']); - public function testCreateController() - { - $app = $this->newApp('test', '0.0.1'); - $app->add(new ControllerCommand(['app_root' => 'tests/commands/'])); - $app->handle(['runway', 'make:controller', 'Test']); + $this->assertStringContainsString('TestController already exists.', file_get_contents(static::$ou)); + } - $this->assertFileExists(__DIR__.'/controllers/TestController.php'); - } + public function testCreateController() + { + $app = $this->newApp('test', '0.0.1'); + $app->add(new ControllerCommand(['app_root' => 'tests/commands/'])); + $app->handle(['runway', 'make:controller', 'Test']); + $this->assertFileExists(__DIR__ . '/controllers/TestController.php'); + } } diff --git a/tests/commands/RouteCommandTest.php b/tests/commands/RouteCommandTest.php index b4597340..eae0b811 100644 --- a/tests/commands/RouteCommandTest.php +++ b/tests/commands/RouteCommandTest.php @@ -13,15 +13,14 @@ class RouteCommandTest extends TestCase { - - protected static $in = __DIR__ . '/input.test'; + protected static $in = __DIR__ . '/input.test'; protected static $ou = __DIR__ . '/output.test'; public function setUp(): void { file_put_contents(static::$in, '', LOCK_EX); file_put_contents(static::$ou, '', LOCK_EX); - $_SERVER = []; + $_SERVER = []; $_REQUEST = []; Flight::init(); Flight::setEngine(new Engine()); @@ -37,25 +36,25 @@ public function tearDown(): void unlink(static::$ou); } - if (file_exists(__DIR__.'/index.php')) { - unlink(__DIR__.'/index.php'); - } + if (file_exists(__DIR__ . '/index.php')) { + unlink(__DIR__ . '/index.php'); + } - unset($_REQUEST); + unset($_REQUEST); unset($_SERVER); Flight::clear(); - } + } - protected function newApp(string $name, string $version = '') + protected function newApp(string $name, string $version = '') { $app = new Application($name, $version ?: '0.0.1', fn () => false); return $app->io(new Interactor(static::$in, static::$ou)); } - protected function createIndexFile() - { - $index = <<newApp('test', '0.0.1'); - $app->add(new RouteCommand([])); - $app->handle(['runway', 'routes']); + public function testConfigIndexRootNotSet() + { + $app = $this->newApp('test', '0.0.1'); + $app->add(new RouteCommand([])); + $app->handle(['runway', 'routes']); - $this->assertStringContainsString('index_root not set in .runway-config.json', file_get_contents(static::$ou)); - } + $this->assertStringContainsString('index_root not set in .runway-config.json', file_get_contents(static::$ou)); + } - public function testGetRoutes() - { - $app = $this->newApp('test', '0.0.1'); - $this->createIndexFile(); - $app->add(new RouteCommand(['index_root' => 'tests/commands/index.php'])); - $app->handle(['runway', 'routes']); + public function testGetRoutes() + { + $app = $this->newApp('test', '0.0.1'); + $this->createIndexFile(); + $app->add(new RouteCommand(['index_root' => 'tests/commands/index.php'])); + $app->handle(['runway', 'routes']); - $this->assertStringContainsString('Routes', file_get_contents(static::$ou)); - $this->assertStringContainsString('+---------+-----------+-------+----------+----------------+ + $this->assertStringContainsString('Routes', file_get_contents(static::$ou)); + $this->assertStringContainsString('+---------+-----------+-------+----------+----------------+ | Pattern | Methods | Alias | Streamed | Middleware | +---------+-----------+-------+----------+----------------+ | / | GET, HEAD | | No | - | @@ -104,20 +103,20 @@ public function testGetRoutes() | /put | PUT | | No | - | | /patch | PATCH | | No | Bad Middleware | +---------+-----------+-------+----------+----------------+', $this->removeColors(file_get_contents(static::$ou))); - } + } - public function testGetPostRoute() { - $app = $this->newApp('test', '0.0.1'); - $this->createIndexFile(); - $app->add(new RouteCommand(['index_root' => 'tests/commands/index.php'])); - $app->handle(['runway', 'routes', '--post']); + public function testGetPostRoute() + { + $app = $this->newApp('test', '0.0.1'); + $this->createIndexFile(); + $app->add(new RouteCommand(['index_root' => 'tests/commands/index.php'])); + $app->handle(['runway', 'routes', '--post']); - $this->assertStringContainsString('Routes', file_get_contents(static::$ou)); - $this->assertStringContainsString('+---------+---------+-------+----------+------------+ + $this->assertStringContainsString('Routes', file_get_contents(static::$ou)); + $this->assertStringContainsString('+---------+---------+-------+----------+------------+ | Pattern | Methods | Alias | Streamed | Middleware | +---------+---------+-------+----------+------------+ | /post | POST | | No | Closure | +---------+---------+-------+----------+------------+', $this->removeColors(file_get_contents(static::$ou))); - } - + } }