Issue #2224861 by Wim Leers, dawehner, pwolanin: Cache SystemMenuBlock and...

Issue #2224861 by Wim Leers, dawehner, pwolanin: Cache SystemMenuBlock and BookNavigationBlock per active trail (currently cached per URL, this breaks on very long URLs).

core/modules/book/lib/Drupal/book/BookManager.php

@@ -488,16 +488,9 @@ public function bookTreeAllData($bid, $link = NULL, $max_depth = NULL) {
         '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 @@ public function bookTreeAllData($bid, $link = NULL, $max_depth = NULL) {
     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}
    */

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.
    *

core/modules/book/lib/Drupal/book/Plugin/Block/BookNavigationBlock.php

@@ -164,14 +164,30 @@ public function build() {
     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');
   }
 
 }

core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php

@@ -204,8 +204,6 @@ public function buildAllData($menu_name, $link = NULL, $max_depth = NULL) {
   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 @@ public function buildPageData($menu_name, $max_depth = NULL, $only_active_trail
             '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 @@ public function buildPageData($menu_name, $max_depth = NULL, $only_active_trail
     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}
    */

core/modules/menu_link/lib/Drupal/menu_link/MenuTreeInterface.php

@@ -59,6 +59,17 @@ public function setPath($menu_name, $path = NULL);
    */
   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.
    *

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 @@ public function testBuildTreeWithComplexData() {
     $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 @@ protected function menuLinkTranslate(&$item) {
    * {@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);
+  }
+}

core/modules/system/lib/Drupal/system/Plugin/Block/SystemMenuBlock.php

@@ -85,6 +85,17 @@ public function defaultConfiguration() {
     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 @@ public function getCacheTags() {
    * {@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');
   }
 
 }