diff --git a/core/lib/Drupal/Core/Routing/RouteCompiler.php b/core/lib/Drupal/Core/Routing/RouteCompiler.php index 1d223ff8d6cc2d18221f1e3bbb901bc5626fcb16..639feff442257aa47e2d52ae2bde51451e39218d 100644 --- a/core/lib/Drupal/Core/Routing/RouteCompiler.php +++ b/core/lib/Drupal/Core/Routing/RouteCompiler.php @@ -43,7 +43,9 @@ public static function compile(Route $route) { $stripped_path = static::getPathWithoutDefaults($route); $fit = static::getFit($stripped_path); $pattern_outline = static::getPatternOutline($stripped_path); - $num_parts = count(explode('/', trim($pattern_outline, '/'))); + // We count the number of parts including any optional trailing parts. This + // allows the RouteProvider to filter candidate routes more efficiently. + $num_parts = count(explode('/', trim($route->getPath(), '/'))); return new CompiledRoute( $fit, diff --git a/core/lib/Drupal/Core/Routing/RouteProvider.php b/core/lib/Drupal/Core/Routing/RouteProvider.php index 278b486377ec08c1e6aeeadd7c09fd63c047045b..20e90228369fbc46cf9a0b5da23c06e983f96e96 100644 --- a/core/lib/Drupal/Core/Routing/RouteProvider.php +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php @@ -298,11 +298,9 @@ public function getRoutesByPattern($pattern) { * Returns a route collection of matching routes. */ protected function getRoutesByPath($path) { - // Filter out each empty value, though allow '0' and 0, which would be - // filtered out by empty(). - $parts = array_values(array_filter(explode('/', $path), function($value) { - return $value !== NULL && $value !== ''; - })); + // Split the path up on the slashes, ignoring multiple slashes in a row + // or leading or trailing slashes. + $parts = preg_split('@/+@', $path, NULL, PREG_SPLIT_NO_EMPTY); $collection = new RouteCollection(); @@ -311,21 +309,36 @@ protected function getRoutesByPath($path) { return $collection; } - $routes = $this->connection->query("SELECT name, route FROM {" . $this->connection->escapeTable($this->tableName) . "} WHERE pattern_outline IN ( :patterns[] ) ORDER BY fit DESC, name ASC", array( - ':patterns[]' => $ancestors, + // The >= check on number_parts allows us to match routes with optional + // trailing wildcard parts as long as the pattern matches, since we + // dump the route pattern without those optional parts. + $routes = $this->connection->query("SELECT name, route, fit FROM {" . $this->connection->escapeTable($this->tableName) . "} WHERE pattern_outline IN ( :patterns[] ) AND number_parts >= :count_parts", array( + ':patterns[]' => $ancestors, ':count_parts' => count($parts), )) - ->fetchAllKeyed(); + ->fetchAll(\PDO::FETCH_ASSOC); - foreach ($routes as $name => $route) { - $route = unserialize($route); - if (preg_match($route->compile()->getRegex(), $path, $matches)) { - $collection->add($name, $route); - } + // We sort by fit and name in PHP to avoid a SQL filesort. + usort($routes, array($this, 'routeProviderRouteCompare')); + + foreach ($routes as $row) { + $collection->add($row['name'], unserialize($row['route'])); } return $collection; } + /** + * Comparison function for usort on routes. + */ + public function routeProviderRouteCompare(array $a, array $b) { + if ($a['fit'] == $b['fit']) { + return strcmp($a['name'], $b['name']); + } + // Reverse sort from highest to lowest fit. PHP should cast to int, but + // the explicit cast makes this sort more robust against unexpected input. + return (int) $a['fit'] < (int) $b['fit'] ? 1 : -1; + } + /** * {@inheritdoc} */ diff --git a/core/modules/system/src/Tests/Routing/RouteProviderTest.php b/core/modules/system/src/Tests/Routing/RouteProviderTest.php index 74cbaa30e915e6b7031b849a43e72c59254dee75..8be53ff261b6c209819a834f85f30b9dc15fa415 100644 --- a/core/modules/system/src/Tests/Routing/RouteProviderTest.php +++ b/core/modules/system/src/Tests/Routing/RouteProviderTest.php @@ -316,7 +316,46 @@ function testOutlinePathMatchDefaultsCollision2() { $this->assertEqual(array('narf', 'poink'), array_keys($routes_array), 'Ensure the fitness was taken into account.'); $this->assertNotNull($routes->get('narf'), 'The first matching route was found.'); $this->assertNotNull($routes->get('poink'), 'The second matching route was found.'); - $this->assertNull($routes->get('eep'), 'Noin-matching route was not found.'); + $this->assertNull($routes->get('eep'), 'Non-matching route was not found.'); + } + catch (ResourceNotFoundException $e) { + $this->fail('No matching route found with default argument value.'); + } + } + + /** + * Confirms that we can find multiple routes that match the request equally. + */ + function testOutlinePathMatchDefaultsCollision3() { + $connection = Database::getConnection(); + $provider = new RouteProvider($connection, $this->state, $this->currentPath, $this->cache, $this->pathProcessor, 'test_routes'); + + $this->fixtures->createTables($connection); + + $collection = new RouteCollection(); + $collection->add('poink', new Route('/some/{value}/path')); + // Add a second route matching the same path pattern. + $collection->add('poink2', new Route('/some/{object}/path')); + $collection->add('narf', new Route('/some/here/path')); + $collection->add('eep', new Route('/something/completely/different')); + + $dumper = new MatcherDumper($connection, $this->state, 'test_routes'); + $dumper->addRoutes($collection); + $dumper->dump(); + + $path = '/some/over-there/path'; + + $request = Request::create($path, 'GET'); + + try { + $routes = $provider->getRouteCollectionForRequest($request); + $routes_array = $routes->all(); + + $this->assertEqual(count($routes), 2, 'The correct number of routes was found.'); + $this->assertEqual(array('poink', 'poink2'), array_keys($routes_array), 'Ensure the fitness and name were taken into account in the sort.'); + $this->assertNotNull($routes->get('poink'), 'The first matching route was found.'); + $this->assertNotNull($routes->get('poink2'), 'The second matching route was found.'); + $this->assertNull($routes->get('eep'), 'Non-matching route was not found.'); } catch (ResourceNotFoundException $e) { $this->fail('No matching route found with default argument value.'); diff --git a/core/modules/system/src/Tests/Routing/RouterTest.php b/core/modules/system/src/Tests/Routing/RouterTest.php index b945676744f54b31485e36dfd6e13eda9c86c8d2..16f423441d33cefed27598a99d719db590250c3c 100644 --- a/core/modules/system/src/Tests/Routing/RouterTest.php +++ b/core/modules/system/src/Tests/Routing/RouterTest.php @@ -197,6 +197,15 @@ public function testRouterMatching() { $this->drupalGet('router_test/test14/2'); $this->assertResponse(200); $this->assertText('Route not matched.'); + + // Check that very long paths don't cause an error. + $path = 'router_test/test1'; + $suffix = '/d/r/u/p/a/l'; + for ($i = 0; $i < 10; $i++) { + $path .= $suffix; + $this->drupalGet($path); + $this->assertResponse(404); + } } /** diff --git a/core/modules/system/system.install b/core/modules/system/system.install index a0849b11de23d9a91e581bbb54ced3894e6d1a1d..d2c1b40b4c51ae5faf7b0c5759f51ea28ae3b66d 100644 --- a/core/modules/system/system.install +++ b/core/modules/system/system.install @@ -946,7 +946,7 @@ function system_schema() { ), ), 'indexes' => array( - 'pattern_outline_fit' => array('pattern_outline', 'fit'), + 'pattern_outline_parts' => array('pattern_outline', 'number_parts'), ), 'primary key' => array('name'), ); @@ -1085,3 +1085,11 @@ function system_schema() { return $schema; } + +/** + * Change the index on the {router} table. + */ +function system_update_8001() { + db_drop_index('router', 'pattern_outline_fit'); + db_add_index('router', 'pattern_outline_parts', array('pattern_outline', 'number_parts')); +}