.../Core/Menu/DefaultMenuLinkTreeManipulators.php | 31 ++++++----- core/lib/Drupal/Core/Menu/MenuLinkTree.php | 59 ++++++++++++++------ core/lib/Drupal/Core/Menu/MenuLinkTreeElement.php | 6 +- core/modules/block/src/BlockViewBuilder.php | 13 +++++ .../MenuLinkContentCacheabilityBubblingTest.php | 13 +++++ .../system/src/Plugin/Block/SystemMenuBlock.php | 8 ++- core/modules/toolbar/toolbar.module | 5 ++ .../Menu/DefaultMenuLinkTreeManipulatorsTest.php | 64 ++++++++++++---------- 8 files changed, 132 insertions(+), 67 deletions(-) diff --git a/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php index d22f538..a545996 100644 --- a/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php @@ -8,6 +8,7 @@ namespace Drupal\Core\Menu; use Drupal\Core\Access\AccessManagerInterface; +use Drupal\Core\Access\AccessResult; use Drupal\Core\Entity\Query\QueryFactory; use Drupal\Core\Session\AccountInterface; @@ -63,12 +64,13 @@ public function __construct(AccessManagerInterface $access_manager, AccountInter /** * Performs access checks of a menu tree. * - * Removes menu links from the given menu tree whose links are inaccessible - * for the current user, sets the 'access' property to TRUE on tree elements - * that are accessible for the current user. + * Sets the 'access' property to AccessResultInterface objects on menu link + * tree elements. Descends into subtrees if the root of the subtree is + * accessible. * - * Makes the resulting menu tree impossible to render cache, unless render - * caching per user is acceptable. + * This is compatible with render caching, because of cache context bubbling: + * conditionally defined cache contexts (i.e. subtrees that are only + * accessible to some users) will bubble just * * @param \Drupal\Core\Menu\MenuLinkTreeElement[] $tree * The menu link tree to manipulate. @@ -83,7 +85,7 @@ public function checkAccess(array $tree) { if (!isset($element->access)) { $tree[$key]->access = $this->menuLinkCheckAccess($element->link); } - if ($tree[$key]->access) { + if ($tree[$key]->access->isAllowed()) { if ($tree[$key]->subtree) { $tree[$key]->subtree = $this->checkAccess($tree[$key]->subtree); } @@ -120,17 +122,19 @@ public function checkNodeAccess(array $tree) { // query rewrite as well as not checking for the node status. The // 'view own unpublished nodes' permission is ignored to not require cache // entries per user. + $access_result = AccessResult::allowed()->cachePerPermissions(); if ($this->account->hasPermission('bypass node access')) { $query->accessCheck(FALSE); } else { + $access_result->addCacheContexts(['user.node_grants:view']); $query->condition('status', NODE_PUBLISHED); } $nids = $query->execute(); foreach ($nids as $nid) { foreach ($node_links[$nid] as $key => $link) { - $node_links[$nid][$key]->access = TRUE; + $node_links[$nid][$key]->access = $access_result; } } } @@ -155,7 +159,7 @@ protected function collectNodeLinks(array &$tree, array &$node_links) { $nid = $element->link->getRouteParameters()['node']; $node_links[$nid][$key] = $element; // Deny access by default. checkNodeAccess() will re-add it. - $element->access = FALSE; + $element->access = AccessResult::neutral(); } if ($element->hasChildren) { $this->collectNodeLinks($element->subtree, $node_links); @@ -169,24 +173,23 @@ protected function collectNodeLinks(array &$tree, array &$node_links) { * @param \Drupal\Core\Menu\MenuLinkInterface $instance * The menu link instance. * - * @return bool - * TRUE if the current user can access the link, FALSE otherwise. + * @return \Drupal\Core\Access\AccessResultInterface + * The access result. */ protected function menuLinkCheckAccess(MenuLinkInterface $instance) { if ($this->account->hasPermission('link to any page')) { - return TRUE; + return AccessResult::allowed()->cachePerPermissions(); } // Use the definition here since that's a lot faster than creating a Url // object that we don't need. $definition = $instance->getPluginDefinition(); // 'url' should only be populated for external links. if (!empty($definition['url']) && empty($definition['route_name'])) { - $access = TRUE; + return AccessResult::allowed(); } else { - $access = $this->accessManager->checkNamedRoute($definition['route_name'], $definition['route_parameters'], $this->account); + return $this->accessManager->checkNamedRoute($definition['route_name'], $definition['route_parameters'], $this->account, TRUE); } - return $access; } /** diff --git a/core/lib/Drupal/Core/Menu/MenuLinkTree.php b/core/lib/Drupal/Core/Menu/MenuLinkTree.php index 44eed04..6496133 100644 --- a/core/lib/Drupal/Core/Menu/MenuLinkTree.php +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php @@ -8,6 +8,7 @@ namespace Drupal\Core\Menu; use Drupal\Component\Utility\NestedArray; +use Drupal\Core\Cache\CacheableMetadata; use Drupal\Core\Cache\CacheBackendInterface; use Drupal\Core\Controller\ControllerResolverInterface; use Drupal\Core\Routing\RouteMatchInterface; @@ -154,8 +155,20 @@ public function transform(array $tree, array $manipulators) { */ public function build(array $tree, $level = 0) { $items = array(); + $tree_access_cacheability = new CacheableMetadata(); foreach ($tree as $data) { + // Gather the access cacheability of every item in the menu link tree, + // including inaccessible items. This allows us to render cache the menu + // tree, yet still automatically the rendered menu by the same cache + // contexts that the access results vary by. + $tree_access_cacheability = $tree_access_cacheability->merge(CacheableMetadata::createFromObject($data->access)); + + // Only render accessible links. + if (!$data->access->isAllowed()) { + continue; + } + $class = ['menu-item']; /** @var \Drupal\Core\Menu\MenuLinkInterface $link */ $link = $data->link; @@ -176,7 +189,8 @@ public function build(array $tree, $level = 0) { $class[] = 'menu-item--active-trail'; } - // Allow menu-specific theme overrides. + // Note: links are rendered in the menu.html.twig template; and they + // automatically bubble their associated cacheability metadata. $element = array(); $element['attributes'] = new Attribute(); $element['attributes']['class'] = $class; @@ -192,25 +206,34 @@ public function build(array $tree, $level = 0) { $items[$link->getPluginId()] = $element; } - if (!$items) { - return array(); - } - elseif ($level == 0) { - $build = array(); - // Make sure drupal_render() does not re-order the links. - $build['#sorted'] = TRUE; - // Get the menu name from the last link. - $menu_name = $link->getMenuName(); - // Add the theme wrapper for outer markup. - // Allow menu-specific theme overrides. - $build['#theme'] = 'menu__' . strtr($menu_name, '-', '_'); - $build['#items'] = $items; - // Set cache tag. - $build['#cache']['tags'][] = 'config:system.menu.' . $menu_name; - return $build; + // Non-root levels: return $items. + if ($level > 0) { + return $items; } + // Root level: return a render array. else { - return $items; + $build = []; + + // Apply the tree-wide gathered access cacheability metadata to the + // render array. This ensures that the rendered menu is varied by the + // cache contexts that the access results depended upon, and invalidated + // by the cache tags that may change the values of the access results. + $tree_access_cacheability->applyTo($build); + + if ($items) { + // Make sure drupal_render() does not re-order the links. + $build['#sorted'] = TRUE; + // Get the menu name from the last link. + $menu_name = $link->getMenuName(); + // Add the theme wrapper for outer markup. + // Allow menu-specific theme overrides. + $build['#theme'] = 'menu__' . strtr($menu_name, '-', '_'); + $build['#items'] = $items; + // Set cache tag. + $build['#cache']['tags'][] = 'config:system.menu.' . $menu_name; + } + + return $build; } } diff --git a/core/lib/Drupal/Core/Menu/MenuLinkTreeElement.php b/core/lib/Drupal/Core/Menu/MenuLinkTreeElement.php index a52b6c2..7358e4f 100644 --- a/core/lib/Drupal/Core/Menu/MenuLinkTreeElement.php +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeElement.php @@ -70,10 +70,10 @@ class MenuLinkTreeElement { /** * Whether this link is accessible by the current user. * - * If the value is NULL the access was not determined yet, if Boolean it was - * determined already. + * If the value is NULL the access was not determined yet, if an access result + * object, it was determined already. * - * @var bool|NULL + * @var \Drupal\Core\Access\AccessResultInterface|NULL */ public $access; diff --git a/core/modules/block/src/BlockViewBuilder.php b/core/modules/block/src/BlockViewBuilder.php index 216dfa8..80085ac 100644 --- a/core/modules/block/src/BlockViewBuilder.php +++ b/core/modules/block/src/BlockViewBuilder.php @@ -12,6 +12,7 @@ use Drupal\Core\Entity\EntityViewBuilder; use Drupal\Core\Entity\EntityViewBuilderInterface; use Drupal\Core\Entity\EntityInterface; +use Drupal\Core\Render\Element; /** * Provides a Block view builder. @@ -104,6 +105,18 @@ public function buildBlock($build) { // can be rendered without the block config entity. unset($build['#block']); if (!empty($content)) { + // If the content consists only of cacheability metadata, then don't + // render the block wrapper, but an empty string with that cacheability + // metadata. + // This allows blocks to be empty, yet still bubble cacheability metadata, + // to indicate *why* it is empty. + if (count($content) === 1 && array_keys($content) === ['#cache']) { + return [ + '#markup' => '', + '#cache' => $content['#cache'], + ]; + } + // Place the $content returned by the block plugin into a 'content' child // element, as a way to allow the plugin to have complete control of its // properties and rendering (e.g., its own #theme) without conflicting diff --git a/core/modules/menu_link_content/src/Tests/MenuLinkContentCacheabilityBubblingTest.php b/core/modules/menu_link_content/src/Tests/MenuLinkContentCacheabilityBubblingTest.php index 7980c77..3c1e820 100644 --- a/core/modules/menu_link_content/src/Tests/MenuLinkContentCacheabilityBubblingTest.php +++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentCacheabilityBubblingTest.php @@ -7,6 +7,7 @@ namespace Drupal\menu_link_content\Tests; +use Drupal\Core\Access\AccessResult; use Drupal\Core\Cache\Cache; use Drupal\Core\Cache\CacheableMetadata; use Drupal\Core\Language\LanguageInterface; @@ -110,6 +111,16 @@ public function testOutboundPathAndRouteProcessing() { ], ]; + $access_anything_tree_manipulator = function (array $tree) use (&$access_anything_tree_manipulator) { + foreach ($tree as $key => $element) { + $tree[$key]->access = AccessResult::allowed(); + if ($tree[$key]->subtree) { + $tree[$key]->subtree = $access_anything_tree_manipulator($tree[$key]->subtree); + } + } + return $tree; + }; + // Test each expectation individually. foreach ($test_cases as $expectation) { $menu_link_content = MenuLinkContent::create([ @@ -118,6 +129,7 @@ public function testOutboundPathAndRouteProcessing() { ]); $menu_link_content->save(); $tree = $menu_tree->load('tools', new MenuTreeParameters()); + $tree = $menu_tree->transform($tree, [['callable' => $access_anything_tree_manipulator]]); $build = $menu_tree->build($tree); $renderer->renderRoot($build); @@ -140,6 +152,7 @@ public function testOutboundPathAndRouteProcessing() { $expected_cacheability = $expected_cacheability->merge($expectation['cacheability']); } $tree = $menu_tree->load('tools', new MenuTreeParameters()); + $tree = $menu_tree->transform($tree, [['callable' => $access_anything_tree_manipulator]]); $build = $menu_tree->build($tree); $renderer->renderRoot($build); $expected_cacheability = $expected_cacheability->merge($default_menu_cacheability); diff --git a/core/modules/system/src/Plugin/Block/SystemMenuBlock.php b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php index 4a6a2ea..aec9716 100644 --- a/core/modules/system/src/Plugin/Block/SystemMenuBlock.php +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php @@ -187,10 +187,14 @@ public function getCacheTags() { * {@inheritdoc} */ public function getCacheContexts() { - // Menu blocks must be cached per role and per active trail. + // ::build() uses MenuLinkTreeInterface::getCurrentRouteMenuTreeParameters() + // to generate menu tree parameters, and those take the active menu trail + // into account. Therefore, we must vary the rendered menu by the active + // trail of the rendered menu. + // Additional cache contexts, e.g. those that determine link text or + // accessibility of a menu, will be bubbled automatically. $menu_name = $this->getDerivativeId(); return [ - 'user.roles', 'route.menu_active_trails:' . $menu_name, ]; } diff --git a/core/modules/toolbar/toolbar.module b/core/modules/toolbar/toolbar.module index e9808d5..4a10761 100644 --- a/core/modules/toolbar/toolbar.module +++ b/core/modules/toolbar/toolbar.module @@ -177,6 +177,11 @@ function toolbar_toolbar() { '#heading' => t('Administration menu'), '#attached' => $subtrees_attached, 'toolbar_administration' => array( + // The cache contexts that must be varied by will bubble up; therefore + // setting just cache keys is sufficient. + '#cache' => [ + 'keys' => ['toolbar', 'admin_menu_tray'], + ], '#pre_render' => array( 'toolbar_prerender_toolbar_administration_tray', ), diff --git a/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php index cd1a392..83fd3eb 100644 --- a/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php @@ -7,6 +7,7 @@ namespace Drupal\Tests\Core\Menu; +use Drupal\Core\Access\AccessResult; use Drupal\Core\Menu\DefaultMenuLinkTreeManipulators; use Drupal\Core\Menu\MenuLinkTreeElement; use Drupal\Tests\UnitTestCase; @@ -154,15 +155,15 @@ public function testCheckAccess() { $this->accessManager->expects($this->exactly(4)) ->method('checkNamedRoute') ->will($this->returnValueMap(array( - array('example1', array(), $this->currentUser, FALSE, FALSE), - array('example2', array('foo' => 'bar'), $this->currentUser, FALSE, TRUE), - array('example3', array('baz' => 'qux'), $this->currentUser, FALSE, FALSE), - array('example5', array(), $this->currentUser, FALSE, TRUE), + array('example1', array(), $this->currentUser, TRUE, AccessResult::forbidden()), + array('example2', array('foo' => 'bar'), $this->currentUser, TRUE, AccessResult::allowed()->cachePerPermissions()), + array('example3', array('baz' => 'qux'), $this->currentUser, TRUE, AccessResult::neutral()), + array('example5', array(), $this->currentUser, TRUE, AccessResult::allowed()), ))); $this->mockTree(); - $this->originalTree[5]->subtree[7]->access = TRUE; - $this->originalTree[8]->access = FALSE; + $this->originalTree[5]->subtree[7]->access = AccessResult::allowed()->cachePerUser(); + $this->originalTree[8]->access = AccessResult::neutral(); $tree = $this->defaultMenuTreeManipulators->checkAccess($this->originalTree); @@ -170,7 +171,7 @@ public function testCheckAccess() { $this->assertFalse(array_key_exists(1, $tree)); // Menu link 2: route with parameters, access granted. $element = $tree[2]; - $this->assertTrue($element->access); + $this->assertEquals(AccessResult::allowed()->cachePerPermissions(), $element->access); // Menu link 3: route with parameters, access forbidden, hence removed, // including its children. $this->assertFalse(array_key_exists(3, $tree[2]->subtree)); @@ -178,14 +179,14 @@ public function testCheckAccess() { $this->assertSame(array(), $tree[2]->subtree); // Menu link 5: no route name, treated as external, hence access granted. $element = $tree[5]; - $this->assertTrue($element->access); + $this->assertEquals(AccessResult::allowed(), $element->access); // Menu link 6: external URL, hence access granted. $element = $tree[6]; - $this->assertTrue($element->access); + $this->assertEquals(AccessResult::allowed(), $element->access); // Menu link 7: 'access' already set. $element = $tree[5]->subtree[7]; - $this->assertTrue($element->access); - // Menu link 8: 'access' already set, to FALSE, hence removed. + $this->assertEquals(AccessResult::allowed()->cachePerUser(), $element->access); + // Menu link 8: 'access' already set: AccessResult::neutral(), hence removed. $this->assertFalse(array_key_exists(8, $tree)); } @@ -205,13 +206,14 @@ public function testCheckAccessWithLinkToAnyPagePermission() { $this->mockTree(); $this->defaultMenuTreeManipulators->checkAccess($this->originalTree); - $this->assertTrue($this->originalTree[1]->access); - $this->assertTrue($this->originalTree[2]->access); - $this->assertTrue($this->originalTree[2]->subtree[3]->access); - $this->assertTrue($this->originalTree[2]->subtree[3]->subtree[4]->access); - $this->assertTrue($this->originalTree[5]->subtree[7]->access); - $this->assertTrue($this->originalTree[6]->access); - $this->assertTrue($this->originalTree[8]->access); + $expected_access_result = AccessResult::allowed()->cachePerPermissions(); + $this->assertEquals($expected_access_result, $this->originalTree[1]->access); + $this->assertEquals($expected_access_result, $this->originalTree[2]->access); + $this->assertEquals($expected_access_result, $this->originalTree[2]->subtree[3]->access); + $this->assertEquals($expected_access_result, $this->originalTree[2]->subtree[3]->subtree[4]->access); + $this->assertEquals($expected_access_result, $this->originalTree[5]->subtree[7]->access); + $this->assertEquals($expected_access_result, $this->originalTree[6]->access); + $this->assertEquals($expected_access_result, $this->originalTree[8]->access); } /** @@ -233,7 +235,7 @@ public function testFlatten() { * @covers ::collectNodeLinks * @covers ::checkAccess */ - public function testCheckNodeAccess() { + public function testCheckNodeAccess() { $links = array( 1 => MenuLinkMock::create(array('id' => 'node.1', 'route_name' => 'entity.node.canonical', 'title' => 'foo', 'parent' => '', 'route_parameters' => array('node' => 1))), 2 => MenuLinkMock::create(array('id' => 'node.2', 'route_name' => 'entity.node.canonical', 'title' => 'bar', 'parent' => '', 'route_parameters' => array('node' => 2))), @@ -268,12 +270,14 @@ public function testCheckNodeAccess() { ->with('node') ->willReturn($query); + $node_access_result = AccessResult::allowed()->addCacheContexts(['user.node_grants:view']); + $tree = $this->defaultMenuTreeManipulators->checkNodeAccess($tree); - $this->assertTrue($tree[1]->access); - $this->assertTrue($tree[2]->access); + $this->assertEquals($node_access_result, $tree[1]->access); + $this->assertEquals($node_access_result, $tree[2]->access); // Ensure that access denied is set. - $this->assertFalse($tree[2]->subtree[3]->access); - $this->assertTrue($tree[2]->subtree[3]->subtree[4]->access); + $this->assertEquals(AccessResult::neutral(), $tree[2]->subtree[3]->access); + $this->assertEquals($node_access_result, $tree[2]->subtree[3]->subtree[4]->access); // Ensure that other routes than entity.node.canonical are set as well. $this->assertNull($tree[5]->access); $this->assertNull($tree[5]->subtree[6]->access); @@ -284,18 +288,18 @@ public function testCheckNodeAccess() { // Ensure that the access manager is just called for the non-node routes. $this->accessManager->expects($this->at(0)) ->method('checkNamedRoute') - ->with('test_route', [], $this->currentUser) - ->willReturn(TRUE); + ->with('test_route', [], $this->currentUser, TRUE) + ->willReturn(AccessResult::allowed()); $this->accessManager->expects($this->at(1)) ->method('checkNamedRoute') - ->with('test_route', [], $this->currentUser) - ->willReturn(FALSE); + ->with('test_route', [], $this->currentUser, TRUE) + ->willReturn(AccessResult::neutral()); $tree = $this->defaultMenuTreeManipulators->checkAccess($tree); - $this->assertTrue($tree[1]->access); - $this->assertTrue($tree[2]->access); + $this->assertEquals($node_access_result, $tree[1]->access); + $this->assertEquals($node_access_result, $tree[2]->access); $this->assertFalse(isset($tree[2]->subtree[3])); - $this->assertTrue($tree[5]->access); + $this->assertEquals(AccessResult::allowed(), $tree[5]->access); $this->assertFalse(isset($tree[5]->subtree[6])); }