Issue #2250315 by dawehner, pwolanin: Fixed Regression: Bring back node access optimization to menu trees.

core/core.services.yml

@@ -314,7 +314,7 @@ services:
     arguments: ['@menu.tree_storage', '@plugin.manager.menu.link', '@router.route_provider', '@menu.active_trail', '@controller_resolver', '@cache.menu', '@current_route_match']
   menu.default_tree_manipulators:
     class: Drupal\Core\Menu\DefaultMenuLinkTreeManipulators
-    arguments: ['@access_manager', '@current_user']
+    arguments: ['@access_manager', '@current_user', '@entity.query']
   menu.active_trail:
     class: Drupal\Core\Menu\MenuActiveTrail
     arguments: ['@plugin.manager.menu.link', '@current_route_match']

core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php

@@ -8,7 +8,7 @@
 namespace Drupal\Core\Menu;
 
 use Drupal\Core\Access\AccessManagerInterface;
-use Drupal\Core\Path\PathValidator;
+use Drupal\Core\Entity\Query\QueryFactory;
 use Drupal\Core\Session\AccountInterface;
 
 /**
@@ -16,6 +16,7 @@ use Drupal\Core\Session\AccountInterface;
  *
  * This class provides menu link tree manipulators to:
  * - perform access checking
+ * - optimized node access checking
  * - generate a unique index for the elements in a tree and sorting by it
  * - flatten a tree (i.e. a 1-dimensional tree)
  * - extract a subtree of the given tree according to the active trail
@@ -36,6 +37,13 @@ class DefaultMenuLinkTreeManipulators {
    */
   protected $account;
 
+  /**
+   * The entity query factory.
+   *
+   * @var \Drupal\Core\Entity\Query\QueryFactory
+   */
+   protected $queryFactory;
+
   /**
    * Constructs a \Drupal\Core\Menu\DefaultMenuLinkTreeManipulators object.
    *
@@ -43,10 +51,13 @@ class DefaultMenuLinkTreeManipulators {
    *   The access manager.
    * @param \Drupal\Core\Session\AccountInterface $account
    *   The current user.
+   * @param \Drupal\Core\Entity\Query\QueryFactory $query_factory
+   *   The entity query factory.
    */
-  public function __construct(AccessManagerInterface $access_manager, AccountInterface $account) {
+  public function __construct(AccessManagerInterface $access_manager, AccountInterface $account, QueryFactory $query_factory) {
     $this->accessManager = $access_manager;
     $this->account = $account;
+    $this->queryFactory = $query_factory;
   }
 
   /**
@@ -84,6 +95,74 @@ class DefaultMenuLinkTreeManipulators {
     return $tree;
   }
 
+  /**
+   * Performs access checking for nodes in an optimized way.
+   *
+   * This manipulator should be added before the generic ::checkAccess() one,
+   * because it provides a performance optimization for ::checkAccess().
+   *
+   * @param \Drupal\Core\Menu\MenuLinkTreeElement[] $tree
+   *   The menu link tree to manipulate.
+   *
+   * @return \Drupal\Core\Menu\MenuLinkTreeElement[]
+   *   The manipulated menu link tree.
+   */
+  public function checkNodeAccess(array $tree) {
+    $node_links = array();
+    $this->collectNodeLinks($tree, $node_links);
+    if ($node_links) {
+      $nids = array_keys($node_links);
+
+      $query = $this->queryFactory->get('node');
+      $query->condition('nid', $nids);
+
+      // Allows admins to view all nodes, by both disabling node_access
+      // 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.
+      if ($this->account->hasPermission('bypass node access')) {
+        $query->accessCheck(FALSE);
+      }
+      else {
+        $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;
+        }
+      }
+    }
+
+    return $tree;
+  }
+
+  /**
+   * Collects the node links in the menu tree.
+   *
+   * @param \Drupal\Core\Menu\MenuLinkTreeElement[] $tree
+   *   The menu link tree to manipulate.
+   * @param array $node_links
+   *   Stores references to menu link elements to effectively set access.
+   *
+   * @return \Drupal\Core\Menu\MenuLinkTreeElement[]
+   *   The manipulated menu link tree.
+   */
+  protected function collectNodeLinks(array &$tree, array &$node_links) {
+    foreach ($tree as $key => &$element) {
+      if ($element->link->getRouteName() == 'entity.node.canonical') {
+        $nid = $element->link->getRouteParameters()['node'];
+        $node_links[$nid][$key] = $element;
+        // Deny access by default. checkNodeAccess() will re-add it.
+        $element->access = FALSE;
+      }
+      if ($element->hasChildren) {
+        $this->collectNodeLinks($element->subtree, $node_links);
+      }
+    }
+  }
+
   /**
    * Checks access for one menu link instance.
    *

core/lib/Drupal/Core/Menu/MenuLinkBase.php

@@ -114,13 +114,26 @@ abstract class MenuLinkBase extends PluginBase implements MenuLinkInterface {
     return TRUE;
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function getRouteName() {
+    return isset($this->pluginDefinition['route_name']) ? $this->pluginDefinition['route_name'] : '';
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getRouteParameters() {
+    return isset($this->pluginDefinition['route_parameters']) ? $this->pluginDefinition['route_parameters'] : array();
+  }
+
   /**
    * {@inheritdoc}
    */
   public function getUrlObject($title_attribute = TRUE) {
     $options = $this->getOptions();
-    $description = $this->getDescription();
-    if ($title_attribute && $description) {
+    if ($title_attribute && $description = $this->getDescription()) {
       $options['attributes']['title'] = $description;
     }
     if (empty($this->pluginDefinition['url'])) {

core/lib/Drupal/Core/Menu/MenuLinkInterface.php

@@ -106,6 +106,20 @@ interface MenuLinkInterface extends PluginInspectionInterface, DerivativeInspect
    */
   public function isDeletable();
 
+  /**
+   * Returns the route name, if available.
+   *
+   * @return string
+   */
+  public function getRouteName();
+
+  /**
+   * Returns the route parameters, if available.
+   *
+   * @return array
+   */
+  public function getRouteParameters();
+
   /**
    * Returns a URL object containing either the external path or route.
    *

core/lib/Drupal/Core/Menu/MenuParentFormSelector.php

@@ -67,6 +67,7 @@ class MenuParentFormSelector implements MenuParentFormSelectorInterface {
       $parameters->setMaxDepth($depth_limit);
       $tree = $this->menuLinkTree->load($menu_name, $parameters);
       $manipulators = array(
+        array('callable' => 'menu.default_tree_manipulators:checkNodeAccess'),
         array('callable' => 'menu.default_tree_manipulators:checkAccess'),
         array('callable' => 'menu.default_tree_manipulators:generateIndexAndSort'),
       );

core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php

@@ -34,6 +34,13 @@ class DefaultMenuLinkTreeManipulatorsTest extends UnitTestCase {
    */
   protected $currentUser;
 
+  /**
+   * The mocked query factory.
+   *
+   * @var \Drupal\Core\Entity\Query\QueryFactory|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $queryFactory;
+
   /**
    * The default menu link tree manipulators.
    *
@@ -63,8 +70,11 @@ class DefaultMenuLinkTreeManipulatorsTest extends UnitTestCase {
 
     $this->accessManager = $this->getMock('\Drupal\Core\Access\AccessManagerInterface');
     $this->currentUser = $this->getMock('Drupal\Core\Session\AccountInterface');
+    $this->queryFactory = $this->getMockBuilder('Drupal\Core\Entity\Query\QueryFactory')
+      ->disableOriginalConstructor()
+      ->getMock();
 
-    $this->defaultMenuTreeManipulators = new DefaultMenuLinkTreeManipulators($this->accessManager, $this->currentUser);
+    $this->defaultMenuTreeManipulators = new DefaultMenuLinkTreeManipulators($this->accessManager, $this->currentUser, $this->queryFactory);
   }
 
   /**
@@ -275,4 +285,81 @@ class DefaultMenuLinkTreeManipulatorsTest extends UnitTestCase {
     $this->assertEquals(array(4), array_keys($tree));
   }
 
+  /**
+   * Tests the optimized node access checking.
+   *
+   * @covers ::checkNodeAccess
+   * @covers ::collectNodeLinks
+   * @covers ::checkAccess
+   */
+  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))),
+      3 => MenuLinkMock::create(array('id' => 'node.3', 'route_name' => 'entity.node.canonical', 'title' => 'baz', 'parent' => 'node.2', 'route_parameters' => array('node' => 3))),
+      4 => MenuLinkMock::create(array('id' => 'node.4', 'route_name' => 'entity.node.canonical', 'title' => 'qux', 'parent' => 'node.3', 'route_parameters' => array('node' => 4))),
+      5 => MenuLinkMock::create(array('id' => 'test.1', 'route_name' => 'test_route', 'title' => 'qux', 'parent' => '')),
+      6 => MenuLinkMock::create(array('id' => 'test.2', 'route_name' => 'test_route', 'title' => 'qux', 'parent' => 'test.1')),
+    );
+    $tree = array();
+    $tree[1] = new MenuLinkTreeElement($links[1], FALSE, 1, FALSE, array());
+    $tree[2] = new MenuLinkTreeElement($links[2], TRUE, 1, FALSE, array(
+      3 => new MenuLinkTreeElement($links[3], TRUE, 2, FALSE, array(
+        4 => new MenuLinkTreeElement($links[4], FALSE, 3, FALSE, array()),
+      )),
+    ));
+    $tree[5] = new MenuLinkTreeElement($links[5], TRUE, 1, FALSE, array(
+      6 => new MenuLinkTreeElement($links[6], FALSE, 2, FALSE, array()),
+    ));
+
+    $query = $this->getMock('Drupal\Core\Entity\Query\QueryInterface');
+    $query->expects($this->at(0))
+      ->method('condition')
+      ->with('nid', array(1, 2, 3, 4));
+    $query->expects($this->at(1))
+      ->method('condition')
+      ->with('status', NODE_PUBLISHED);
+    $query->expects($this->once())
+      ->method('execute')
+      ->willReturn(array(1, 2, 4));
+    $this->queryFactory->expects($this->once())
+      ->method('get')
+      ->with('node')
+      ->willReturn($query);
+
+    $tree = $this->defaultMenuTreeManipulators->checkNodeAccess($tree);
+    $this->assertTrue($tree[1]->access);
+    $this->assertTrue($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);
+    // Ensure that other routes than entity.node.canonical are set as well.
+    $this->assertNull($tree[5]->access);
+    $this->assertNull($tree[5]->subtree[6]->access);
+
+    // On top of the node access checking now run the ordinary route based
+    // access checkers.
+
+    // 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, NULL)
+      ->willReturn(TRUE);
+    $this->accessManager->expects($this->at(1))
+      ->method('checkNamedRoute')
+      ->with('test_route', [], $this->currentUser, NULL)
+      ->willReturn(FALSE);
+    $tree = $this->defaultMenuTreeManipulators->checkAccess($tree);
+
+    $this->assertTrue($tree[1]->access);
+    $this->assertTrue($tree[2]->access);
+    $this->assertFalse(isset($tree[2]->subtree[3]));
+    $this->assertTrue($tree[5]->access);
+    $this->assertFalse(isset($tree[5]->subtree[6]));
+  }
+
+}
+
+if (!defined('NODE_PUBLISHED')) {
+  define('NODE_PUBLISHED', 1);
 }