Problem/Motivation
Until #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees landed, the Masquerade module was impossible to get to work correctly in Drupal 8. The Masquerade menu links contain a CSRF token and have a dynamic (uncacheable) menu link text (e.g. Stop masquerading as
). Due to the menu block's incorrect caching, this link wasn't updated dynamically as it should be.
Proposed resolution
Add an integration test covering this particular case, the problem was fixed in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees, but more integration tests covering complex use cases are of course welcome :)
Remaining tasks
None.
User interface changes
None.
API changes
None.
Original report by andypost
Problem/Motivation
Currently blocks with menus are cached by default, varying only by a few hardcoded cache contexts. This is completely wrong.
@andypost is porting the Masquerade module to Drupal 8. The Masquerade menu links contain a CSRF token and have a dynamic (uncacheable) menu link text (e.g. Stop masquerading as
). Due to the menu block's incorrect caching, this link isn't updated dynamically as it should be.
Proposed resolution
Do not cache menu blocks. Menu blocks will be made cacheable in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.
Remaining tasks
None.
User interface changes
None.
API changes
None.
Beta phase evaluation
Issue category | Task because it now just adds test coverage. |
---|---|
Issue priority | Normal, because while its important its not majorly important. |
Unfrozen changes | Unfrozen because it only changes tests. |
Comment | File | Size | Author |
---|---|---|---|
#54 | interdiff.txt | 11.19 KB | Wim Leers |
#54 | 2463659-menu-block-cache-54.patch | 5.7 KB | Wim Leers |
#47 | regression_test-2463659-47.patch | 3.81 KB | googletorp |
#39 | 2463659-menu-block-cache-39.patch | 5.74 KB | andypost |
| |||
#39 | interdiff.txt | 791 bytes | andypost |
Comments
Comment #1
andypostLet's see the tests
Comment #3
andypostWe have tests, so just prevent caching and bubble up contexts
Comment #4
Wim Leersandypost talked to me in IRC before filing this issue. (Updating the IS with that information.) Andy is porting the Masquerade module to Drupal 8. The Masquerade menu links contain a CSRF token and have a dynamic (uncacheable) menu link text (e.g.
). Due to the menu block's incorrect caching, this link isn't updated dynamically as it should be.#2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags incorrectly introduced this:
Back then, in that issue, we (stupidly!) didn't realize that not all access checking is role-based. So whenever any menu link is cacheable per something else than URL or user roles… then it breaks. I've known this for a year, but surprisingly nobody has reported this, until now! (People are starting to build actual sites, port more modules, so now this bug is being noticed.)
We've been not wanting to make this change to not make D8 slower, but we should first and foremost make Drupal 8 correct. This caching is effectively breaking correctness, as @andypost's problem demonstrates. And it comes with an important consequence: it makes it impossible to port certain contributed modules.
Comment #5
andypostThat's only a half of problem, the second one in related issue #2463753: [regression] Do not bypass route access with 'link to any page' permissions for menu links
So here's a test from that, let's see how caching affects this
PS: I think it would be easy to add title callback with counter.
Comment #6
andypost#5 has wrong assertion, so fixed
this test exactly for that, still thinking to extend the title of link with callback
Comment #7
andypostIt looks that test outdated, so maybe to clean it here or related issue?
Comment #9
andypostComment #10
Wim Leers#1805054: Cache localized, access filtered, URL resolved, and rendered menu trees landed, so this is no longer necessary :)
Comment #11
Fabianx CreditAttribution: Fabianx as a volunteer commentedI know Wim wanted to close that, but I want to see if the test-only patch now passes, we could still add the test coverage here.
Comment #12
Wim LeersOhhh, good call!
Comment #13
Wim LeersHurray! We didn't screw up :)
Comment #14
Fabianx CreditAttribution: Fabianx as a volunteer commentedClassified as normal task and added beta eval that test only changes are unfrozen.
Comment #17
Fabianx CreditAttribution: Fabianx as a volunteer commentedComment #21
andypostSo test now fails but in
Drupal\system\Tests\Routing\RouterTest
Comment #22
Wim LeersWeird.
Comment #25
Wim LeersWe should do a reroll and land this.
Comment #26
rpayanmRerolled and fixed the filename of test-only.
Comment #27
andyposthm, test-only passes, so no idea how to proceed here
Comment #28
Fabianx CreditAttribution: Fabianx as a volunteer commented#27: The problem was already fixed, this is just the test coverage.
RTBC for the test-only patch.
Could we upload only that patch, please to not confuse core committers?
Thanks!
Comment #29
andypostI'd like to see fix for book navigation in the patch because this was broken before fixed (see interdiff - the needed part of non-test-only patch)
since #4
Comment #30
Fabianx CreditAttribution: Fabianx as a volunteer commentedCan we do that in a follow-up, please? It does not match the beta eval of test only changes and should go in independently as a normal bug fix.
Comment #31
andypost..otoh maybe better to file follow-up to make proper cache context for book navigation block because it depends on node access not roles
Comment #32
andypostFiled new bug #2525804: Fix cache contexts of book navigation block
Clean patch
EDIT issue is #2483181: Make book navigation block cacheable
Comment #33
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC, given that this originally once failed, this is fine to go in.
Comment #34
Wim LeersAgreed with Fabian's reason for pushing back, and agreed with RTBC.
Comment #35
alexpottThis looks like it is testing menu translation but it is doing nothing of the sort - let's rename it to something better.
Comment #36
Wim LeersComment #37
andyposthere it is
Comment #38
Wim LeersShouldn't we also rename the test class?
Comment #39
andypostfail was unrelated
Comment #40
Fabianx CreditAttribution: Fabianx as a volunteer commentedBack to RTBC
Comment #41
googletorp CreditAttribution: googletorp as a volunteer commentedComment #44
Wim LeersComment #46
Wim LeersComment #47
googletorp CreditAttribution: googletorp as a volunteer commentedRerolled.
Comment #48
googletorp CreditAttribution: googletorp as a volunteer commentedForgot to update status.
Comment #49
Wim LeersThanks for the reroll!
Comment #50
dawehnerI don't get that, isn't the point of cache contexts that we vary by the session cache context and not disable caching directly? At least the issue title seems to say so as well.
We should maybe have something on top which tests with max age 0, don't you think so?
Comment #51
Wim LeersThis predates the session cache context and really is just an example. Replace
$_SESSION
with$_GLOBALS
if you want. The point is that it's something completely uncacheable, and that it correctly bubbles, hence ensuring the menu is re-rendered on every page request.Comment #52
Wim LeersComment #53
alexpottI must be missing something but this issue doesn't add any assertions... which is quite surprising for a test only change.
Comment #54
Wim LeersUgh, good catch, we lost the assertions it in the #2463659-47: Regression test coverage: integration test for an uncacheable menu link that depends on session data reroll…
This is a straight reroll from #39, to ensure nothing is lost.
Comment #55
Wim LeersI just did a straight reroll, so it should be okay for me to re-RTBC.
Comment #56
alexpottThis is a test only change and therefore permitted in beta. Committed 9ba65aa and pushed to 8.0.x. Thanks!
Comment #58
dawehnerIts sad that noone asked why we cannot use
$request->GetSession()