Merge pull request #591 from flightphp/method-not-found

Added ability to throw a method not found instead of 404

flight/Engine.php

@@ -395,7 +395,6 @@ class Engine
         $params = $route->params;
 
         foreach ($middlewares as $middleware) {
-
             // Assume that nothing is going to be executed for the middleware.
             $middlewareObject = false;
 
@@ -579,9 +578,15 @@ class Engine
         if ($failedMiddlewareCheck === true) {
             $this->halt(403, 'Forbidden', empty(getenv('PHPUNIT_TEST')));
         } elseif ($dispatched === false) {
+            // 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();
             }
         }
+    }
 
     /**
      * Sends an HTTP 500 response for any errors.

flight/commands/RouteCommand.php

@@ -41,7 +41,7 @@ class RouteCommand extends AbstractBaseCommand
     {
         $io = $this->app()->io();
 
-		if(isset($this->config['index_root']) === false) {
+        if (isset($this->config['index_root']) === false) {
             $io->error('index_root not set in .runway-config.json', true);
             return;
         }
@@ -71,7 +71,7 @@ class RouteCommand extends AbstractBaseCommand
                     } catch (\TypeError $e) {
                         $middlewares[] = 'Bad Middleware';
                     } finally {
-						if(is_string($route->middleware) === true) {
+                        if (is_string($route->middleware) === true) {
                             $middlewares[] = $route->middleware;
                         }
                     }

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<int,mixed>
      */
-    protected array $group_middlewares = [];
+    protected array $groupMiddlewares = [];
 
     /**
      * Allowed HTTP methods
      *
      * @var array<int, string>
      */
-    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 @@ class Router
         //    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 @@ class Router
         }
 
         // 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 @@ class Router
     /**
      * 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<int, callable|object> $group_middlewares
+     * @param array<int, callable|object> $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 @@ class Router
     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 @@ class Router
         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 @@ class Router
      */
     public function reset(): void
     {
-        $this->index = 0;
+        $this->rewind();
     }
 }

tests/EngineTest.php

@@ -899,4 +899,47 @@ class EngineTest extends TestCase
 
         $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());
+	}
+
 }

tests/commands/ControllerCommandTest.php

@@ -11,7 +11,6 @@ use PHPUnit\Framework\TestCase;
 
 class ControllerCommandTest extends TestCase
 {
-
     protected static $in = __DIR__ . '/input.test';
     protected static $ou = __DIR__ . '/output.test';
 
@@ -31,12 +30,12 @@ class ControllerCommandTest extends TestCase
             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/');
         }
     }
 
@@ -59,8 +58,8 @@ class ControllerCommandTest extends TestCase
     public function testControllerAlreadyExists()
     {
         $app = $this->newApp('test', '0.0.1');
-		mkdir(__DIR__.'/controllers/');
-		file_put_contents(__DIR__.'/controllers/TestController.php', '<?php class TestController {}');
+        mkdir(__DIR__ . '/controllers/');
+        file_put_contents(__DIR__ . '/controllers/TestController.php', '<?php class TestController {}');
         $app->add(new ControllerCommand(['app_root' => 'tests/commands/']));
         $app->handle(['runway', 'make:controller', 'Test']);
 
@@ -73,7 +72,6 @@ class ControllerCommandTest extends TestCase
         $app->add(new ControllerCommand(['app_root' => 'tests/commands/']));
         $app->handle(['runway', 'make:controller', 'Test']);
 
-		$this->assertFileExists(__DIR__.'/controllers/TestController.php');
+        $this->assertFileExists(__DIR__ . '/controllers/TestController.php');
     }
-
 }

tests/commands/RouteCommandTest.php

@@ -13,7 +13,6 @@ use PHPUnit\Framework\TestCase;
 
 class RouteCommandTest extends TestCase
 {
-
     protected static $in = __DIR__ . '/input.test';
     protected static $ou = __DIR__ . '/output.test';
 
@@ -37,8 +36,8 @@ class RouteCommandTest extends TestCase
             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);
@@ -70,7 +69,7 @@ Flight::router()->case_sensitive = true;
 Flight::start();
 PHP;
 
-		file_put_contents(__DIR__.'/index.php', $index);
+        file_put_contents(__DIR__ . '/index.php', $index);
     }
 
     protected function removeColors(string $str): string
@@ -106,7 +105,8 @@ PHP;
 +---------+-----------+-------+----------+----------------+', $this->removeColors(file_get_contents(static::$ou)));
     }
 
-	public function testGetPostRoute() {
+    public function testGetPostRoute()
+    {
         $app = $this->newApp('test', '0.0.1');
         $this->createIndexFile();
         $app->add(new RouteCommand(['index_root' => 'tests/commands/index.php']));
@@ -119,5 +119,4 @@ PHP;
 | /post   | POST    |       | No       | Closure    |
 +---------+---------+-------+----------+------------+', $this->removeColors(file_get_contents(static::$ou)));
     }
-
 }