I ran into the issue of admin_toolbar_tools being incompatible with dynamic page cache - #2897309: admin_toolbar_tools module makes all pages uncacheable. After some debugging I figured the cause are the CSRF tokens in the links. However those where already correctly place-holdered, still they ended up being rendered too early causing the 'session' cache context being added in, making the requests uncachable.
Turns out, problem is that toolbar forces replacing placeholders. Fixing that resolves the problem admin_toolbar_tools and it works just fine.
Steps to reproduce based on #62:
- Log in as administrator
- Install admin_toolbar_tools
- View home page and check headers
- Apply patch
- View home page and check headers
Without patch:
X-Drupal-Dynamic-Cache: UNCACHEABLE
With patch (after reloading at least once):
X-Drupal-Dynamic-Cache: HIT
Comment | File | Size | Author |
---|---|---|---|
#75 | 2949457-9.4.x-75.patch | 7.51 KB | kim.pepper |
#75 | 2949457-9.3.x-75.patch | 7.51 KB | kim.pepper |
Comments
Comment #2
fagoPatch fixes the problem for me, applies to 8.4.x, 8.5.x, 8.6.x.
Comment #3
fagoNote that the fix of #2899392: user_hook_toolbar() makes all pages uncacheable is also required to make it work with 8.4.x
Comment #5
MiroslavBanov CreditAttribution: MiroslavBanov as a volunteer and at FFW commentedI am reproducing this same issue on Drupal 8.6.x. When I comment
toolbar_page_top()
dynamic page cache starts working.This can easily be reproduced in simplytest.me with just installing base drupal:
I tried the patch above - it doesn't make any difference for me. I don't really understand why it would make a difference, and what exactly CSRF tokens you are referring to, and where they are coming from.
Comment #7
fagoMaybe in your scenario something else is preventing dynamic cache to work in addition.
> I don't really understand why it would make a difference, and what exactly CSRF tokens you are referring to, and where they are coming from.
Well, in my case it was devel toolbar adding links with CSRF tokens. That's usually fine as they should be lazy-placeholdered, but toolbar's wrong call to renderPlain() forces the placesholders to be applied to early, and thus makes the render array dependent on cache context that prevents dynamic page cache.
As the testbot has shown, there was a problem in my previous patch though. The access handler tries to use that internal code without an active render context. I updated the patch to fix that also.
Comment #8
fagoI guess bug-fixes can still make it into 8.6.x? -> Changing version.
Comment #9
idebr CreditAttribution: idebr at ezCompany commented@fago Thanks for the patch and clearly describing the issue at hand. I added appropriate issue tags for increased visibility. If I am not mistaken, bug reports are fixed in the latest release branch (currently 8.7.x) and then cherry-picked to earlier supported branches (currently 8.6.x and 8.5.x) when applicable.
Attached patch adds failing test to show the Toolbar currently handles rendering incorrectly.
Comment #11
andypostTest-only failed to apply
Comment #12
nils.destoop CreditAttribution: nils.destoop as a volunteer and at Wunder commentedAt first sight, this patch was not working for me. But it was because shortcut was also enabled. Shortcut always adds the user context. Uninstalled shortcut, and dynamic page cache . was ok.
Comment #13
MiroslavBanov CreditAttribution: MiroslavBanov as a volunteer and at FFW commented#12
Thanks for pointing that out. Uninstalling Shortcut module fixes the dynamic page cache, for when installing just Standard profile. That was the problem in #5. Also found the issue here #2914110: Shortcut hook_toolbar implementation makes all pages uncacheable
Comment #14
idebr CreditAttribution: idebr at ezCompany commentedRerolled #9 so the patches can be applied to 8.7.x
Comment #16
idebr CreditAttribution: idebr at ezCompany commentedComment #18
akalam CreditAttribution: akalam at Metadrop commented#9 is working for us on d8.5
Comment #19
borisson_This looks awesome, great work everyone!
Comment #20
catchThis needs a reword, for example "thus provide one to avoid an early rendering exception" - if that's the correct explanation.
Comment #21
idebr CreditAttribution: idebr at ezCompany commentedThe solution to render the subtree in a new RenderContext is similar to the implementation of media embeds rendering in #2831944: Implement media source plugin for remote video via oEmbed, so I updated the comment accordingly.
Comment #23
andypostComment #24
alexpottIs this necessary? I can't see why it is. I'm probably missing something.
Whilst this patch is current adding test coverage it looks a bit fragile because we're not actually asserting that the additional cacheability metadata is captured.
Comment #25
idebr CreditAttribution: idebr at ezCompany commented#24.1 The conflict with Dynamic Page Cache becomes apparent when the Toolbar displays a menu link with a CSRF token. This is the case when a site uses the Admin Toolbar module. The test coverage adds menu links with a CSRF token to simulate this use case.
#25.2 Actually the patch removes the 'session' cache context, so Dynamic Page Cache starts working again. This is checked using the existing assertion in the 'X-Drupal-Dynamic-Cache' response header in
\Drupal\Tests\toolbar\Functional\ToolbarCacheContextsTest::testCacheIntegration()
.Comment #26
Wim LeersI'll review this. Will try to do it before the end of the week.
Comment #27
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC + 1 from my side and great catch. (leaving final RTBC to Wim due to it having been RTBC twice already before)
executeInRenderContext is correct to use here for the access check as the result is not actually rendered and the inner cacheability hence does not matter.
i.e. the only reason for using renderPlain() in the first place was to avoid the Exception thrown when no render context is active during the access check.
Therefore this should be good to go.
Comment #28
Wim LeersSorry for the delay, got a lot of very high priority things on my plate.
👍
This is changing
render this, including replacing placeholders for things like CSRF tokens in links, and also, don't bubble cacheability
to:
render this, but don't replace placeholders just yet for things like CSRF tokens in links, and also, letting cacheability bubble is fine
(This is also what the original path in #2 did.)
👎 Because of the change above, this change is necessary, but not for the reason described in this comment. The reason that we need to execute this in a render context is to avoid the bubbling of cacheability when merely calculating the hash to check access.
We wouldn't need any of these changes if #1 would be executing its render logic in a render context. Those are the places concerned with rendering, not this one. The need for the changes in this hunk shows we're making an abstraction leaky.
This fixes that. Makes the patch much simpler.
Comment #29
Wim LeersFar more importantly though, is the question of whether this still results in correct caching. Yes, perhaps the appearance of a link with a CSRF token resulted in an uncacheable toolbar, but … is making it cacheable actually correct?
So let's test with D8 HEAD with the
csrf_test
module enabled and the two hunks from the above patch applied to it (in2949457-29-manual_testing-do-not-test.patch
, attached)The first two are for the toolbar that you see at the top of the page. The last two are for the admin menu subtree, which is accessible in the vertical orientation of the toolbar tray labeled "manage".
The 1st and 3rd are "redirecting" cache items, where bubbled cache contexts are collected to point to the appropriate variation (the 2nd and 4th, respectively).
Note how the 2nd and 4th contain:
[session]=4ODqTRyHK_ypQzGtUfVeXgRInoCPOc9uwQw43kpOE8c
. That means that thesession
cache context is part of their CID.This is what allows them to cache the entire subtree: because the subtree contains a CSRF token, it ought to vary by it. Otherwise a user with the same permissions would be able to see this user's CSRF token.
Note the absence of the
session
cache context! Now, this is fine, if the contents then also do not vary bysession
, i.e. if the cached contents don't contain a CSRF token.Sadly, they do!
So this patch "fixes" the symptom of responses not being cacheable by Dynamic Page Cache. But it turns out that this was happening to guarantee cache correctness.
Also, this means that the issue title was quite misleading. Fixed.
Comment #30
Wim LeersI also don't see how this is
. How many people truly are affected by this? Who adds a CSRF link to their admin menu?Comment #31
Wim LeersComment #32
idebr CreditAttribution: idebr at iO commentedThe Admin Toolbar has a submodule Admin Toolbar Tools that adds menu links that require a CSRF token, such as 'Run cron'. Drupal.org does not report on submodule usage, but I imagine a large portion of module installs has this submodule enabled as well. This means many site builders and developers are currently working without Dynamic Page Caching.
Comment #33
rgpublic@Wim: Well, I thought this is due to the menu items added by admin_tools. Flush cache, run cronjobs, etc. They all have CSRF tokens AFAIK. So this issue here effectively means: For all people with administrator privileges, all pages are uncacheable. Is it "Major"? Well, perhaps, no. Is it still quite significant? I think so, because it's totally unexpected that you get completely different caching behavior depending on whether you see some menu items or not. After D8 was launched, I took my quite a while to figure out why seemingly no pages were cached whenever I was logged in. It looked like the Dynamic Cache didnt work at all.
Comment #34
Wim Leers#32 + #33: Aha! Thanks for adding that information to this issue :) Evidently increasing priority to again!
I think that pretty much all of the Toolbar's caching should be reassessed. It was written in a time when render caching was pretty new, when cache contexts did not exist yet and more … all because it finished in ~2012/2013, at the very start of the Drupal 8 development cycle.
Comment #36
jibranReroll
Comment #37
jibranCorrect reroll.
Comment #39
neclimdulthis seems to renders twice? That doesn't seem right.
I'm trying to catch up and this seems pretty complicated(caching often is). What's the problem we're trying to solve out of #29?
Comment #40
jibranWe have CSRF links in the Toolbar when Admin Toolbar Tools is enabled which is a submodule Admin Toolbar but CSRF link calls kill switch and make the toolbar uncacheable which in 8.8 makes the whole page uncacheable in 8.7 and before it was just toolbar.
Comment #41
neclimdulright, that's how i got here :) just trying to figure out how to move it from NW to NR/RTBC ;)
Comment #42
larowlanComment #43
jibranFixed #39 and added asserts for session context. Turns out we need to render the link with CSRF token to get the session context.
Comment #44
kualeeReviewing this now at @Drupal South 2019. Just commenting to avoid duplication :)
Comment #45
kualeeI have tested the patch in #43 and it applied cleanly and worked fine on a fresh install of 8.8, with admin toolbar enabled. Though I have to uninstall Shortcut module like mentioned in #12 and #13
Does this patch mean to work even with the Shortcut module enabled?
Comment #46
BerdirI'm a bit confused about the status here, is there something that needs to be done based on the feedback from @Wim or is that OK now?
Comment #47
jibranI think the latest patch addressed issue discussed in #29.
Comment #48
acbramley CreditAttribution: acbramley at PreviousNext commentedI've applied #43 to a couple of projects and it works well. Along with admin_toolbar_tools, pages are now cacheable. Without the patch all pages for logged in users were marked as UNCACHEABLE.
Comment #49
fagoYep, I can confirm the patch is still needed to keep dynamic page cache working.
Comment #50
idebr CreditAttribution: idebr at iO commentedThe feedback in #29 was addressed: the cache context now includes the session. I did another manual test to confirm the patch now works as intented.
Uploading test-only patch to show the current behaviour. The patch itself is unchanged.
Comment #53
idebr CreditAttribution: idebr at iO commentedAdded legacy annotation to test, since it uses a module with a deprecated route parameter.
Comment #54
Wim Leers\Drupal\Core\Render\MainContent\HtmlRenderer::renderResponse()
vs\Drupal\Core\Render\MainContent\HtmlRenderer::prepare()
for respectively an example where we do throw its cacheability away and where we don't.)Comment #57
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedRerolling for 9.2.
Comment #59
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedFixed failed tests and some CS issues of patch #57.
Comment #60
neclimdulbump deprecations because I'm being hopeful.
re: #53.1 I don't know the answer to why the second one isn't passed by reference but I documented the first. Seems like if there was additional cache metadata deeper in subtrees that would get lost right?
re: #53.2 I'm not sure I understand the question but I get the feeling that's probably related to me not understanding why the pre-process method isn't rendering the subtrees by reference.
Comment #61
Kristen PolThanks for the update.
1) Patch still applies cleanly.
2) Reviewed the interdiff and see the version change, type hint, and new comment. IMO this part isn't super clear:
Maybe should be something like:
3) Tested as follows:
In both cases, when first viewing the home page, I get:
and when reloading:
as expected.
3) While a comment was added for the pass-by-reference noted in #54.1, #54.2 isn't addressed as is noted in #60.
Thus, while this appears to be working, marking back to needs work to address that documentation.
Comment #62
Kristen PolI didn't test properly above so here are the correct steps:
Without patch:
With patch (after reloading at least once):
Comment #63
Kristen PolBtw, regarding #54.2:
I don't see the render context used in
prepare
, only inrenderResponse
.Comment #65
dungahkAdding a patch that should resolve 2) from #61.2
#54.2 still needs to be addressed.
Comment #66
dungahkAnd here is a patch for 8.9
Comment #67
tunicTests passed, moving to needs review.
Comment #68
neclimdulStill need to address Wim's comment note and now the deprecation version needs to be bumped to 9.3
Comment #69
dungahkJust adding that this has worked for a site I've tested as well.
Comment #70
kim.pepperComment #71
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedA re-rolled patch against 8.9.x. Please review.
Comment #72
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedComment #75
kim.pepperRe-rolled #65 for 9.4.x and 9.3.x
Comment #76
Kristen PolUpdated issue summary with steps to reproduce from #62 and tagging for manual testing.
Comment #77
joshua1234511Manually tested the patch provided in #75 with the steps from issue summary.
Loged in as administrator
Install admin_toolbar_tools
checked home page and check headers
Applied the patch cleared cache
Checked home page and check headers
Test results for 9.3
Pass - Correct header is applied
Test results for 9.4
Pass - Correct header is applied
Comment #81
catchCommitted/pushed the 9.4.x patch to 10.0.x and cherry-picked back, and committed the 9.3.x patch too. Good to see this one ready to go.
Comment #82
Wim LeersWow, blast from the past! Nice to see this land indeed :)