Commit e8a5d9e4 authored by catch's avatar catch

Issue #2479363 by Wim Leers, dawehner: Cache MenuActiveTrail::getActiveIds()...

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
parent 13922dbd
...@@ -487,13 +487,15 @@ services: ...@@ -487,13 +487,15 @@ services:
arguments: ['@menu.tree_storage', '@menu_link.static.overrides', '@module_handler'] arguments: ['@menu.tree_storage', '@menu_link.static.overrides', '@module_handler']
menu.link_tree: menu.link_tree:
class: Drupal\Core\Menu\MenuLinkTree 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: menu.default_tree_manipulators:
class: Drupal\Core\Menu\DefaultMenuLinkTreeManipulators class: Drupal\Core\Menu\DefaultMenuLinkTreeManipulators
arguments: ['@access_manager', '@current_user', '@entity.query'] arguments: ['@access_manager', '@current_user', '@entity.query']
menu.active_trail: menu.active_trail:
class: Drupal\Core\Menu\MenuActiveTrail 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: menu.parent_form_selector:
class: Drupal\Core\Menu\MenuParentFormSelector class: Drupal\Core\Menu\MenuParentFormSelector
arguments: ['@menu.link_tree', '@entity.manager', '@string_translation'] arguments: ['@menu.link_tree', '@entity.manager', '@string_translation']
......
...@@ -7,6 +7,9 @@ ...@@ -7,6 +7,9 @@
namespace Drupal\Core\Menu; namespace Drupal\Core\Menu;
use Drupal\Core\Cache\CacheBackendInterface;
use Drupal\Core\Cache\CacheCollector;
use Drupal\Core\Lock\LockBackendInterface;
use Drupal\Core\Routing\RouteMatchInterface; use Drupal\Core\Routing\RouteMatchInterface;
/** /**
...@@ -15,7 +18,7 @@ ...@@ -15,7 +18,7 @@
* It uses the current route name and route parameters to compare with the ones * It uses the current route name and route parameters to compare with the ones
* of the menu links. * of the menu links.
*/ */
class MenuActiveTrail implements MenuActiveTrailInterface { class MenuActiveTrail extends CacheCollector implements MenuActiveTrailInterface {
/** /**
* The menu link plugin manager. * The menu link plugin manager.
...@@ -38,16 +41,66 @@ class MenuActiveTrail implements MenuActiveTrailInterface { ...@@ -38,16 +41,66 @@ class MenuActiveTrail implements MenuActiveTrailInterface {
* The menu link plugin manager. * The menu link plugin manager.
* @param \Drupal\Core\Routing\RouteMatchInterface $route_match * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
* A route match object for finding the active link. * 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->menuLinkManager = $menu_link_manager;
$this->routeMatch = $route_match; $this->routeMatch = $route_match;
} }
/** /**
* {@inheritdoc} * {@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) { 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. // Parent ids; used both as key and value to ensure uniqueness.
// We always want all the top-level links with parent == ''. // We always want all the top-level links with parent == ''.
$active_trail = array('' => ''); $active_trail = array('' => '');
......
...@@ -47,30 +47,6 @@ class MenuLinkTree implements MenuLinkTreeInterface { ...@@ -47,30 +47,6 @@ class MenuLinkTree implements MenuLinkTreeInterface {
*/ */
protected $controllerResolver; 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. * Constructs a \Drupal\Core\Menu\MenuLinkTree object.
* *
...@@ -84,53 +60,31 @@ class MenuLinkTree implements MenuLinkTreeInterface { ...@@ -84,53 +60,31 @@ class MenuLinkTree implements MenuLinkTreeInterface {
* The active menu trail service. * The active menu trail service.
* @param \Drupal\Core\Controller\ControllerResolverInterface $controller_resolver * @param \Drupal\Core\Controller\ControllerResolverInterface $controller_resolver
* The 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->treeStorage = $tree_storage;
$this->menuLinkManager = $menu_link_manager; $this->menuLinkManager = $menu_link_manager;
$this->routeProvider = $route_provider; $this->routeProvider = $route_provider;
$this->menuActiveTrail = $menu_active_trail; $this->menuActiveTrail = $menu_active_trail;
$this->controllerResolver = $controller_resolver; $this->controllerResolver = $controller_resolver;
// @todo Remove these two in https://www.drupal.org/node/1805054.
$this->cache = $cache;
$this->routeMatch = $route_match;
} }
/** /**
* {@inheritdoc} * {@inheritdoc}
*/ */
public function getCurrentRouteMenuTreeParameters($menu_name) { public function getCurrentRouteMenuTreeParameters($menu_name) {
$route_parameters = $this->routeMatch->getRawParameters()->all(); $active_trail = $this->menuActiveTrail->getActiveTrailIds($menu_name);
ksort($route_parameters);
$cid = 'current-route-parameters:' . $menu_name . ':route:' . $this->routeMatch->getRouteName() . ':route_parameters:' . serialize($route_parameters); $parameters = new MenuTreeParameters();
$parameters->setActiveTrail($active_trail)
if (!isset($this->cachedCurrentRouteParameters[$cid])) { // We want links in the active trail to be expanded.
$cache = $this->cache->get($cid); ->addExpandedParents($active_trail)
if ($cache && $cache->data) { // We marked the links in the active trail to be expanded, but we also
$parameters = $cache->data; // want their descendants that have the "expanded" flag enabled to be
} // expanded.
else { ->addExpandedParents($this->treeStorage->getExpanded($menu_name, $active_trail));
$active_trail = $this->menuActiveTrail->getActiveTrailIds($menu_name);
return $parameters;
$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];
} }
/** /**
......
...@@ -239,6 +239,12 @@ public function testConfigLevelDepth() { ...@@ -239,6 +239,12 @@ public function testConfigLevelDepth() {
$request->attributes->set(RouteObjectInterface::ROUTE_NAME, 'example3'); $request->attributes->set(RouteObjectInterface::ROUTE_NAME, 'example3');
$request->attributes->set(RouteObjectInterface::ROUTE_OBJECT, $route); $request->attributes->set(RouteObjectInterface::ROUTE_OBJECT, $route);
$this->container->get('request_stack')->push($request); $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 = [];
$active_trail_expectations['all'] = [ $active_trail_expectations['all'] = [
'test.example1' => [], 'test.example1' => [],
......
...@@ -11,6 +11,7 @@ ...@@ -11,6 +11,7 @@
use Drupal\Core\Routing\CurrentRouteMatch; use Drupal\Core\Routing\CurrentRouteMatch;
use Drupal\Tests\UnitTestCase; use Drupal\Tests\UnitTestCase;
use Symfony\Cmf\Component\Routing\RouteObjectInterface; use Symfony\Cmf\Component\Routing\RouteObjectInterface;
use Symfony\Component\DependencyInjection\Container;
use Symfony\Component\HttpFoundation\ParameterBag; use Symfony\Component\HttpFoundation\ParameterBag;
use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\HttpFoundation\RequestStack;
...@@ -53,6 +54,20 @@ class MenuActiveTrailTest extends UnitTestCase { ...@@ -53,6 +54,20 @@ class MenuActiveTrailTest extends UnitTestCase {
*/ */
protected $menuLinkManager; 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} * {@inheritdoc}
*/ */
...@@ -62,8 +77,14 @@ protected function setUp() { ...@@ -62,8 +77,14 @@ protected function setUp() {
$this->requestStack = new RequestStack(); $this->requestStack = new RequestStack();
$this->currentRouteMatch = new CurrentRouteMatch($this->requestStack); $this->currentRouteMatch = new CurrentRouteMatch($this->requestStack);
$this->menuLinkManager = $this->getMock('Drupal\Core\Menu\MenuLinkManagerInterface'); $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 ...@@ -148,12 +169,14 @@ public function testGetActiveTrailIds(Request $request, $links, $menu_name, $exp
$this->requestStack->push($request); $this->requestStack->push($request);
if ($links !== FALSE) { 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') ->method('loadLinksbyRoute')
->with('baby_llama') ->with('baby_llama')
->will($this->returnValue($links)); ->will($this->returnValue($links));
if ($expected_link !== NULL) { if ($expected_link !== NULL) {
$this->menuLinkManager->expects($this->once()) $this->menuLinkManager->expects($this->exactly(2))
->method('getParentIds') ->method('getParentIds')
->will($this->returnValueMap(array( ->will($this->returnValueMap(array(
array($expected_link->getPluginId(), $expected_trail_ids), array($expected_link->getPluginId(), $expected_trail_ids),
...@@ -161,7 +184,54 @@ public function testGetActiveTrailIds(Request $request, $links, $menu_name, $exp ...@@ -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->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();
} }
} }
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