Commit 6f3e7073 authored by catch's avatar catch

Issue #1209226 by catch, Berdir, miro_dietiker, beejeebus, dagmar: Fixed Avoid...

Issue #1209226 by catch, Berdir, miro_dietiker, beejeebus, dagmar: Fixed Avoid slow query for path alias whitelists.
parent 219ec3c0
...@@ -2619,8 +2619,7 @@ function menu_router_rebuild() { ...@@ -2619,8 +2619,7 @@ function menu_router_rebuild() {
$transaction = db_transaction(); $transaction = db_transaction();
try { try {
list($menu, $masks) = menu_router_build(); list($menu, $masks) = menu_router_build(TRUE);
_menu_router_save($menu, $masks);
_menu_navigation_links_rebuild($menu); _menu_navigation_links_rebuild($menu);
// Clear the menu, page and block caches. // Clear the menu, page and block caches.
menu_cache_clear_all(); menu_cache_clear_all();
...@@ -2639,8 +2638,11 @@ function menu_router_rebuild() { ...@@ -2639,8 +2638,11 @@ function menu_router_rebuild() {
/** /**
* Collects and alters the menu definitions. * Collects and alters the menu definitions.
*
* @param bool $save
* (optional) Save the new router to the database. Defaults to FALSE.
*/ */
function menu_router_build() { function menu_router_build($save = FALSE) {
// We need to manually call each module so that we can know which module // We need to manually call each module so that we can know which module
// a given item came from. // a given item came from.
$callbacks = array(); $callbacks = array();
...@@ -2666,7 +2668,7 @@ function menu_router_build() { ...@@ -2666,7 +2668,7 @@ function menu_router_build() {
} }
// Alter the menu as defined in modules, keys are like user/%user. // Alter the menu as defined in modules, keys are like user/%user.
drupal_alter('menu', $callbacks); drupal_alter('menu', $callbacks);
list($menu, $masks) = _menu_router_build($callbacks); list($menu, $masks) = _menu_router_build($callbacks, $save);
_menu_router_cache($menu); _menu_router_cache($menu);
return array($menu, $masks); return array($menu, $masks);
...@@ -2949,11 +2951,12 @@ function _menu_find_router_path($link_path) { ...@@ -2949,11 +2951,12 @@ function _menu_find_router_path($link_path) {
/** /**
* Builds the router table based on the data from hook_menu(). * Builds the router table based on the data from hook_menu().
*/ */
function _menu_router_build($callbacks) { function _menu_router_build($callbacks, $save = FALSE) {
// First pass: separate callbacks from paths, making paths ready for // First pass: separate callbacks from paths, making paths ready for
// matching. Calculate fitness, and fill some default values. // matching. Calculate fitness, and fill some default values.
$menu = array(); $menu = array();
$masks = array(); $masks = array();
$path_roots = array();
foreach ($callbacks as $path => $item) { foreach ($callbacks as $path => $item) {
$load_functions = array(); $load_functions = array();
$to_arg_functions = array(); $to_arg_functions = array();
...@@ -2961,6 +2964,7 @@ function _menu_router_build($callbacks) { ...@@ -2961,6 +2964,7 @@ function _menu_router_build($callbacks) {
$move = FALSE; $move = FALSE;
$parts = explode('/', $path, MENU_MAX_PARTS); $parts = explode('/', $path, MENU_MAX_PARTS);
$path_roots[$parts[0]] = $parts[0];
$number_parts = count($parts); $number_parts = count($parts);
// We store the highest index of parts here to save some work in the fit // We store the highest index of parts here to save some work in the fit
// calculation loop. // calculation loop.
...@@ -3169,6 +3173,17 @@ function _menu_router_build($callbacks) { ...@@ -3169,6 +3173,17 @@ function _menu_router_build($callbacks) {
$masks = array_keys($masks); $masks = array_keys($masks);
rsort($masks); rsort($masks);
if ($save) {
$path_roots = array_values($path_roots);
// Update the path roots variable and reset the path alias whitelist cache
// if the list has changed.
if ($path_roots != state()->get('menu_path_roots')) {
state()->set('menu_path_roots', array_values($path_roots));
drupal_container()->get('path.alias_manager')->cacheClear();
}
_menu_router_save($menu, $masks);
}
return array($menu, $masks); return array($menu, $masks);
} }
......
...@@ -125,9 +125,16 @@ public function build(ContainerBuilder $container) { ...@@ -125,9 +125,16 @@ public function build(ContainerBuilder $container) {
->register('queue.database', 'Drupal\Core\Queue\QueueDatabaseFactory') ->register('queue.database', 'Drupal\Core\Queue\QueueDatabaseFactory')
->addArgument(new Reference('database')); ->addArgument(new Reference('database'));
$container->register('path.alias_manager', 'Drupal\Core\Path\AliasManager') $container->register('path.alias_whitelist', 'Drupal\Core\Path\AliasWhitelist')
->addArgument('path_alias_whitelist')
->addArgument('cache')
->addArgument(new Reference('keyvalue'))
->addArgument(new Reference('database')) ->addArgument(new Reference('database'))
->addArgument(new Reference('state')) ->addTag('needs_destruction');
$container->register('path.alias_manager', 'Drupal\Core\Path\AliasManager')
->addArgument(new Reference('database'))
->addArgument(new Reference('path.alias_whitelist'))
->addArgument(new Reference('language_manager')); ->addArgument(new Reference('language_manager'));
$container->register('http_client_simpletest_subscriber', 'Drupal\Core\Http\Plugin\SimpletestHttpRequestSubscriber'); $container->register('http_client_simpletest_subscriber', 'Drupal\Core\Http\Plugin\SimpletestHttpRequestSubscriber');
......
...@@ -8,7 +8,6 @@ ...@@ -8,7 +8,6 @@
namespace Drupal\Core\Path; namespace Drupal\Core\Path;
use Drupal\Core\Database\Connection; use Drupal\Core\Database\Connection;
use Drupal\Core\KeyValueStore\KeyValueStoreInterface;
use Drupal\Core\Language\LanguageManager; use Drupal\Core\Language\LanguageManager;
class AliasManager implements AliasManagerInterface { class AliasManager implements AliasManagerInterface {
...@@ -51,7 +50,7 @@ class AliasManager implements AliasManagerInterface { ...@@ -51,7 +50,7 @@ class AliasManager implements AliasManagerInterface {
/** /**
* Holds the array of whitelisted path aliases. * Holds the array of whitelisted path aliases.
* *
* @var array * @var \Drupal\Core\Utility\PathAliasWhitelist;
*/ */
protected $whitelist; protected $whitelist;
...@@ -79,15 +78,20 @@ class AliasManager implements AliasManagerInterface { ...@@ -79,15 +78,20 @@ class AliasManager implements AliasManagerInterface {
*/ */
protected $preloadedPathLookups = array(); protected $preloadedPathLookups = array();
public function __construct(Connection $connection, KeyValueStoreInterface $state, LanguageManager $language_manager) { /**
* Constructs an AliasManager.
*
* @param \Drupal\Core\Database\Connection $connection
* The database connection to use.
* @param \Drupal\Core\Path\AliasWhitelist $whitelist
* The whitelist implementation to use.
* @param \Drupal\Core\Language\LanguageManager $language_manager
* The language manager.
*/
public function __construct(Connection $connection, AliasWhitelist $whitelist, LanguageManager $language_manager) {
$this->connection = $connection; $this->connection = $connection;
$this->state = $state;
$this->languageManager = $language_manager; $this->languageManager = $language_manager;
$this->whitelist = $this->state->get('system.path_alias_whitelist', NULL); $this->whitelist = $whitelist;
if (!isset($this->whitelist)) {
$this->whitelist = $this->pathAliasWhitelistRebuild();
}
} }
/** /**
...@@ -133,7 +137,7 @@ public function cacheClear($source = NULL) { ...@@ -133,7 +137,7 @@ public function cacheClear($source = NULL) {
$this->no_aliases = array(); $this->no_aliases = array();
$this->firstCall = TRUE; $this->firstCall = TRUE;
$this->preloadedPathLookups = array(); $this->preloadedPathLookups = array();
$this->whitelist = $this->pathAliasWhitelistRebuild($source); $this->pathAliasWhitelistRebuild($source);
} }
/** /**
...@@ -302,21 +306,10 @@ protected function pathAliasWhitelistRebuild($source = NULL) { ...@@ -302,21 +306,10 @@ protected function pathAliasWhitelistRebuild($source = NULL) {
// When paths are inserted, only rebuild the whitelist if the system path // When paths are inserted, only rebuild the whitelist if the system path
// has a top level component which is not already in the whitelist. // has a top level component which is not already in the whitelist.
if (!empty($source)) { if (!empty($source)) {
// @todo Inject state so we don't have this function call. if (isset($this->whitelist[strtok($source, '/')])) {
$whitelist = $this->state->get('system.path_alias_whitelist', NULL); return;
if (isset($whitelist[strtok($source, '/')])) { }
return $whitelist;
}
}
// For each alias in the database, get the top level component of the system
// path it corresponds to. This is the portion of the path before the first
// '/', if present, otherwise the whole path itself.
$whitelist = array();
$result = $this->connection->query("SELECT DISTINCT SUBSTRING_INDEX(source, '/', 1) AS path FROM {url_alias}");
foreach ($result as $row) {
$whitelist[$row->path] = TRUE;
} }
$this->state->set('system.path_alias_whitelist', $whitelist); $this->whitelist->clear();
return $whitelist;
} }
} }
<?php
/**
* @file
* Contains \Drupal\Core\Path\AliasWhitelist.
*/
namespace Drupal\Core\Path;
use Drupal\Core\Database\Connection;
use Drupal\Core\DestructableInterface;
use Drupal\Core\KeyValueStore\KeyValueFactory;
use Drupal\Core\Utility\CacheArray;
/**
* Extends CacheArray to build the path alias whitelist over time.
*/
class AliasWhitelist extends CacheArray implements DestructableInterface {
/**
* The Key/Value Store to use for state.
*
* @var \Drupal\Core\KeyValueStore\KeyValueStoreInterface
*/
protected $state;
/**
* The database connection.
*
* @var \Drupal\Core\Database\Connection
*/
protected $connection;
/**
* Constructs an AliasWhitelist object.
*
* @param string $cid
* The cache id to use.
* @param string $bin
* The cache bin that should be used.
* @param \Drupal\Core\KeyValueStore\KeyValueFactory $keyvalue
* The keyvalue factory to get the state cache from.
* @param \Drupal\Core\Database\Connection $connection
* The database connection.
*/
public function __construct($cid, $bin, KeyValueFactory $keyvalue, Connection $connection) {
parent::__construct($cid, $bin);
$this->state = $keyvalue->get('state');
$this->connection = $connection;
// On a cold start $this->storage will be empty and the whitelist will
// need to be rebuilt from scratch. The whitelist is initialized from the
// list of all valid path roots stored in the 'menu_path_roots' state,
// with values initialized to NULL. During the request, each path requested
// that matches one of these keys will be looked up and the array value set
// to either TRUE or FALSE. This ensures that paths which do not exist in
// the router are not looked up, and that paths that do exist in the router
// are only looked up once.
if (empty($this->storage)) {
$this->loadMenuPathRoots();
}
}
/**
* Loads menu path roots to prepopulate cache.
*/
protected function loadMenuPathRoots() {
if ($roots = $this->state->get('menu_path_roots')) {
foreach ($roots as $root) {
$this->storage[$root] = NULL;
$this->persist($root);
}
}
}
/**
* Overrides \ArrayAccess::offsetGet().
*/
public function offsetGet($offset) {
// url() may be called with paths that are not represented by menu router
// items such as paths that will be rewritten by hook_url_outbound_alter().
// Therefore internally TRUE is used to indicate whitelisted paths. FALSE is
// used to indicate paths that have already been checked but are not
// whitelisted, and NULL indicates paths that have not been checked yet.
if (isset($this->storage[$offset])) {
if ($this->storage[$offset]) {
return TRUE;
}
}
elseif (array_key_exists($offset, $this->storage)) {
return $this->resolveCacheMiss($offset);
}
}
/**
* Overrides \Drupal\Core\Utility\CacheArray::resolveCacheMiss().
*/
public function resolveCacheMiss($root) {
$query = $this->connection->select('url_alias', 'u');
$query->addExpression(1);
$exists = (bool) $query
->condition('u.source', $this->connection->escapeLike($root) . '%', 'LIKE')
->range(0, 1)
->execute()
->fetchField();
$this->storage[$root] = $exists;
$this->persist($root);
if ($exists) {
return TRUE;
}
}
/**
* Overrides \Drupal\Core\Utility\CacheArray::set().
*/
public function set($data, $lock = TRUE) {
$lock_name = $this->cid . ':' . $this->bin;
if (!$lock || lock()->acquire($lock_name)) {
if ($cached = cache($this->bin)->get($this->cid)) {
// Use array merge instead of union so that filled in values in $data
// overwrite empty values in the current cache.
$data = array_merge($cached->data, $data);
}
cache($this->bin)->set($this->cid, $data);
if ($lock) {
lock()->release($lock_name);
}
}
}
/**
* Overrides \Drupal\Core\Utility\CacheArray::clear().
*/
public function clear() {
parent::clear();
$this->loadMenuPathRoots();
}
/**
* Implements Drupal\Core\DestructableInterface::destruct().
*/
public function destruct() {
parent::__destruct();
}
/**
* Overrides \Drupal\Core\Utility\CacheArray::clear().
*/
public function __destruct() {
// Do nothing to avoid segmentation faults. This can go away after the
// cache collector from http://drupal.org/node/1786490 is used.
}
}
...@@ -211,6 +211,15 @@ protected function set($data, $lock = TRUE) { ...@@ -211,6 +211,15 @@ protected function set($data, $lock = TRUE) {
} }
} }
/**
* Clear the cache.
*/
public function clear() {
$this->storage = array();
$this->keysToPersist = array();
cache($this->bin)->delete($this->cid);
}
/** /**
* Destructs the CacheArray object. * Destructs the CacheArray object.
*/ */
......
...@@ -82,7 +82,7 @@ function testForumNodeAccess() { ...@@ -82,7 +82,7 @@ function testForumNodeAccess() {
// Test for $access_user. // Test for $access_user.
$this->drupalLogin($access_user); $this->drupalLogin($access_user);
$this->drupalGet('/'); $this->drupalGet('');
// Ensure private node and public node are found. // Ensure private node and public node are found.
$this->assertText($private_node->title, 'Private node found in block by $access_user'); $this->assertText($private_node->title, 'Private node found in block by $access_user');
...@@ -90,7 +90,7 @@ function testForumNodeAccess() { ...@@ -90,7 +90,7 @@ function testForumNodeAccess() {
// Test for $no_access_user. // Test for $no_access_user.
$this->drupalLogin($no_access_user); $this->drupalLogin($no_access_user);
$this->drupalGet('/'); $this->drupalGet('');
// Ensure private node is not found but public is found. // Ensure private node is not found but public is found.
$this->assertNoText($private_node->title, 'Private node not found in block by $no_access_user'); $this->assertNoText($private_node->title, 'Private node not found in block by $no_access_user');
......
...@@ -51,7 +51,8 @@ function testPathCache() { ...@@ -51,7 +51,8 @@ function testPathCache() {
// Visit the system path for the node and confirm a cache entry is // Visit the system path for the node and confirm a cache entry is
// created. // created.
cache('path')->deleteAll(); cache('path')->deleteAll();
$this->drupalGet($edit['source']); // Make sure the path is not converted to the alias.
$this->drupalGet($edit['source'], array('alias' => TRUE));
$this->assertTrue(cache('path')->get($edit['source']), 'Cache entry was created.'); $this->assertTrue(cache('path')->get($edit['source']), 'Cache entry was created.');
// Visit the alias for the node and confirm a cache entry is created. // Visit the alias for the node and confirm a cache entry is created.
......
...@@ -7,10 +7,10 @@ ...@@ -7,10 +7,10 @@
namespace Drupal\system\Tests\Path; namespace Drupal\system\Tests\Path;
use Drupal\simpletest\DrupalUnitTestBase;
use Drupal\Core\Database\Database;
use Drupal\Core\Path\Path; use Drupal\Core\Path\Path;
use Drupal\Core\Database\Database;
use Drupal\Core\Path\AliasManager; use Drupal\Core\Path\AliasManager;
use Drupal\Core\Path\AliasWhitelist;
/** /**
* Tests path alias CRUD and lookup functionality. * Tests path alias CRUD and lookup functionality.
...@@ -31,7 +31,8 @@ function testCRUD() { ...@@ -31,7 +31,8 @@ function testCRUD() {
$this->fixtures->createTables($connection); $this->fixtures->createTables($connection);
//Create AliasManager and Path object. //Create AliasManager and Path object.
$aliasManager = new AliasManager($connection, $this->container->get('state'), $this->container->get('language_manager')); $whitelist = new AliasWhitelist('path_alias_whitelist', 'cache', $this->container->get('keyvalue'), $connection);
$aliasManager = new AliasManager($connection, $whitelist, $this->container->get('language_manager'));
$path = new Path($connection, $aliasManager); $path = new Path($connection, $aliasManager);
$aliases = $this->fixtures->sampleUrlAliases(); $aliases = $this->fixtures->sampleUrlAliases();
...@@ -84,7 +85,8 @@ function testLookupPath() { ...@@ -84,7 +85,8 @@ function testLookupPath() {
$this->fixtures->createTables($connection); $this->fixtures->createTables($connection);
//Create AliasManager and Path object. //Create AliasManager and Path object.
$aliasManager = new AliasManager($connection, $this->container->get('state'), $this->container->get('language_manager')); $whitelist = new AliasWhitelist('path_alias_whitelist', 'cache', $this->container->get('keyvalue'), $connection);
$aliasManager = new AliasManager($connection, $whitelist, $this->container->get('language_manager'));
$pathObject = new Path($connection, $aliasManager); $pathObject = new Path($connection, $aliasManager);
// Test the situation where the source is the same for multiple aliases. // Test the situation where the source is the same for multiple aliases.
...@@ -148,4 +150,44 @@ function testLookupPath() { ...@@ -148,4 +150,44 @@ function testLookupPath() {
$pathObject->save('user/2', 'bar'); $pathObject->save('user/2', 'bar');
$this->assertEqual($aliasManager->getSystemPath('bar'), 'user/2', 'Newer alias record is returned when comparing two LANGUAGE_NOT_SPECIFIED paths with the same alias.'); $this->assertEqual($aliasManager->getSystemPath('bar'), 'user/2', 'Newer alias record is returned when comparing two LANGUAGE_NOT_SPECIFIED paths with the same alias.');
} }
/**
* Tests the alias whitelist.
*/
function testWhitelist() {
// Prepare database table.
$connection = Database::getConnection();
$this->fixtures->createTables($connection);
// Create AliasManager and Path object.
$whitelist = new AliasWhitelist('path_alias_whitelist', 'cache', $this->container->get('keyvalue'), $connection);
$aliasManager = new AliasManager($connection, $whitelist, $this->container->get('language_manager'));
$path = new Path($connection, $aliasManager);
// No alias for user and admin yet, so should be NULL.
$this->assertNull($whitelist['user']);
$this->assertNull($whitelist['admin']);
// Non-existing path roots should be NULL too. Use a length of 7 to avoid
// possible conflict with random aliases below.
$this->assertNull($whitelist[$this->randomName()]);
// Add an alias for user/1, user should get whitelisted now.
$path->save('user/1', $this->randomName());
$this->assertTrue($whitelist['user']);
$this->assertNull($whitelist['admin']);
$this->assertNull($whitelist[$this->randomName()]);
// Add an alias for admin, both should get whitelisted now.
$path->save('admin/something', $this->randomName());
$this->assertTrue($whitelist['user']);
$this->assertTrue($whitelist['admin']);
$this->assertNull($whitelist[$this->randomName()]);
// Remove the user alias again, whitelist entry should be removed.
$path->delete(array('source' => 'user/1'));
$this->assertNull($whitelist['user']);
$this->assertTrue($whitelist['admin']);
$this->assertNull($whitelist[$this->randomName()]);
}
} }
...@@ -18,6 +18,9 @@ class PathUnitTestBase extends DrupalUnitTestBase { ...@@ -18,6 +18,9 @@ class PathUnitTestBase extends DrupalUnitTestBase {
public function setUp() { public function setUp() {
parent::setUp(); parent::setUp();
$this->fixtures = new UrlAliasFixtures(); $this->fixtures = new UrlAliasFixtures();
// The alias whitelist expects that the menu path roots are set by a
// menu router rebuild.
state()->set('menu_path_roots', array('user', 'admin'));
} }
public function tearDown() { public function tearDown() {
......
...@@ -11,6 +11,7 @@ ...@@ -11,6 +11,7 @@
use Drupal\Core\Database\Database; use Drupal\Core\Database\Database;
use Drupal\Core\Path\Path; use Drupal\Core\Path\Path;
use Drupal\Core\Path\AliasManager; use Drupal\Core\Path\AliasManager;
use Drupal\Core\Path\AliasWhitelist;
use Drupal\Core\PathProcessor\PathProcessorAlias; use Drupal\Core\PathProcessor\PathProcessorAlias;
use Drupal\Core\PathProcessor\PathProcessorDecode; use Drupal\Core\PathProcessor\PathProcessorDecode;
use Drupal\Core\PathProcessor\PathProcessorFront; use Drupal\Core\PathProcessor\PathProcessorFront;
...@@ -46,7 +47,8 @@ function testProcessInbound() { ...@@ -46,7 +47,8 @@ function testProcessInbound() {
$this->fixtures->createTables($connection); $this->fixtures->createTables($connection);
// Create dependecies needed by various path processors. // Create dependecies needed by various path processors.
$alias_manager = new AliasManager($connection, $this->container->get('state'), $this->container->get('language_manager')); $whitelist = new AliasWhitelist('path_alias_whitelist', 'cache', $this->container->get('keyvalue'), $connection);
$alias_manager = new AliasManager($connection, $whitelist, $this->container->get('language_manager'));
$module_handler = $this->container->get('module_handler'); $module_handler = $this->container->get('module_handler');
// Create the processors. // Create the processors.
......
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