From 8866760fca31ea9080b55d2f6e21e5bc180c78ff Mon Sep 17 00:00:00 2001 From: Nathaniel Catchpole Date: Fri, 4 Apr 2014 12:16:47 +0100 Subject: [PATCH] Issue #2224861 by Wim Leers, dawehner, pwolanin: Cache SystemMenuBlock and BookNavigationBlock per active trail (currently cached per URL, this breaks on very long URLs). --- .../book/lib/Drupal/book/BookManager.php | 29 ++++--- .../lib/Drupal/book/BookManagerInterface.php | 13 +++ .../book/Plugin/Block/BookNavigationBlock.php | 24 +++++- .../lib/Drupal/menu_link/MenuTree.php | 66 +++++++++------ .../Drupal/menu_link/MenuTreeInterface.php | 11 +++ .../Drupal/menu_link/Tests/MenuTreeTest.php | 80 ++++++++++++++++++- .../system/Plugin/Block/SystemMenuBlock.php | 18 ++++- 7 files changed, 196 insertions(+), 45 deletions(-) diff --git a/core/modules/book/lib/Drupal/book/BookManager.php b/core/modules/book/lib/Drupal/book/BookManager.php index 04a16550ca5..68bbb1c9c59 100644 --- a/core/modules/book/lib/Drupal/book/BookManager.php +++ b/core/modules/book/lib/Drupal/book/BookManager.php @@ -488,16 +488,9 @@ class BookManager implements BookManagerInterface { 'max_depth' => $max_depth, ); if ($nid) { - // The tree is for a single item, so we need to match the values in its - // p columns and 0 (the top level) with the plid values of other links. - $parents = array(0); - for ($i = 1; $i < static::BOOK_MAX_DEPTH; $i++) { - if (!empty($link["p$i"])) { - $parents[] = $link["p$i"]; - } - } - $tree_parameters['expanded'] = $parents; - $tree_parameters['active_trail'] = $parents; + $active_trail = $this->getActiveTrailIds($bid, $link); + $tree_parameters['expanded'] = $active_trail; + $tree_parameters['active_trail'] = $active_trail; $tree_parameters['active_trail'][] = $nid; } @@ -508,6 +501,22 @@ class BookManager implements BookManagerInterface { return $tree[$cid]; } + /** + * {@inheritdoc} + */ + public function getActiveTrailIds($bid, $link) { + $nid = isset($link['nid']) ? $link['nid'] : 0; + // The tree is for a single item, so we need to match the values in its + // p columns and 0 (the top level) with the plid values of other links. + $active_trail = array(0); + for ($i = 1; $i < static::BOOK_MAX_DEPTH; $i++) { + if (!empty($link["p$i"])) { + $active_trail[] = $link["p$i"]; + } + } + return $active_trail; + } + /** * {@inheritdoc} */ diff --git a/core/modules/book/lib/Drupal/book/BookManagerInterface.php b/core/modules/book/lib/Drupal/book/BookManagerInterface.php index c096e68df3f..81dfe080d0e 100644 --- a/core/modules/book/lib/Drupal/book/BookManagerInterface.php +++ b/core/modules/book/lib/Drupal/book/BookManagerInterface.php @@ -40,6 +40,19 @@ interface BookManagerInterface { */ public function bookTreeAllData($bid, $link = NULL, $max_depth = NULL); + /** + * Gets the active trail IDs for the specified book at the provided path. + * + * @param string $bid + * The Book ID to find links for. + * @param array $link + * A fully loaded menu link. + * + * @return array + * An array containing the active trail: a list of mlids. + */ + public function getActiveTrailIds($bid, $link); + /** * Loads a single book entry. * diff --git a/core/modules/book/lib/Drupal/book/Plugin/Block/BookNavigationBlock.php b/core/modules/book/lib/Drupal/book/Plugin/Block/BookNavigationBlock.php index d98be3bccd8..c5d7842ec6e 100644 --- a/core/modules/book/lib/Drupal/book/Plugin/Block/BookNavigationBlock.php +++ b/core/modules/book/lib/Drupal/book/Plugin/Block/BookNavigationBlock.php @@ -164,14 +164,30 @@ class BookNavigationBlock extends BlockBase implements ContainerFactoryPluginInt return array(); } + /** + * {@inheritdoc} + */ + public function getCacheKeys() { + // Add a key for the active book trail. + $current_bid = 0; + if ($node = $this->request->get('node')) { + $current_bid = empty($node->book['bid']) ? 0 : $node->book['bid']; + } + if ($current_bid === 0) { + return parent::getCacheKeys(); + } + $active_trail = $this->bookManager->getActiveTrailIds($node->book['bid'], $node->book); + $active_trail_key = 'trail.' . implode('|', $active_trail); + return array_merge(parent::getCacheKeys(), array($active_trail_key)); + } + /** * {@inheritdoc} */ protected function getRequiredCacheContexts() { - // The "Book navigation" block must be cached per URL and per role: the - // "active" menu link may differ per URL and different roles may have access - // to different menu links. - return array('cache_context.url', 'cache_context.user.roles'); + // The "Book navigation" block must be cached per role: different roles may + // have access to different menu links. + return array('cache_context.user.roles'); } } diff --git a/core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php b/core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php index f12cae888d3..24a531f93a1 100644 --- a/core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php @@ -204,8 +204,6 @@ class MenuTree implements MenuTreeInterface { public function buildPageData($menu_name, $max_depth = NULL, $only_active_trail = FALSE) { $language_interface = $this->languageManager->getCurrentLanguage(); - // Check if the active trail has been overridden for this menu tree. - $active_path = $this->getPath($menu_name); // Load the request corresponding to the current page. $request = $this->requestStack->getCurrentRequest(); $system_path = NULL; @@ -247,34 +245,17 @@ class MenuTree implements MenuTreeInterface { 'min_depth' => 1, 'max_depth' => $max_depth, ); - // Parent mlids; used both as key and value to ensure uniqueness. - // We always want all the top-level links with plid == 0. - $active_trail = array(0 => 0); + $active_trail = $this->getActiveTrailIds($menu_name); // If this page is accessible to the current user, build the tree // parameters accordingly. if ($page_not_403) { - // Find a menu link corresponding to the current path. If - // $active_path is NULL, let menu_link_get_preferred() determine - // the path. - if ($active_link = $this->menuLinkGetPreferred($menu_name, $active_path)) { - // The active link may only be taken into account to build the - // active trail, if it resides in the requested menu. - // Otherwise, we'd needlessly re-run _menu_build_tree() queries - // for every menu on every page. - if ($active_link['menu_name'] == $menu_name) { - // Use all the coordinates, except the last one because - // there can be no child beyond the last column. - for ($i = 1; $i < MENU_MAX_DEPTH; $i++) { - if ($active_link['p' . $i]) { - $active_trail[$active_link['p' . $i]] = $active_link['p' . $i]; - } - } - // If we are asked to build links for the active trail only,skip - // the entire 'expanded' handling. - if ($only_active_trail) { - $tree_parameters['only_active_trail'] = TRUE; - } + // The active trail contains more than only array(0 => 0). + if (count($active_trail) > 1) { + // If we are asked to build links for the active trail only,skip + // the entire 'expanded' handling. + if ($only_active_trail) { + $tree_parameters['only_active_trail'] = TRUE; } } $parents = $active_trail; @@ -317,6 +298,39 @@ class MenuTree implements MenuTreeInterface { return array(); } + /** + * {@inheritdoc} + */ + public function getActiveTrailIds($menu_name) { + // Parent mlids; used both as key and value to ensure uniqueness. + // We always want all the top-level links with plid == 0. + $active_trail = array(0 => 0); + + $request = $this->requestStack->getCurrentRequest(); + + if ($route_name = $request->attributes->get(RouteObjectInterface::ROUTE_NAME)) { + // @todo https://drupal.org/node/2068471 is adding support so we can tell + // if this is called on a 404/403 page. + // Check if the active trail has been overridden for this menu tree. + $active_path = $this->getPath($menu_name); + // Find a menu link corresponding to the current path. If + // $active_path is NULL, let menu_link_get_preferred() determine + // the path. + if ($active_link = $this->menuLinkGetPreferred($menu_name, $active_path)) { + if ($active_link['menu_name'] == $menu_name) { + // Use all the coordinates, except the last one because + // there can be no child beyond the last column. + for ($i = 1; $i < MENU_MAX_DEPTH; $i++) { + if ($active_link['p' . $i]) { + $active_trail[$active_link['p' . $i]] = $active_link['p' . $i]; + } + } + } + } + } + return $active_trail; + } + /** * {@inheritdoc} */ diff --git a/core/modules/menu_link/lib/Drupal/menu_link/MenuTreeInterface.php b/core/modules/menu_link/lib/Drupal/menu_link/MenuTreeInterface.php index e4cd83de3f0..6cb192f35c9 100644 --- a/core/modules/menu_link/lib/Drupal/menu_link/MenuTreeInterface.php +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTreeInterface.php @@ -59,6 +59,17 @@ interface MenuTreeInterface { */ public function getPath($menu_name); + /** + * Gets the active trail IDs of the specified menu tree. + * + * @param string $menu_name + * The menu name of the requested tree. + * + * @return array + * An array containing the active trail: a list of mlids. + */ + public function getActiveTrailIds($menu_name); + /** * Sorts and returns the built data representing a menu tree. * diff --git a/core/modules/menu_link/tests/Drupal/menu_link/Tests/MenuTreeTest.php b/core/modules/menu_link/tests/Drupal/menu_link/Tests/MenuTreeTest.php index 3b44b087cea..50bebc613c4 100644 --- a/core/modules/menu_link/tests/Drupal/menu_link/Tests/MenuTreeTest.php +++ b/core/modules/menu_link/tests/Drupal/menu_link/Tests/MenuTreeTest.php @@ -5,10 +5,12 @@ * Contains \Drupal\menu_link\Tests\MenuTreeTest. */ -namespace Drupal\menu_link\Tests; +namespace Drupal\menu_link\Tests { use Drupal\menu_link\MenuTree; use Drupal\Tests\UnitTestCase; +use Symfony\Cmf\Component\Routing\RouteObjectInterface; +use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\RequestStack; /** @@ -272,6 +274,52 @@ class MenuTreeTest extends UnitTestCase { $this->assertEquals($items[4]['mlid'], $tree['50000 2']['below']['50000 3']['below']['50000 4']['link']['mlid']); } + /** + * Tests getActiveTrailIds(). + * + * @covers ::getActiveTrailIds() + */ + public function testGetActiveTrailIds() { + $menu_link = array( + 'mlid' => 10, + 'route_name' => 'example1', + 'p1' => 3, + 'p2' => 2, + 'p3' => 1, + 'p4' => 4, + 'p5' => 9, + 'p6' => 5, + 'p7' => 6, + 'p8' => 7, + 'p9' => 8, + 'menu_name' => 'test_menu' + ); + $this->menuTree->setPreferredMenuLink('test_menu', 'test/path', $menu_link); + $request = (new Request()); + $request->attributes->set(RouteObjectInterface::ROUTE_NAME, 'test_route'); + $this->requestStack->push($request); + $this->menuTree->setPath('test_menu', 'test/path'); + + $trail = $this->menuTree->getActiveTrailIds('test_menu'); + $this->assertEquals(array(0 => 0, 3 => 3, 2 => 2, 1 => 1, 4 => 4, 9 => 9, 5 => 5, 6 => 6, 7 => 7), $trail); + } + + /** + * Tests getActiveTrailIds() without preferred link. + * + * @covers ::getActiveTrailIds() + */ + public function testGetActiveTrailIdsWithoutPreferredLink() { + $request = (new Request()); + $request->attributes->set(RouteObjectInterface::ROUTE_NAME, 'test_route'); + $this->requestStack->push($request); + $this->menuTree->setPath('test_menu', 'test/path'); + + $trail = $this->menuTree->getActiveTrailIds('test_menu'); + $this->assertEquals(array(0 => 0), $trail); + } + + /** * Tests the output with a single level. * @@ -369,6 +417,13 @@ class TestMenuTree extends MenuTree { */ public $menuLinkTranslateCallable; + /** + * Stores the preferred menu link per menu and path. + * + * @var array + */ + protected $preferredMenuLink; + /** * {@inheritdoc} */ @@ -382,6 +437,29 @@ class TestMenuTree extends MenuTree { * {@inheritdoc} */ protected function menuLinkGetPreferred($menu_name, $active_path) { + return isset($this->preferredMenuLink[$menu_name][$active_path]) ? $this->preferredMenuLink[$menu_name][$active_path] : NULL; + } + + /** + * Sets the preferred menu link. + * + * @param string $menu_name + * The menu name. + * @param string $active_path + * The active path. + * @param array $menu_link + * The preferred menu link. + */ + public function setPreferredMenuLink($menu_name, $active_path, $menu_link) { + $this->preferredMenuLink[$menu_name][$active_path] = $menu_link; } } + +} + +namespace { + if (!defined('MENU_MAX_DEPTH')) { + define('MENU_MAX_DEPTH', 9); + } +} diff --git a/core/modules/system/lib/Drupal/system/Plugin/Block/SystemMenuBlock.php b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemMenuBlock.php index 32a84d7cffa..14c0837e3d7 100644 --- a/core/modules/system/lib/Drupal/system/Plugin/Block/SystemMenuBlock.php +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemMenuBlock.php @@ -85,6 +85,17 @@ class SystemMenuBlock extends BlockBase implements ContainerFactoryPluginInterfa return array('cache' => array('max_age' => \Drupal\Core\Cache\Cache::PERMANENT)); } + /** + * {@inheritdoc} + */ + public function getCacheKeys() { + // Add a key for the active menu trail. + $menu = $this->getDerivativeId(); + $active_trail = $this->menuTree->getActiveTrailIds($menu); + $active_trail_key = 'trail.' . implode('|', $active_trail); + return array_merge(parent::getCacheKeys(), array($active_trail_key)); + } + /** * {@inheritdoc} */ @@ -101,10 +112,9 @@ class SystemMenuBlock extends BlockBase implements ContainerFactoryPluginInterfa * {@inheritdoc} */ protected function getRequiredCacheContexts() { - // Menu blocks must be cached per URL and per role: the "active" menu link - // may differ per URL and different roles may have access to different menu - // links. - return array('cache_context.url', 'cache_context.user.roles'); + // Menu blocks must be cached per role: different roles may have access to + // different menu links. + return array('cache_context.user.roles'); } }