Add new matching logic and add tests

src/Routing/RouteBuilder.php

@@ -930,7 +930,7 @@ class RouteBuilder
      * Apply a set of middleware to a group
      *
      * @param string $name Name of the middleware group
-     * @param array $names Names of the middleware
+     * @param array $middlewareNames Names of the middleware
      * @return $this
      */
     public function middlewareGroup($name, array $middlewareNames)

src/Routing/RouteCollection.php

@@ -435,7 +435,7 @@ class RouteCollection
      * Add middleware to a middleware group
      *
      * @param string $name Name of the middleware group
-     * @param array $names Names of the middleware
+     * @param array $middlewareNames Names of the middleware
      * @return $this
      */
     public function middlewareGroup($name, $middlewareNames)
@@ -494,14 +494,8 @@ class RouteCollection
     {
         foreach ($middleware as $name) {
             if (!$this->hasMiddleware($name) && !$this->hasMiddlewareGroup($name)) {
-                if (!$this->hasMiddleware($name)) {
-                    $message = "Cannot apply '$name' middleware to path '$path'. It has not been registered.";
-                    throw new RuntimeException($message);
-                }
-                if (!$this->hasMiddlewareGroup($name)) {
-                    $message = "Cannot apply '$name' middleware group to path '$path'. It has not been added.";
-                    throw new RuntimeException($message);
-                }
+                $message = "Cannot apply '$name' middleware or middleware group to path '$path'. It has not been registered.";
+                throw new RuntimeException($message);
             }
         }
         // Matches route element pattern in Cake\Routing\Route
@@ -519,30 +513,37 @@ class RouteCollection
     /**
      * Get an array of middleware that matches the provided URL.
      *
-     * All middleware lists that match the URL will be merged together from shortest
-     * path to longest path. If a middleware would be added to the set more than
-     * once because it is connected to multiple path substrings match, it will only
-     * be added once at its first occurrence.
+     * Only the middlewrae applied to the longest matching scope will be used.
      *
      * @param string $needle The URL path to find middleware for.
      * @return array
      */
     public function getMatchingMiddleware($needle)
     {
-        $matching = [];
-        foreach ($this->_middlewarePaths as $pattern => $middleware) {
+        $paths = $this->_middlewarePaths;
+        $keys = array_map('strlen', array_keys($paths));
+        array_multisort($keys, SORT_DESC, $paths);
+
+        foreach ($paths as $pattern => &$middleware) {
+            $matching = [];
+
+            foreach ($middleware as $key => $name) {
+                if ($this->hasMiddlewareGroup($name)) {
+                    unset($middleware[$key]);
+                    $middleware = array_merge($middleware, $this->_middlewareGroups[$name]);
+                }
+            }
+
             if (preg_match($pattern, $needle)) {
                 $matching = array_merge($matching, $middleware);
-            }
-        }
+                foreach ($matching as $name) {
+                    $resolved[] = $this->_middleware[$name];
+                }
 
-        $resolved = [];
-        foreach ($matching as $name) {
-            if (!isset($resolved[$name])) {
-                $resolved[$name] = $this->_middleware[$name];
+                return array_values($resolved);
             }
         }
 
-        return array_values($resolved);
+        return [];
     }
 }

tests/TestCase/Routing/Middleware/RoutingMiddlewareTest.php

@@ -251,7 +251,51 @@ class RoutingMiddlewareTest extends TestCase
     public function testInvokeScopedMiddlewareReturnResponse()
     {
         $this->counter = 0;
-        Router::scope('/api', function ($routes) {
+        Router::scope('/', function ($routes) {
+            $routes->registerMiddleware('first', function ($req, $res, $next) {
+                $this->log[] = 'first';
+
+                return $next($req, $res);
+            });
+            $routes->registerMiddleware('second', function ($req, $res, $next) {
+                $this->log[] = 'second';
+
+                return $res;
+            });
+
+            $routes->applyMiddleware('first');
+            $routes->connect('/', ['controller' => 'Home']);
+
+            $routes->scope('/api', function ($routes) {
+                $routes->applyMiddleware('second');
+                $routes->connect('/articles', ['controller' => 'Articles']);
+            });
+        });
+
+        $request = ServerRequestFactory::fromGlobals([
+            'REQUEST_METHOD' => 'GET',
+            'REQUEST_URI' => '/api/articles'
+        ]);
+        $response = new Response();
+        $next = function ($req, $res) {
+            $this->fail('Should not be invoked as first should be ignored.');
+        };
+        $middleware = new RoutingMiddleware();
+        $result = $middleware($request, $response, $next);
+
+        $this->assertSame($response, $result, 'Should return result');
+        $this->assertSame(['second'], $this->log);
+    }
+
+    /**
+     * Test control flow in scoped middleware.
+     *
+     * @return void
+     */
+    public function testInvokeScopedMiddlewareReturnResponseMainScope()
+    {
+        $this->counter = 0;
+        Router::scope('/', function ($routes) {
             $routes->registerMiddleware('first', function ($req, $res, $next) {
                 $this->log[] = 'first';
 
@@ -263,27 +307,27 @@ class RoutingMiddlewareTest extends TestCase
                 return $next($req, $res);
             });
 
-            $routes->applyMiddleware('second');
-            $routes->connect('/ping', ['controller' => 'Pings']);
+            $routes->applyMiddleware('first');
+            $routes->connect('/', ['controller' => 'Home']);
 
-            $routes->scope('/v1', function ($routes) {
-                $routes->applyMiddleware('first');
+            $routes->scope('/api', function ($routes) {
+                $routes->applyMiddleware('second');
                 $routes->connect('/articles', ['controller' => 'Articles']);
             });
         });
 
         $request = ServerRequestFactory::fromGlobals([
             'REQUEST_METHOD' => 'GET',
-            'REQUEST_URI' => '/api/v1/articles'
+            'REQUEST_URI' => '/'
         ]);
         $response = new Response();
         $next = function ($req, $res) {
-            $this->fail('Should not be invoked as second returns a response');
+            $this->fail('Should not be invoked as second should be ignored.');
         };
         $middleware = new RoutingMiddleware();
         $result = $middleware($request, $response, $next);
 
         $this->assertSame($response, $result, 'Should return result');
-        $this->assertSame(['second', 'first'], $this->log);
+        $this->assertSame(['first'], $this->log);
     }
 }

tests/TestCase/Routing/RouteBuilderTest.php

@@ -842,7 +842,6 @@ class RouteBuilderTest extends TestCase
         $routes->registerMiddleware('bad', 'strlen');
     }
 
-
     /**
      * Test middleware group
      *
@@ -857,7 +856,6 @@ class RouteBuilderTest extends TestCase
         $routes->registerMiddleware('test_two', $func);
         $result = $routes->middlewareGroup('group', ['test', 'test_two']);
 
-
         $this->assertSame($result, $routes);
         $this->assertTrue($this->collection->hasMiddlewareGroup('group'));
     }
@@ -882,7 +880,7 @@ class RouteBuilderTest extends TestCase
      * Test applying middleware to a scope when it doesn't exist
      *
      * @expectedException \RuntimeException
-     * @expectedExceptionMessage Cannot apply 'bad' middleware to path '/api'. It has not been registered.
+     * @expectedExceptionMessage Cannot apply 'bad' middleware or middleware group to path '/api'. It has not been registered.
      * @return void
      */
     public function testApplyMiddlewareInvalidName()

tests/TestCase/Routing/RouteCollectionTest.php

@@ -671,7 +671,8 @@ class RouteCollectionTest extends TestCase
      */
     public function testMiddlewareGroup()
     {
-        $this->collection->registerMiddleware('closure', function () {});
+        $this->collection->registerMiddleware('closure', function () {
+        });
 
         $mock = $this->getMockBuilder('\stdClass')
             ->setMethods(['__invoke'])
@@ -679,12 +680,46 @@ class RouteCollectionTest extends TestCase
         $result = $this->collection->registerMiddleware('callable', $mock);
         $this->collection->registerMiddleware('callable', $mock);
 
-        $routes->groupMiddleware('group', ['closure', 'callable']);
+        $this->collection->middlewareGroup('group', ['closure', 'callable']);
 
         $this->assertTrue($this->collection->hasMiddlewareGroup('group'));
     }
 
     /**
+     * Test adding a middleware group with the same name twice fails.
+     *
+     * @expectedException \RuntimeException
+     * @expectedExceptionMessage Cannot add middle ware group 'group' . A middleware group by this name has already been added.
+     * @return void
+     */
+    public function testMiddlewareGroupDuplicate()
+    {
+        $this->collection->registerMiddleware('closure', function () {
+        });
+
+        $mock = $this->getMockBuilder('\stdClass')
+            ->setMethods(['__invoke'])
+            ->getMock();
+        $result = $this->collection->registerMiddleware('callable', $mock);
+        $this->collection->registerMiddleware('callable', $mock);
+
+        $this->collection->middlewareGroup('group', ['closure', 'callable']);
+        $this->collection->middlewareGroup('group', ['closure', 'callable']);
+    }
+
+    /**
+     * Test adding ab unregistered middleware to a middleware group fails.
+     *
+     * @expectedException \RuntimeException
+     * @expectedExceptionMessage Cannot add 'bad' middleware to group 'group'. It has not been registered.
+     * @return void
+     */
+    public function testMiddlewareGroupUnregisteredMiddleware()
+    {
+        $this->collection->middlewareGroup('group', ['bad']);
+    }
+
+    /**
      * Test adding middleware with a placeholder in the path.
      *
      * @return void
@@ -743,8 +778,8 @@ class RouteCollectionTest extends TestCase
         $this->assertCount(0, $middleware);
 
         $middleware = $this->collection->getMatchingMiddleware('/api/v1/articles/1');
-        $this->assertCount(2, $middleware);
-        $this->assertEquals([$mock, $mockTwo], $middleware, 'Both middleware match');
+        $this->assertCount(1, $middleware);
+        $this->assertEquals([$mock], $middleware, 'Should only match the strongest scope');
 
         $middleware = $this->collection->getMatchingMiddleware('/api/v1/comments');
         $this->assertCount(1, $middleware);
@@ -806,7 +841,7 @@ class RouteCollectionTest extends TestCase
      * Test applying middleware to a scope when it doesn't exist
      *
      * @expectedException \RuntimeException
-     * @expectedExceptionMessage Cannot apply 'bad' middleware to path '/api'. It has not been registered.
+     * @expectedExceptionMessage Cannot apply 'bad' middleware or middleware group to path '/api'. It has not been registered.
      * @return void
      */
     public function testApplyMiddlewareUnregistered()