diff --git a/core/includes/menu.inc b/core/includes/menu.inc index 6492b10..895174c 100644 --- a/core/includes/menu.inc +++ b/core/includes/menu.inc @@ -2668,8 +2668,7 @@ function menu_router_rebuild() { $transaction = db_transaction(); try { - list($menu, $masks) = menu_router_build(); - _menu_router_save($menu, $masks); + list($menu, $masks) = menu_router_build(TRUE); _menu_navigation_links_rebuild($menu); // Clear the menu, page and block caches. menu_cache_clear_all(); @@ -2688,8 +2687,11 @@ function menu_router_rebuild() { /** * 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 // a given item came from. $callbacks = array(); @@ -2704,7 +2706,7 @@ function menu_router_build() { } // Alter the menu as defined in modules, keys are like user/%user. drupal_alter('menu', $callbacks); - list($menu, $masks) = _menu_router_build($callbacks); + list($menu, $masks) = _menu_router_build($callbacks, $save); _menu_router_cache($menu); return array($menu, $masks); @@ -3506,11 +3508,12 @@ function _menu_link_parents_set(&$item, $parent) { /** * 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 // matching. Calculate fitness, and fill some default values. $menu = array(); $masks = array(); + $path_roots = array(); foreach ($callbacks as $path => $item) { $load_functions = array(); $to_arg_functions = array(); @@ -3518,6 +3521,7 @@ function _menu_router_build($callbacks) { $move = FALSE; $parts = explode('/', $path, MENU_MAX_PARTS); + $path_roots[$parts[0]] = $parts[0]; $number_parts = count($parts); // We store the highest index of parts here to save some work in the fit // calculation loop. @@ -3724,6 +3728,17 @@ function _menu_router_build($callbacks) { $masks = array_keys($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); } diff --git a/core/lib/Drupal/Core/CoreBundle.php b/core/lib/Drupal/Core/CoreBundle.php index e1147ac..55f9a93 100644 --- a/core/lib/Drupal/Core/CoreBundle.php +++ b/core/lib/Drupal/Core/CoreBundle.php @@ -75,9 +75,15 @@ public function build(ContainerBuilder $container) { ->register('keyvalue.database', 'Drupal\Core\KeyValueStore\KeyValueDatabaseFactory') ->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')); + + $container->register('path.alias_manager', 'Drupal\Core\Path\AliasManager') ->addArgument(new Reference('database')) - ->addArgument(new Reference('keyvalue')); + ->addArgument(new Reference('path.alias_whitelist')); $container->register('http_client_simpletest_subscriber', 'Drupal\Core\Http\Plugin\SimpletestHttpRequestSubscriber'); $container->register('http_default_client', 'Guzzle\Http\Client') diff --git a/core/lib/Drupal/Core/Path/AliasManager.php b/core/lib/Drupal/Core/Path/AliasManager.php index 47bf77b..8a6b0a8 100644 --- a/core/lib/Drupal/Core/Path/AliasManager.php +++ b/core/lib/Drupal/Core/Path/AliasManager.php @@ -8,7 +8,6 @@ namespace Drupal\Core\Path; use Drupal\Core\Database\Connection; -use Drupal\Core\KeyValueStore\KeyValueFactory; class AliasManager implements AliasManagerInterface { @@ -20,13 +19,6 @@ class AliasManager implements AliasManagerInterface { protected $connection; /** - * The Key/Value Store to use for state - * - * @var \Drupal\Core\KeyValueStore\DatabaseStorage - */ - protected $state; - - /** * The default langcode to use when none is specified for path lookups. * * @var string @@ -50,7 +42,7 @@ class AliasManager implements AliasManagerInterface { /** * Holds the array of whitelisted path aliases. * - * @var array + * @var \Drupal\Core\Utility\PathAliasWhitelist; */ protected $whitelist; @@ -78,14 +70,18 @@ class AliasManager implements AliasManagerInterface { */ protected $preloadedPathLookups = array(); - public function __construct(Connection $connection, KeyValueFactory $keyvalue) { + /** + * 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. + */ + public function __construct(Connection $connection, AliasWhitelist $whitelist) { $this->connection = $connection; - $this->state = $keyvalue->get('state'); $this->langcode = language(LANGUAGE_TYPE_URL)->langcode; - $this->whitelist = $this->state->get('system.path_alias_whitelist', NULL); - if (!isset($this->whitelist)) { - $this->whitelist = $this->pathAliasWhitelistRebuild(); - } + $this->whitelist = $whitelist; } /** @@ -131,7 +127,7 @@ public function cacheClear($source = NULL) { $this->no_aliases = array(); $this->firstCall = TRUE; $this->preloadedPathLookups = array(); - $this->whitelist = $this->pathAliasWhitelistRebuild($source); + $this->pathAliasWhitelistRebuild($source); } /** @@ -300,21 +296,10 @@ protected function pathAliasWhitelistRebuild($source = NULL) { // When paths are inserted, only rebuild the whitelist if the system path // has a top level component which is not already in the whitelist. if (!empty($source)) { - // @todo Inject state so we don't have this function call. - $whitelist = $this->state->get('system.path_alias_whitelist', NULL); - 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; + if (isset($this->whitelist[strtok($source, '/')])) { + return; + } } - $this->state->set('system.path_alias_whitelist', $whitelist); - return $whitelist; + $this->whitelist->clear(); } } diff --git a/core/lib/Drupal/Core/Path/AliasWhitelist.php b/core/lib/Drupal/Core/Path/AliasWhitelist.php new file mode 100644 index 0000000..6c62cf4 --- /dev/null +++ b/core/lib/Drupal/Core/Path/AliasWhitelist.php @@ -0,0 +1,137 @@ +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(); + } +} diff --git a/core/lib/Drupal/Core/Utility/CacheArray.php b/core/lib/Drupal/Core/Utility/CacheArray.php index eddfc27..6eef834 100644 --- a/core/lib/Drupal/Core/Utility/CacheArray.php +++ b/core/lib/Drupal/Core/Utility/CacheArray.php @@ -212,6 +212,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. */ public function __destruct() { diff --git a/core/modules/forum/lib/Drupal/forum/Tests/ForumNodeAccessTest.php b/core/modules/forum/lib/Drupal/forum/Tests/ForumNodeAccessTest.php index 402409a..3617a9e 100644 --- a/core/modules/forum/lib/Drupal/forum/Tests/ForumNodeAccessTest.php +++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumNodeAccessTest.php @@ -91,7 +91,7 @@ function testForumNodeAccess() { // Test for $access_user. $this->drupalLogin($access_user); - $this->drupalGet('/'); + $this->drupalGet(''); // Ensure private node and public node are found. $this->assertText($private_node->title, 'Private node found in block by $access_user'); @@ -99,7 +99,7 @@ function testForumNodeAccess() { // Test for $no_access_user. $this->drupalLogin($no_access_user); - $this->drupalGet('/'); + $this->drupalGet(''); // 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'); diff --git a/core/modules/path/lib/Drupal/path/Tests/PathAliasTest.php b/core/modules/path/lib/Drupal/path/Tests/PathAliasTest.php index 23ff7f7..b6ce86d 100644 --- a/core/modules/path/lib/Drupal/path/Tests/PathAliasTest.php +++ b/core/modules/path/lib/Drupal/path/Tests/PathAliasTest.php @@ -51,7 +51,8 @@ function testPathCache() { // Visit the system path for the node and confirm a cache entry is // created. 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.'); // Visit the alias for the node and confirm a cache entry is created. diff --git a/core/modules/system/lib/Drupal/system/Tests/Path/AliasTest.php b/core/modules/system/lib/Drupal/system/Tests/Path/AliasTest.php index 21a129a..2709fd9 100644 --- a/core/modules/system/lib/Drupal/system/Tests/Path/AliasTest.php +++ b/core/modules/system/lib/Drupal/system/Tests/Path/AliasTest.php @@ -7,10 +7,11 @@ namespace Drupal\system\Tests\Path; -use Drupal\simpletest\DrupalUnitTestBase; use Drupal\Core\Database\Database; -use Drupal\Core\Path\Path; use Drupal\Core\Path\AliasManager; +use Drupal\Core\Path\AliasWhitelist; +use Drupal\Core\Path\Path; +use Drupal\simpletest\DrupalUnitTestBase; /** * Tests path alias CRUD and lookup functionality. @@ -28,6 +29,9 @@ public static function getInfo() { public function setUp() { parent::setUp(); $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() { @@ -43,7 +47,8 @@ function testCRUD() { $this->fixtures->createTables($connection); //Create AliasManager and Path object. - $aliasManager = new AliasManager($connection, $this->container->get('keyvalue')); + $whitelist = new AliasWhitelist('path_alias_whitelist', 'cache', $this->container->get('keyvalue'), $connection); + $aliasManager = new AliasManager($connection, $whitelist); $path = new Path($connection, $aliasManager); $aliases = $this->fixtures->sampleUrlAliases(); @@ -96,7 +101,8 @@ function testLookupPath() { $this->fixtures->createTables($connection); //Create AliasManager and Path object. - $aliasManager = new AliasManager($connection, $this->container->get('keyvalue')); + $whitelist = new AliasWhitelist('path_alias_whitelist', 'cache', $this->container->get('keyvalue'), $connection); + $aliasManager = new AliasManager($connection, $whitelist); $pathObject = new Path($connection, $aliasManager); // Test the situation where the source is the same for multiple aliases. @@ -160,4 +166,43 @@ function testLookupPath() { $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.'); } + + /** + * Test 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); + $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. + $this->assertNull($whitelist['invalid']); + + // Add an alias for user/1, user should get whitelisted now. + $path->save('user/1', 'admin'); + $this->assertTrue($whitelist['user']); + $this->assertNull($whitelist['admin']); + $this->assertNull($whitelist['invalid']); + + // Add an alias for admin, both should get whitelisted now. + $path->save('admin/something', 'admin'); + $this->assertTrue($whitelist['user']); + $this->assertTrue($whitelist['admin']); + $this->assertNull($whitelist['invalid']); + + // 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['invalid']); + + } }