From e8a5d9e4e0fd0e05da47216b10ee4e6a92555008 Mon Sep 17 00:00:00 2001
From: Nathaniel Catchpole <catch@35733.no-reply.drupal.org>
Date: Mon, 4 May 2015 11:21:13 +0100
Subject: [PATCH] Issue #2479363 by Wim Leers, dawehner: Cache
MenuActiveTrail::getActiveIds() for *all* menus per route match: 1 cache get
instead of N DB queries, saves 1 ms/response
---
core/core.services.yml | 6 +-
core/lib/Drupal/Core/Menu/MenuActiveTrail.php | 57 +++++++++++++-
core/lib/Drupal/Core/Menu/MenuLinkTree.php | 72 ++++--------------
.../src/Tests/Block/SystemMenuBlockTest.php | 6 ++
.../Tests/Core/Menu/MenuActiveTrailTest.php | 76 ++++++++++++++++++-
5 files changed, 151 insertions(+), 66 deletions(-)
diff --git a/core/core.services.yml b/core/core.services.yml
index 5959c4470693..891d0b777254 100644
--- a/core/core.services.yml
+++ b/core/core.services.yml
@@ -487,13 +487,15 @@ services:
arguments: ['@menu.tree_storage', '@menu_link.static.overrides', '@module_handler']
menu.link_tree:
class: Drupal\Core\Menu\MenuLinkTree
- arguments: ['@menu.tree_storage', '@plugin.manager.menu.link', '@router.route_provider', '@menu.active_trail', '@controller_resolver', '@cache.menu', '@current_route_match']
+ arguments: ['@menu.tree_storage', '@plugin.manager.menu.link', '@router.route_provider', '@menu.active_trail', '@controller_resolver']
menu.default_tree_manipulators:
class: Drupal\Core\Menu\DefaultMenuLinkTreeManipulators
arguments: ['@access_manager', '@current_user', '@entity.query']
menu.active_trail:
class: Drupal\Core\Menu\MenuActiveTrail
- arguments: ['@plugin.manager.menu.link', '@current_route_match']
+ arguments: ['@plugin.manager.menu.link', '@current_route_match', '@cache.menu', '@lock']
+ tags:
+ - { name: needs_destruction }
menu.parent_form_selector:
class: Drupal\Core\Menu\MenuParentFormSelector
arguments: ['@menu.link_tree', '@entity.manager', '@string_translation']
diff --git a/core/lib/Drupal/Core/Menu/MenuActiveTrail.php b/core/lib/Drupal/Core/Menu/MenuActiveTrail.php
index 6247b29ee13f..2acb9723f8ca 100644
--- a/core/lib/Drupal/Core/Menu/MenuActiveTrail.php
+++ b/core/lib/Drupal/Core/Menu/MenuActiveTrail.php
@@ -7,6 +7,9 @@
namespace Drupal\Core\Menu;
+use Drupal\Core\Cache\CacheBackendInterface;
+use Drupal\Core\Cache\CacheCollector;
+use Drupal\Core\Lock\LockBackendInterface;
use Drupal\Core\Routing\RouteMatchInterface;
/**
@@ -15,7 +18,7 @@
* It uses the current route name and route parameters to compare with the ones
* of the menu links.
*/
-class MenuActiveTrail implements MenuActiveTrailInterface {
+class MenuActiveTrail extends CacheCollector implements MenuActiveTrailInterface {
/**
* The menu link plugin manager.
@@ -38,16 +41,66 @@ class MenuActiveTrail implements MenuActiveTrailInterface {
* The menu link plugin manager.
* @param \Drupal\Core\Routing\RouteMatchInterface $route_match
* A route match object for finding the active link.
+ * @param \Drupal\Core\Cache\CacheBackendInterface $cache
+ * The cache backend.
+ * @param \Drupal\Core\Lock\LockBackendInterface $lock
+ * The lock backend.
*/
- public function __construct(MenuLinkManagerInterface $menu_link_manager, RouteMatchInterface $route_match) {
+ public function __construct(MenuLinkManagerInterface $menu_link_manager, RouteMatchInterface $route_match, CacheBackendInterface $cache, LockBackendInterface $lock) {
+ parent::__construct(NULL, $cache, $lock);
$this->menuLinkManager = $menu_link_manager;
$this->routeMatch = $route_match;
}
/**
* {@inheritdoc}
+ *
+ * @see ::getActiveTrailIds()
+ */
+ protected function getCid() {
+ if (!isset($this->cid)) {
+ $route_parameters = $this->routeMatch->getRawParameters()->all();
+ ksort($route_parameters);
+ return 'active-trail:route:' . $this->routeMatch->getRouteName() . ':route_parameters:' . serialize($route_parameters);
+ }
+
+ return $this->cid;
+ }
+
+ /**
+ * {@inheritdoc}
+ *
+ * @see ::getActiveTrailIds()
+ */
+ protected function resolveCacheMiss($menu_name) {
+ $this->storage[$menu_name] = $this->doGetActiveTrailIds($menu_name);
+ $this->tags[] = 'config:system.menu.' . $menu_name;
+ $this->persist($menu_name);
+
+ return $this->storage[$menu_name];
+ }
+
+ /**
+ * {@inheritdoc}
+ *
+ * This implementation caches all active trail IDs per route match for *all*
+ * menus whose active trails are calculated on that page. This ensures 1 cache
+ * get for all active trails per page load, rather than N.
+ *
+ * It uses the cache collector pattern to do this.
+ *
+ * @see ::get()
+ * @see \Drupal\Core\Cache\CacheCollectorInterface
+ * @see \Drupal\Core\Cache\CacheCollector
*/
public function getActiveTrailIds($menu_name) {
+ return $this->get($menu_name);
+ }
+
+ /**
+ * Helper method for ::getActiveTrailIds().
+ */
+ protected function doGetActiveTrailIds($menu_name) {
// Parent ids; used both as key and value to ensure uniqueness.
// We always want all the top-level links with parent == ''.
$active_trail = array('' => '');
diff --git a/core/lib/Drupal/Core/Menu/MenuLinkTree.php b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
index 40dfbbe54c95..44eed04456c5 100644
--- a/core/lib/Drupal/Core/Menu/MenuLinkTree.php
+++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
@@ -47,30 +47,6 @@ class MenuLinkTree implements MenuLinkTreeInterface {
*/
protected $controllerResolver;
- /**
- * The cache backend.
- *
- * @var \Drupal\Core\Cache\CacheBackendInterface
- */
- protected $cache;
-
- /**
- * The current route match.
- *
- * @var \Drupal\Core\Routing\RouteMatchInterface
- */
- protected $routeMatch;
-
- /**
- * Stores the cached current route parameters by menu and current route match.
- *
- * @todo Remove this non-static caching in
- * https://www.drupal.org/node/1805054.
- *
- * @var \Drupal\Core\Menu\MenuTreeParameters[]
- */
- protected $cachedCurrentRouteParameters;
-
/**
* Constructs a \Drupal\Core\Menu\MenuLinkTree object.
*
@@ -84,53 +60,31 @@ class MenuLinkTree implements MenuLinkTreeInterface {
* The active menu trail service.
* @param \Drupal\Core\Controller\ControllerResolverInterface $controller_resolver
* The controller resolver.
- * @param \Drupal\Core\Cache\CacheBackendInterface $cache
- * The cache backend.
- * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
- * The current route match.
*/
- public function __construct(MenuTreeStorageInterface $tree_storage, MenuLinkManagerInterface $menu_link_manager, RouteProviderInterface $route_provider, MenuActiveTrailInterface $menu_active_trail, ControllerResolverInterface $controller_resolver, CacheBackendInterface $cache, RouteMatchInterface $route_match) {
+ public function __construct(MenuTreeStorageInterface $tree_storage, MenuLinkManagerInterface $menu_link_manager, RouteProviderInterface $route_provider, MenuActiveTrailInterface $menu_active_trail, ControllerResolverInterface $controller_resolver) {
$this->treeStorage = $tree_storage;
$this->menuLinkManager = $menu_link_manager;
$this->routeProvider = $route_provider;
$this->menuActiveTrail = $menu_active_trail;
$this->controllerResolver = $controller_resolver;
- // @todo Remove these two in https://www.drupal.org/node/1805054.
- $this->cache = $cache;
- $this->routeMatch = $route_match;
}
/**
* {@inheritdoc}
*/
public function getCurrentRouteMenuTreeParameters($menu_name) {
- $route_parameters = $this->routeMatch->getRawParameters()->all();
- ksort($route_parameters);
- $cid = 'current-route-parameters:' . $menu_name . ':route:' . $this->routeMatch->getRouteName() . ':route_parameters:' . serialize($route_parameters);
-
- if (!isset($this->cachedCurrentRouteParameters[$cid])) {
- $cache = $this->cache->get($cid);
- if ($cache && $cache->data) {
- $parameters = $cache->data;
- }
- else {
- $active_trail = $this->menuActiveTrail->getActiveTrailIds($menu_name);
-
- $parameters = new MenuTreeParameters();
- $parameters->setActiveTrail($active_trail)
- // We want links in the active trail to be expanded.
- ->addExpandedParents($active_trail)
- // We marked the links in the active trail to be expanded, but we also
- // want their descendants that have the "expanded" flag enabled to be
- // expanded.
- ->addExpandedParents($this->treeStorage->getExpanded($menu_name, $active_trail));
-
- $this->cache->set($cid, $parameters, CacheBackendInterface::CACHE_PERMANENT, array('config:system.menu.' . $menu_name));
- }
- $this->cachedCurrentRouteParameters[$menu_name] = $parameters;
- }
-
- return $this->cachedCurrentRouteParameters[$menu_name];
+ $active_trail = $this->menuActiveTrail->getActiveTrailIds($menu_name);
+
+ $parameters = new MenuTreeParameters();
+ $parameters->setActiveTrail($active_trail)
+ // We want links in the active trail to be expanded.
+ ->addExpandedParents($active_trail)
+ // We marked the links in the active trail to be expanded, but we also
+ // want their descendants that have the "expanded" flag enabled to be
+ // expanded.
+ ->addExpandedParents($this->treeStorage->getExpanded($menu_name, $active_trail));
+
+ return $parameters;
}
/**
diff --git a/core/modules/system/src/Tests/Block/SystemMenuBlockTest.php b/core/modules/system/src/Tests/Block/SystemMenuBlockTest.php
index 0361eb781903..c053295d3f6d 100644
--- a/core/modules/system/src/Tests/Block/SystemMenuBlockTest.php
+++ b/core/modules/system/src/Tests/Block/SystemMenuBlockTest.php
@@ -239,6 +239,12 @@ public function testConfigLevelDepth() {
$request->attributes->set(RouteObjectInterface::ROUTE_NAME, 'example3');
$request->attributes->set(RouteObjectInterface::ROUTE_OBJECT, $route);
$this->container->get('request_stack')->push($request);
+ // \Drupal\Core\Menu\MenuActiveTrail uses the cache collector pattern, which
+ // includes static caching. Since this second scenario simulates a second
+ // request, we must also simulate it for the MenuActiveTrail service, by
+ // clearing the cache collector's static cache.
+ \Drupal::service('menu.active_trail')->clear();
+
$active_trail_expectations = [];
$active_trail_expectations['all'] = [
'test.example1' => [],
diff --git a/core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php b/core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php
index 2ea72ff6ace3..495d29806e32 100644
--- a/core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php
+++ b/core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php
@@ -11,6 +11,7 @@
use Drupal\Core\Routing\CurrentRouteMatch;
use Drupal\Tests\UnitTestCase;
use Symfony\Cmf\Component\Routing\RouteObjectInterface;
+use Symfony\Component\DependencyInjection\Container;
use Symfony\Component\HttpFoundation\ParameterBag;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;
@@ -53,6 +54,20 @@ class MenuActiveTrailTest extends UnitTestCase {
*/
protected $menuLinkManager;
+ /**
+ * The mocked cache backend.
+ *
+ * @var \Drupal\Core\Cache\CacheBackendInterface|\PHPUnit_Framework_MockObject_MockObject
+ */
+ protected $cache;
+
+ /**
+ * The mocked lock.
+ *
+ * @var \Drupal\Core\Lock\LockBackendInterface|\PHPUnit_Framework_MockObject_MockObject
+ */
+ protected $lock;
+
/**
* {@inheritdoc}
*/
@@ -62,8 +77,14 @@ protected function setUp() {
$this->requestStack = new RequestStack();
$this->currentRouteMatch = new CurrentRouteMatch($this->requestStack);
$this->menuLinkManager = $this->getMock('Drupal\Core\Menu\MenuLinkManagerInterface');
+ $this->cache = $this->getMock('\Drupal\Core\Cache\CacheBackendInterface');
+ $this->lock = $this->getMock('\Drupal\Core\Lock\LockBackendInterface');
+
+ $this->menuActiveTrail = new MenuActiveTrail($this->menuLinkManager, $this->currentRouteMatch, $this->cache, $this->lock);
- $this->menuActiveTrail = new MenuActiveTrail($this->menuLinkManager, $this->currentRouteMatch);
+ $container = new Container();
+ $container->set('cache_tags.invalidator', $this->getMock('\Drupal\Core\Cache\CacheTagsInvalidatorInterface'));
+ \Drupal::setContainer($container);
}
/**
@@ -148,12 +169,14 @@ public function testGetActiveTrailIds(Request $request, $links, $menu_name, $exp
$this->requestStack->push($request);
if ($links !== FALSE) {
- $this->menuLinkManager->expects($this->once())
+ // We expect exactly two calls, one for the first call, and one after the
+ // cache clearing below.
+ $this->menuLinkManager->expects($this->exactly(2))
->method('loadLinksbyRoute')
->with('baby_llama')
->will($this->returnValue($links));
if ($expected_link !== NULL) {
- $this->menuLinkManager->expects($this->once())
+ $this->menuLinkManager->expects($this->exactly(2))
->method('getParentIds')
->will($this->returnValueMap(array(
array($expected_link->getPluginId(), $expected_trail_ids),
@@ -161,7 +184,54 @@ public function testGetActiveTrailIds(Request $request, $links, $menu_name, $exp
}
}
+ // Call out the same twice in order to ensure that static caching works.
+ $this->assertSame($expected_trail_ids, $this->menuActiveTrail->getActiveTrailIds($menu_name));
$this->assertSame($expected_trail_ids, $this->menuActiveTrail->getActiveTrailIds($menu_name));
+
+ $this->menuActiveTrail->clear();
+ $this->assertSame($expected_trail_ids, $this->menuActiveTrail->getActiveTrailIds($menu_name));
+ }
+
+ /**
+ * Tests getCid()
+ *
+ * @covers ::getCid
+ */
+ public function testGetCid() {
+ $data = $this->provider()[1];
+ /** @var \Symfony\Component\HttpFoundation\Request $request */
+ $request = $data[0];
+ /** @var \Symfony\Component\Routing\Route $route */
+ $route = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT);
+ $route->setPath('/test/{b}/{a}');
+ $request->attributes->get('_raw_variables')->add(['b' => 1, 'a' => 0]);
+ $this->requestStack->push($request);
+
+ $this->menuLinkManager->expects($this->any())
+ ->method('loadLinksbyRoute')
+ ->with('baby_llama')
+ ->will($this->returnValue($data[1]));
+
+ $expected_link = $data[3];
+ $expected_trail = $data[4];
+ $expected_trail_ids = array_combine($expected_trail, $expected_trail);
+
+ $this->menuLinkManager->expects($this->any())
+ ->method('getParentIds')
+ ->will($this->returnValueMap(array(
+ array($expected_link->getPluginId(), $expected_trail_ids),
+ )));
+
+ $this->assertSame($expected_trail_ids, $this->menuActiveTrail->getActiveTrailIds($data[2]));
+
+ $this->cache->expects($this->once())
+ ->method('set')
+ // Ensure we normalize the serialized data by sorting them.
+ ->with('active-trail:route:baby_llama:route_parameters:' . serialize(['a' => 0, 'b' => 1]));
+ $this->lock->expects($this->any())
+ ->method('acquire')
+ ->willReturn(TRUE);
+ $this->menuActiveTrail->destruct();
}
}
--
GitLab