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 8e350ffd76ac76196bb6ea480c10ebf7c37af688..58b8320b40cf7b04053f30b996c066089f34a96f 100644 --- a/core/lib/Drupal/Core/Routing/RouteProvider.php +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php @@ -325,11 +325,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(); @@ -338,21 +336,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 e8411d6483b9086a9631365846899c4ebec1188f..c1d24ff33a68bb8ad87d3c7879ff41399efbf923 100644 --- a/core/modules/system/src/Tests/Routing/RouteProviderTest.php +++ b/core/modules/system/src/Tests/Routing/RouteProviderTest.php @@ -324,7 +324,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, $this->cacheTagsInvalidator, '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 207049f2ad7282a50b92974aa29c0c120860f194..9f6e8c8133701a653b6878e371ad249fae2cbea4 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/src/Tests/Update/RouterIndexOptimizationTest.php b/core/modules/system/src/Tests/Update/RouterIndexOptimizationTest.php new file mode 100644 index 0000000000000000000000000000000000000000..b206011870ac76fca99860bd252c9b63798aa3f3 --- /dev/null +++ b/core/modules/system/src/Tests/Update/RouterIndexOptimizationTest.php @@ -0,0 +1,41 @@ +databaseDumpFiles = [ + __DIR__ . '/../../../tests/fixtures/update/drupal-8.bare.standard.php.gz', + ]; + parent::setUp(); + } + + /** + * Ensures that the system_update_8002() runs as expected. + */ + public function testUpdate() { + $this->runUpdates(); + $database = $this->container->get('database'); + // Removed index. + $this->assertFalse($database->schema()->indexExists( + 'router', 'pattern_outline_fit' + )); + // Added index. + $this->assertTrue($database->schema()->indexExists( + 'router', 'pattern_outline_parts' + )); + } + +} diff --git a/core/modules/system/system.install b/core/modules/system/system.install index 948e6f4e5270aeee195042514ea8fe5a055eeb59..f38af8a021a6c42ba998b393ed24ba28a5e656bc 100644 --- a/core/modules/system/system.install +++ b/core/modules/system/system.install @@ -1001,7 +1001,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'), ); @@ -1210,3 +1210,34 @@ function system_update_8002() { \Drupal::configFactory()->getEditable('system.filter')->delete(); return t('The system.filter configuration has been moved to a container parameter, see default.services.yml for more information.'); } + +/** + * Change the index on the {router} table. + */ +function system_update_8003() { + $database = \Drupal::database(); + $database->schema()->dropIndex('router', 'pattern_outline_fit'); + $database->schema()->addIndex( + 'router', + 'pattern_outline_parts', + ['pattern_outline', 'number_parts'], + [ + 'fields' => [ + 'pattern_outline' => [ + 'description' => 'The pattern', + 'type' => 'varchar', + 'length' => 255, + 'not null' => TRUE, + 'default' => '', + ], + 'number_parts' => [ + 'description' => 'Number of parts in this router path.', + 'type' => 'int', + 'not null' => TRUE, + 'default' => 0, + 'size' => 'small', + ], + ], + ] + ); +}