Commit 7730866f authored by alexpott's avatar alexpott

Issue #2228217 by pwolanin, neclimdul, lauriii, ecrown, josephdpurcell,...

Issue #2228217 by pwolanin, neclimdul, lauriii, ecrown, josephdpurcell, Fabianx, dawehner, klausi: Further optimize RouteProvider and add web test for large number of path parts
parent 9435942a
......@@ -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,
......
......@@ -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}
*/
......
......@@ -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.');
......
......@@ -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);
}
}
/**
......
<?php
/**
* @file
* Contains \Drupal\system\Tests\Update\RouterIndexOptimizationTest.
*/
namespace Drupal\system\Tests\Update;
/**
* Tests system_update_8002().
*
* @group Update
*/
class RouterIndexOptimizationTest extends UpdatePathTestBase {
/**
* {@inheritdoc}
*/
public function setUp() {
$this->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'
));
}
}
......@@ -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',
],
],
]
);
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment