Issue #2228217 by pwolanin, ecrown, neclimdul, Fabianx, dawehner, klausi: Further optimize RouteProvider and add web test for large number of path parts

core/lib/Drupal/Core/Routing/RouteCompiler.php

@@ -43,7 +43,9 @@ class RouteCompiler extends SymfonyRouteCompiler implements RouteCompilerInterfa
     $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,

core/lib/Drupal/Core/Routing/RouteProvider.php

@@ -298,11 +298,9 @@ class RouteProvider implements PreloadableRouteProviderInterface, PagedRouteProv
    *   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 @@ class RouteProvider implements PreloadableRouteProviderInterface, PagedRouteProv
       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}
    */

core/modules/system/src/Tests/Routing/RouteProviderTest.php

@@ -316,7 +316,46 @@ class RouteProviderTest extends KernelTestBase {
       $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.');

core/modules/system/src/Tests/Routing/RouterTest.php

@@ -197,6 +197,15 @@ class RouterTest extends WebTestBase {
     $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);
+    }
   }
 
   /**

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'));
+}