Updated: Comment #0

Problem/Motivation

Reported at #2158003-135: Remove Block Cache API in favor of blocks returning #cache with cache tags by superspring.

Steps to reproduce:

  1. Install D8 HEAD.
  2. Try to add e.g. a boolean field to the "Article" content type.
  3. You'll get an error like this: Twig_Error_Runtime: An exception has been thrown during the rendering of a template ("SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'cid' […]

Proposed resolution

Cache menu blocks per active trail instead of per URL.

(That was the more efficient solution I had in mind anyway, but avoided in #2158003 because it might trigger a bikeshed. It results in a much higher cache hit ratio, because many pages share the same active menu trail.)

Remaining tasks

None.

User interface changes

None.

API changes

New: MenuActiveTrailCacheContext (cache_context.menu.active_trail is the service name).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Needs review
FileSize
8.17 KB
pwolanin’s picture

Status: Needs review » Needs work

We can't use a cache context here - we have to decide it per block based on what menu name it's rendering.

Example code for getting the numeric active trail:


     $menu_tree = \Drupal::service('menu_link.tree')
    // Check if the active trail has been overridden for this menu tree.
    $active_path = $menu_tree->getPath($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);
    // 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)) {
          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];
              }
            }
      }
  }

Implode the active trail, to get up 2 9 ints which can be part of the cache key.

Wim Leers’s picture

Title: SystemMenuBlock is cached per URL, this breaks on very long URLs » SystemMenuBlock and BookNavigationBlock are cached per URL, this breaks on very long URLs
Status: Needs work » Needs review
FileSize
15.01 KB

Thanks! :)

This reroll follows pwolanin's guidance, and provides a new MenuTreeInterface::getActiveTrail() method. Since the code is so similar in the Book module, I also fixed it there in a similar way: a new BookManagerInterface::getActiveTrail().

pwolanin’s picture

Status: Needs review » Needs work

In this context, it should be called something like the materialized path, not the "active trail".

Wim Leers’s picture

Status: Needs work » Needs review

Discussed with pwolanin, we settled on getActiveTrailIds().

Wim Leers’s picture

WTF, d.o.

The last submitted patch, 3: menu_block_active_trail-2224861-3.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: menu_block_active_trail-2224861-5.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.49 KB
14.34 KB

fixes "Undefined variable: page_not_403"

dawehner’s picture

Tried to figure out how things really work.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php
@@ -320,6 +301,39 @@ public function buildPageData($menu_name, $max_depth = NULL, $only_active_trail
+    $request = $this->requestStack->getCurrentRequest();
+
+    if ($route_name = $request->attributes->get(RouteObjectInterface::ROUTE_NAME)) {

I try to understand that check. What is the primary reason for that? Why should you ever have no route name on a page?

pwolanin’s picture

@dawehner - quite possibly this is a hold-over check from when some paths still used the old router? Also, that condition seems to be met for 404 pages?

Last patch has a bunch of unrelated changes like to EntityResolverManager extends ContainerAware

dawehner’s picture

FileSize
14.25 KB

Oh yeah, I failed here. Here is a rerole with the interdfiff applied.

Wim Leers’s picture

pwolanin & dawehner: who else would be qualified to review this?

Wim Leers’s picture

Title: SystemMenuBlock and BookNavigationBlock are cached per URL, this breaks on very long URLs » Cache SystemMenuBlock and BookNavigationBlock per active trail (currently cached per URL, this breaks on very long URLs)
danblack’s picture

catch’s picture

For cache hit efficiency this should be worth doing in its own right - the active trail can be identical for many, many different paths on a site.

Wim Leers’s picture

Indeed. #15 is nice, but what we do here is much better. From the issue summary:

(That was the more efficient solution I had in mind anyway, but avoided in #2158003 because it might trigger a bikeshed. It results in a much higher cache hit ratio, because many pages share the same active menu trail.)

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed this and it looks really nice. Thanks for refactoring book module into the new system.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Wim Leers’s picture

Issue tags: -sprint

Hurray :)

  • Commit 8866760 on 8.x by catch:
    Issue #2224861 by Wim Leers, dawehner, pwolanin: Cache SystemMenuBlock...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.