From 09702f841389ab2c4e93062d621fb40f1c3959f0 Mon Sep 17 00:00:00 2001 From: Nathaniel Catchpole Date: Wed, 28 May 2014 10:44:50 +0100 Subject: [PATCH] Issue #2233623 by Berdir, slashrsm, xjm: Fixed Merge AliasManagerCacheDecorator into AliasManager. --- core/core.services.yml | 7 +- core/includes/common.inc | 2 +- core/includes/theme.inc | 2 +- .../AliasManagerCacheDecorator.php | 128 ------------------ core/lib/Drupal/Core/Path/AliasManager.php | 128 +++++++++++++----- .../Core/Path/AliasManagerInterface.php | 17 --- .../Plugin/Field/FieldWidget/LinkWidget.php | 2 +- core/modules/menu_link/src/MenuLinkForm.php | 2 +- core/modules/path/src/Tests/PathAliasTest.php | 4 +- .../system/src/Tests/Path/AliasTest.php | 6 +- .../src/Tests/Routing/MockAliasManager.php | 14 -- core/modules/system/system.module | 6 +- .../src/Plugin/views/argument_default/Raw.php | 2 +- core/modules/views_ui/views_ui.module | 2 +- 14 files changed, 110 insertions(+), 212 deletions(-) delete mode 100644 core/lib/Drupal/Core/CacheDecorator/AliasManagerCacheDecorator.php diff --git a/core/core.services.yml b/core/core.services.yml index 306ae953b52..66bc3f2bebc 100644 --- a/core/core.services.yml +++ b/core/core.services.yml @@ -184,7 +184,7 @@ services: arguments: [path_alias_whitelist, '@cache.default', '@lock', '@state', '@path.alias_storage'] path.alias_manager: class: Drupal\Core\Path\AliasManager - arguments: ['@path.alias_storage', '@path.alias_whitelist', '@language_manager'] + arguments: ['@path.alias_storage', '@path.alias_whitelist', '@language_manager', '@cache.data'] http_client: class: Drupal\Core\Http\Client tags: @@ -393,9 +393,6 @@ services: arguments: ['@router.builder', '@lock'] tags: - { name: event_subscriber } - path.alias_manager.cached: - class: Drupal\Core\CacheDecorator\AliasManagerCacheDecorator - arguments: ['@path.alias_manager', '@cache.data'] path.alias_storage: class: Drupal\Core\Path\AliasStorage arguments: ['@database', '@module_handler'] @@ -588,7 +585,7 @@ services: class: Drupal\Core\EventSubscriber\PathSubscriber tags: - { name: event_subscriber } - arguments: ['@path.alias_manager.cached', '@path_processor_manager'] + arguments: ['@path.alias_manager', '@path_processor_manager'] legacy_request_subscriber: class: Drupal\Core\EventSubscriber\LegacyRequestSubscriber tags: diff --git a/core/includes/common.inc b/core/includes/common.inc index 65aef092b1c..349449a2040 100644 --- a/core/includes/common.inc +++ b/core/includes/common.inc @@ -887,7 +887,7 @@ function l($text, $path, array $options = array()) { // Add a "data-drupal-link-system-path" attribute to let the // drupal.active-link library know the path in a standardized manner. if (!isset($variables['options']['attributes']['data-drupal-link-system-path'])) { - $variables['options']['attributes']['data-drupal-link-system-path'] = \Drupal::service('path.alias_manager.cached')->getPathByAlias($path); + $variables['options']['attributes']['data-drupal-link-system-path'] = \Drupal::service('path.alias_manager')->getPathByAlias($path); } } diff --git a/core/includes/theme.inc b/core/includes/theme.inc index e44f13d168d..7a1f7fe541b 100644 --- a/core/includes/theme.inc +++ b/core/includes/theme.inc @@ -1233,7 +1233,7 @@ function template_preprocess_links(&$variables) { // Add a "data-drupal-link-system-path" attribute to let the // drupal.active-link library know the path in a standardized manner. - $li_attributes['data-drupal-link-system-path'] = \Drupal::service('path.alias_manager.cached')->getPathByAlias($path); + $li_attributes['data-drupal-link-system-path'] = \Drupal::service('path.alias_manager')->getPathByAlias($path); } $item['link'] = $link_element; diff --git a/core/lib/Drupal/Core/CacheDecorator/AliasManagerCacheDecorator.php b/core/lib/Drupal/Core/CacheDecorator/AliasManagerCacheDecorator.php deleted file mode 100644 index 8bcbc082ec2..00000000000 --- a/core/lib/Drupal/Core/CacheDecorator/AliasManagerCacheDecorator.php +++ /dev/null @@ -1,128 +0,0 @@ -aliasManager = $alias_manager; - $this->cache = $cache; - } - - /** - * {@inheritdoc} - */ - public function setCacheKey($key) { - $this->cacheKey = $key; - } - - /** - * {@inheritdoc} - * - * Cache an array of the system paths available on each page. We assume - * that aliases will be needed for the majority of these paths during - * subsequent requests, and load them in a single query during path alias - * lookup. - */ - public function writeCache() { - $path_lookups = $this->getPathLookups(); - // Check if the system paths for this page were loaded from cache in this - // request to avoid writing to cache on every request. - if ($this->cacheNeedsWriting && !empty($path_lookups) && !empty($this->cacheKey)) { - // Set the path cache to expire in 24 hours. - $expire = REQUEST_TIME + (60 * 60 * 24); - $this->cache->set($this->cacheKey, $path_lookups, $expire); - } - } - - /** - * {@inheritdoc} - */ - public function getPathByAlias($alias, $langcode = NULL) { - $path = $this->aliasManager->getPathByAlias($alias, $langcode); - // We need to pass on the list of previously cached system paths for this - // key to the alias manager for use in subsequent lookups. - $cached = $this->cache->get($path); - $cached_paths = array(); - if ($cached) { - $cached_paths = $cached->data; - $this->cacheNeedsWriting = FALSE; - } - $this->preloadPathLookups($cached_paths); - return $path; - } - - /** - * {@inheritdoc} - */ - public function getAliasByPath($path, $langcode = NULL) { - return $this->aliasManager->getAliasByPath($path, $langcode); - } - - /** - * {@inheritdoc} - */ - public function getPathLookups() { - return $this->aliasManager->getPathLookups(); - } - - /** - * {@inheritdoc} - */ - public function preloadPathLookups(array $path_list) { - $this->aliasManager->preloadPathLookups($path_list); - } - - /** - * {@inheritdoc} - */ - public function cacheClear($source = NULL) { - $this->cache->delete($this->cacheKey); - $this->aliasManager->cacheClear($source); - } -} diff --git a/core/lib/Drupal/Core/Path/AliasManager.php b/core/lib/Drupal/Core/Path/AliasManager.php index 55d2ddc12cc..e3b686b07ce 100644 --- a/core/lib/Drupal/Core/Path/AliasManager.php +++ b/core/lib/Drupal/Core/Path/AliasManager.php @@ -7,10 +7,13 @@ namespace Drupal\Core\Path; +use Drupal\Core\Cache\CacheBackendInterface; +use Drupal\Core\CacheDecorator\CacheDecoratorInterface; use Drupal\Core\Language\Language; use Drupal\Core\Language\LanguageManager; +use Drupal\Core\Language\LanguageManagerInterface; -class AliasManager implements AliasManagerInterface { +class AliasManager implements AliasManagerInterface, CacheDecoratorInterface { /** * The alias storage service. @@ -19,6 +22,27 @@ class AliasManager implements AliasManagerInterface { */ protected $storage; + /** + * Cache backend service. + * + * @var \Drupal\Core\Cache\CacheBackendInterface; + */ + protected $cache; + + /** + * The cache key to use when caching paths. + * + * @var string + */ + protected $cacheKey; + + /** + * Whether the cache needs to be written. + * + * @var boolean + */ + protected $cacheNeedsWriting = FALSE; + /** * Language manager for retrieving the default langcode when none is specified. * @@ -64,12 +88,12 @@ class AliasManager implements AliasManagerInterface { /** * Holds an array of previously looked up paths for the current request path. * - * This will only ever get populated if the alias manager is being used in - * the context of a request. + * This will only get populated if a cache key has been set, which for example + * happens if the alias manager is used in the context of a request. * * @var array */ - protected $preloadedPathLookups = array(); + protected $preloadedPathLookups = FALSE; /** * Constructs an AliasManager. @@ -78,13 +102,52 @@ class AliasManager implements AliasManagerInterface { * The alias storage service. * @param \Drupal\Core\Path\AliasWhitelistInterface $whitelist * The whitelist implementation to use. - * @param \Drupal\Core\Language\LanguageManager $language_manager + * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager * The language manager. + * @param \Drupal\Core\Cache\CacheBackendInterface $cache + * Cache backend. */ - public function __construct(AliasStorageInterface $storage, AliasWhitelistInterface $whitelist, LanguageManager $language_manager) { + public function __construct(AliasStorageInterface $storage, AliasWhitelistInterface $whitelist, LanguageManagerInterface $language_manager, CacheBackendInterface $cache) { $this->storage = $storage; $this->languageManager = $language_manager; $this->whitelist = $whitelist; + $this->cache = $cache; + } + + /** + * {@inheritdoc} + */ + public function setCacheKey($key) { + // Prefix the cache key to avoid clashes with other caches. + $this->cacheKey = 'preload-paths:' . $key; + } + + /** + * {@inheritdoc} + * + * Cache an array of the paths available on each page. We assume that aliases + * will be needed for the majority of these paths during subsequent requests, + * and load them in a single query during path alias lookup. + */ + public function writeCache() { + // Check if the paths for this page were loaded from cache in this request + // to avoid writing to cache on every request. + if ($this->cacheNeedsWriting && !empty($this->cacheKey)) { + // Start with the preloaded path lookups, so that cached entries for other + // languages will not be lost. + $path_lookups_lookups = $this->preloadedPathLookups ?: array(); + foreach ($this->lookupMap as $langcode => $lookups) { + $path_lookups[$langcode] = array_keys($lookups); + if (!empty($this->noAlias[$langcode])) { + $path_lookups[$langcode] = array_merge($path_lookups[$langcode], array_keys($this->noAlias[$langcode])); + } + } + + if (!empty($path_lookups)) { + $twenty_four_hours_four_hours = 60 * 60 * 24; + $this->cache->set($this->cacheKey, $path_lookups, REQUEST_TIME + $twenty_four_hours); + } + } } /** @@ -110,12 +173,14 @@ class AliasManager implements AliasManagerInterface { // Look for path in storage. if ($path = $this->storage->lookupPathSource($alias, $langcode)) { $this->lookupMap[$langcode][$path] = $alias; + $this->cacheNeedsWriting = TRUE; return $path; } // We can't record anything into $this->lookupMap because we didn't find any // paths for this alias. Thus cache to $this->noPath. $this->noPath[$langcode][$alias] = TRUE; + return $alias; } @@ -141,11 +206,21 @@ class AliasManager implements AliasManagerInterface { if (empty($this->langcodePreloaded[$langcode])) { $this->langcodePreloaded[$langcode] = TRUE; $this->lookupMap[$langcode] = array(); + + // Load the cached paths that should be used for preloading. This only + // happens if a cache key has been set. + if ($this->preloadedPathLookups === FALSE) { + $this->preloadedPathLookups = array(); + if ($this->cacheKey && $cached = $this->cache->get($this->cacheKey)) { + $this->preloadedPathLookups = $cached->data; + } + } + // Load paths from cache. - if (!empty($this->preloadedPathLookups)) { - $this->lookupMap[$langcode] = $this->storage->preloadPathAlias($this->preloadedPathLookups, $langcode); + if (!empty($this->preloadedPathLookups[$langcode])) { + $this->lookupMap[$langcode] = $this->storage->preloadPathAlias($this->preloadedPathLookups[$langcode], $langcode); // Keep a record of paths with no alias to avoid querying twice. - $this->noAlias[$langcode] = array_flip(array_diff_key($this->preloadedPathLookups, array_keys($this->lookupMap[$langcode]))); + $this->noAlias[$langcode] = array_flip(array_diff_key($this->preloadedPathLookups[$langcode], array_keys($this->lookupMap[$langcode]))); } } @@ -162,12 +237,14 @@ class AliasManager implements AliasManagerInterface { // Try to load alias from storage. if ($alias = $this->storage->lookupPathAlias($path, $langcode)) { $this->lookupMap[$langcode][$path] = $alias; + $this->cacheNeedsWriting = TRUE; return $alias; } // We can't record anything into $this->lookupMap because we didn't find any // aliases for this path. Thus cache to $this->noAlias. $this->noAlias[$langcode][$path] = TRUE; + $this->cacheNeedsWriting = TRUE; return $path; } @@ -187,41 +264,24 @@ class AliasManager implements AliasManagerInterface { $this->noAlias = array(); $this->langcodePreloaded = array(); $this->preloadedPathLookups = array(); + $this->cache->delete($this->cacheKey); $this->pathAliasWhitelistRebuild($source); } - /** - * Implements \Drupal\Core\Path\AliasManagerInterface::getPathLookups(). - */ - public function getPathLookups() { - $current = current($this->lookupMap); - if ($current) { - return array_keys($current); - } - return array(); - } - - /** - * Implements \Drupal\Core\Path\AliasManagerInterface::preloadPathLookups(). - */ - public function preloadPathLookups(array $path_list) { - $this->preloadedPathLookups = $path_list; - } - /** * Rebuild the path alias white list. * - * @param $source - * An optional system path for which an alias is being inserted. + * @param string $path + * An optional path for which an alias is being inserted. * * @return * An array containing a white list of path aliases. */ - 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)) { - if ($this->whitelist->get(strtok($source, '/'))) { + protected function pathAliasWhitelistRebuild($path = NULL) { + // When paths are inserted, only rebuild the whitelist if the path has a top + // level component which is not already in the whitelist. + if (!empty($path)) { + if ($this->whitelist->get(strtok($path, '/'))) { return; } } diff --git a/core/lib/Drupal/Core/Path/AliasManagerInterface.php b/core/lib/Drupal/Core/Path/AliasManagerInterface.php index d37db972c07..2338846baf1 100644 --- a/core/lib/Drupal/Core/Path/AliasManagerInterface.php +++ b/core/lib/Drupal/Core/Path/AliasManagerInterface.php @@ -35,23 +35,6 @@ interface AliasManagerInterface { */ public function getAliasByPath($path, $langcode = NULL); - /** - * Returns an array of system paths that have been looked up. - * - * @return array - * An array of all system paths that have been looked up during the current - * request. - */ - public function getPathLookups(); - - /** - * Preload a set of paths for bulk alias lookups. - * - * @param $path_list - * An array of system paths. - */ - public function preloadPathLookups(array $path_list); - /** * Clear internal caches in alias manager. * diff --git a/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php index 6550a0e3938..ad749fbb3a1 100644 --- a/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php @@ -210,7 +210,7 @@ class LinkWidget extends WidgetBase { // If internal links are supported, look up whether the given value is // a path alias and store the system path instead. if ($this->supportsInternalLinks() && !UrlHelper::isExternal($value['url'])) { - $parsed_url['path'] = \Drupal::service('path.alias_manager.cached')->getPathByAlias($parsed_url['path']); + $parsed_url['path'] = \Drupal::service('path.alias_manager')->getPathByAlias($parsed_url['path']); } $url = Url::createFromPath($parsed_url['path']); diff --git a/core/modules/menu_link/src/MenuLinkForm.php b/core/modules/menu_link/src/MenuLinkForm.php index c365dae1cc8..d428734aec8 100644 --- a/core/modules/menu_link/src/MenuLinkForm.php +++ b/core/modules/menu_link/src/MenuLinkForm.php @@ -62,7 +62,7 @@ class MenuLinkForm extends EntityForm { public static function create(ContainerInterface $container) { return new static( $container->get('entity.manager')->getStorage('menu_link'), - $container->get('path.alias_manager.cached'), + $container->get('path.alias_manager'), $container->get('url_generator') ); } diff --git a/core/modules/path/src/Tests/PathAliasTest.php b/core/modules/path/src/Tests/PathAliasTest.php index f99ff786ff3..db01fa9113c 100644 --- a/core/modules/path/src/Tests/PathAliasTest.php +++ b/core/modules/path/src/Tests/PathAliasTest.php @@ -58,12 +58,12 @@ class PathAliasTest extends PathTestBase { \Drupal::cache('data')->deleteAll(); // Make sure the path is not converted to the alias. $this->drupalGet($edit['source'], array('alias' => TRUE)); - $this->assertTrue(\Drupal::cache('data')->get($edit['source']), 'Cache entry was created.'); + $this->assertTrue(\Drupal::cache('data')->get('preload-paths:' . $edit['source']), 'Cache entry was created.'); // Visit the alias for the node and confirm a cache entry is created. \Drupal::cache('data')->deleteAll(); $this->drupalGet($edit['alias']); - $this->assertTrue(\Drupal::cache('data')->get($edit['source']), 'Cache entry was created.'); + $this->assertTrue(\Drupal::cache('data')->get('preload-paths:' . $edit['source']), 'Cache entry was created.'); } /** diff --git a/core/modules/system/src/Tests/Path/AliasTest.php b/core/modules/system/src/Tests/Path/AliasTest.php index 0baae455b82..d118b2e62b5 100644 --- a/core/modules/system/src/Tests/Path/AliasTest.php +++ b/core/modules/system/src/Tests/Path/AliasTest.php @@ -84,7 +84,7 @@ class AliasTest extends PathUnitTestBase { $this->fixtures->createTables($connection); //Create AliasManager and Path object. - $aliasManager = $this->container->get('path.alias_manager.cached'); + $aliasManager = $this->container->get('path.alias_manager'); $aliasStorage = new AliasStorage($connection, $this->container->get('module_handler')); // Test the situation where the source is the same for multiple aliases. @@ -106,7 +106,7 @@ class AliasTest extends PathUnitTestBase { ); $aliasStorage->save($path['source'], $path['alias'], $path['langcode']); // Hook that clears cache is not executed with unit tests. - \Drupal::service('path.alias_manager.cached')->cacheClear(); + \Drupal::service('path.alias_manager')->cacheClear(); $this->assertEqual($aliasManager->getAliasByPath($path['source']), $path['alias'], 'English alias overrides language-neutral alias.'); $this->assertEqual($aliasManager->getPathByAlias($path['alias']), $path['source'], 'English source overrides language-neutral source.'); @@ -170,7 +170,7 @@ class AliasTest extends PathUnitTestBase { // Create AliasManager and Path object. $aliasStorage = new AliasStorage($connection, $this->container->get('module_handler')); $whitelist = new AliasWhitelist('path_alias_whitelist', $memoryCounterBackend, $this->container->get('lock'), $this->container->get('state'), $aliasStorage); - $aliasManager = new AliasManager($aliasStorage, $whitelist, $this->container->get('language_manager')); + $aliasManager = new AliasManager($aliasStorage, $whitelist, $this->container->get('language_manager'), $memoryCounterBackend); // No alias for user and admin yet, so should be NULL. $this->assertNull($whitelist->get('user')); diff --git a/core/modules/system/src/Tests/Routing/MockAliasManager.php b/core/modules/system/src/Tests/Routing/MockAliasManager.php index 53370999ae9..6c3839d2304 100644 --- a/core/modules/system/src/Tests/Routing/MockAliasManager.php +++ b/core/modules/system/src/Tests/Routing/MockAliasManager.php @@ -79,20 +79,6 @@ class MockAliasManager implements AliasManagerInterface { return $this->aliases[$path][$langcode]; } - /** - * {@inheritdoc} - */ - public function getPathLookups() { - return array_keys($this->lookedUp); - } - - /** - * {@inheritdoc} - */ - public function preloadPathLookups(array $path_list) { - // Not needed. - } - /** * {@inheritdoc} */ diff --git a/core/modules/system/system.module b/core/modules/system/system.module index 8feb47b0a6f..abf83320735 100644 --- a/core/modules/system/system.module +++ b/core/modules/system/system.module @@ -1717,19 +1717,19 @@ function theme_system_config_form($variables) { * Implements hook_path_update(). */ function system_path_update() { - \Drupal::service('path.alias_manager.cached')->cacheClear(); + \Drupal::service('path.alias_manager')->cacheClear(); } /** * Implements hook_path_insert(). */ function system_path_insert() { - \Drupal::service('path.alias_manager.cached')->cacheClear(); + \Drupal::service('path.alias_manager')->cacheClear(); } /** * Implements hook_path_delete(). */ function system_path_delete($path) { - \Drupal::service('path.alias_manager.cached')->cacheClear(); + \Drupal::service('path.alias_manager')->cacheClear(); } diff --git a/core/modules/views/src/Plugin/views/argument_default/Raw.php b/core/modules/views/src/Plugin/views/argument_default/Raw.php index e7007fad70c..5fed0670566 100644 --- a/core/modules/views/src/Plugin/views/argument_default/Raw.php +++ b/core/modules/views/src/Plugin/views/argument_default/Raw.php @@ -56,7 +56,7 @@ class Raw extends ArgumentDefaultPluginBase { $configuration, $plugin_id, $plugin_definition, - $container->get('path.alias_manager.cached') + $container->get('path.alias_manager') ); } diff --git a/core/modules/views_ui/views_ui.module b/core/modules/views_ui/views_ui.module index a39fa71c376..ba5e342ba24 100644 --- a/core/modules/views_ui/views_ui.module +++ b/core/modules/views_ui/views_ui.module @@ -320,7 +320,7 @@ function views_ui_views_analyze(ViewExecutable $view) { continue; } if ($display->hasPath() && $path = $display->getOption('path')) { - $normal_path = \Drupal::service('path.alias_manager.cached')->getPathByAlias($path); + $normal_path = \Drupal::service('path.alias_manager')->getPathByAlias($path); if ($path != $normal_path) { $ret[] = Analyzer::formatMessage(t('You have configured display %display with a path which is an path alias as well. This might lead to unwanted effects so better use an internal path.', array('%display' => $display->display['display_title'])), 'warning'); }