Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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:
- Install D8 HEAD.
- Try to add e.g. a boolean field to the "Article" content type.
- 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).
Comment | File | Size | Author |
---|---|---|---|
#12 | menu_block-2224861-12.patch | 14.25 KB | dawehner |
#6 | interdiff.txt | 6.2 KB | Wim Leers |
#6 | menu_block_active_trail-2224861-5.patch | 15.04 KB | Wim Leers |
Comments
Comment #1
Wim LeersComment #2
pwolanin CreditAttribution: pwolanin commentedWe 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:
Implode the active trail, to get up 2 9 ints which can be part of the cache key.
Comment #3
Wim LeersThanks! :)
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 newBookManagerInterface::getActiveTrail()
.Comment #4
pwolanin CreditAttribution: pwolanin commentedIn this context, it should be called something like the materialized path, not the "active trail".
Comment #5
Wim LeersDiscussed with pwolanin, we settled on
getActiveTrailIds()
.Comment #6
Wim LeersWTF, d.o.
Comment #9
pwolanin CreditAttribution: pwolanin commentedfixes "Undefined variable: page_not_403"
Comment #10
dawehnerTried to figure out how things really work.
I try to understand that check. What is the primary reason for that? Why should you ever have no route name on a page?
Comment #11
pwolanin CreditAttribution: pwolanin commented@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
Comment #12
dawehnerOh yeah, I failed here. Here is a rerole with the interdfiff applied.
Comment #13
Wim Leerspwolanin & dawehner: who else would be qualified to review this?
Comment #14
Wim LeersComment #15
danblack CreditAttribution: danblack commentedAlternate fix perhaps in issue 2224847
Comment #16
catchFor 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.
Comment #17
Wim LeersIndeed. #15 is nice, but what we do here is much better. From the issue summary:
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedReviewed this and it looks really nice. Thanks for refactoring book module into the new system.
Comment #19
catchCommitted/pushed to 8.x, thanks!
Comment #20
Wim LeersHurray :)