Problem/Motivation
A consequence of this bug in HEAD is that any route whose access check result varies per role, does not get the cache contexts/tags/max-age set on the response, and hence the page cache doesn't include the necessary cache tags. So when permissions are granted or revoked to the anonymous user, anonymous users will continue to get the stale cached pages from the page cache. (Also see #15.)
Blocks #606840: Enable internal page cache by default.
See #606840-26: Enable internal page cache by default in particular, but also the other comments up to and including comment 31 on that issue.
Proposed resolution
Ensure route access checking's access result's cacheability metadata is applied to the response, hence ensuring proper invalidation of the page cache.
Remaining tasks
None.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#31 | route_access_result_cacheability-2458349-31.patch | 18.82 KB | Wim Leers |
Comments
Comment #1
Wim LeersComment #2
Wim LeersComment #3
Wim LeersComment #5
Wim LeersMinor fixes.
But the test failures themselves can only be fixed by making render arrays not set incorrect
max-age
values. Currently, blocks default to max-age zero, even when they're actually cacheable. The presence of any such block makes the page uncacheable (as expected). AFAICT all test failures are due to tests verifying page cache entries unable to find those page cache entries due to incorrect usage of max-age zero.Blocks setting the wrong max-age will be fixed in #2428805: Remove the ability to configure a block's cache contexts
Comment #7
Wim Leers#2428805: Remove the ability to configure a block's cache contexts landed, now unblocked.
Comment #8
Wim LeersToo many things are currently still setting
max-age
zero, making the addition of the "max age zero -> don't cache" page cache response policy infeasible.Besides being infeasible, it's also out of scope for this issue. So, removing that from this patch.
(It may be the long-term ideal, but that doesn't make this issue the right place to do it.)
Also 2 small changes because #2428703: Add a 'user.permissions' cache context (was: "Should cache contexts be able to associate a cache tag?") has landed.
This should makes the patch (almost) green. Next: tests.
Comment #10
Wim LeersAdded test coverage.
Next: fixing #8's fails.
Comment #11
Wim LeersRemoved all the max-age stuff, that was interfering with the page cache. This is in line with #8: the page cache currently just isn't designed to deal with a bubbling max-age. Therefore, we limit ourselves to cache contexts & cache tags.
EDIT: we have #2352009: [pp-3] Bubbling of elements' max-age to the page's headers and the page cache for doing exactly that.
Comment #13
Wim LeersClearer & more complete test coverage.
Comment #14
dawehnerThe documentation was and is still not helpful at all. Yes, we see what the code is doing, but there is nothing about the why.
I'm glad that Cache::mergeTags works the same as Cache::mergeContexts
nitpick hell: use single quotes ...
Comment #15
Wim LeersI believe this is critical for similar reasons as why #2428703: Add a 'user.permissions' cache context (was: "Should cache contexts be able to associate a cache tag?") was critical: until we have this, any access result that's invalidated by a certain cache tag, or that is invalidated by a change in the permissions for a role, will not be invalidated. Therefore, when page caching is enabled, the anonymous user will continue to have access to things that they shouldn't have access to even after e.g. the permissions for the anonymous user have changed.
Comment #16
Wim Leers#14
Comment #17
dawehnerAlright, you didn't got me ... $cache_contexts = Cache::mergeTags ... its certainly using the wrong method. In general I'm curious whether we at any time had more than 2 contexts/tags we merged at the same time, ... the API is a bit sad as its documentation is not optimal.
Comment #18
Wim LeersUGH, c/p error.
Yes, we merge >2 tags in several places. E.g.
\Drupal\block\BlockViewBuilder::viewMultiple()
. For::mergeContexts()
, I just made it analogous with::mergeTags()
.Comment #19
dawehnerWell, we could have one method doing multiple and the default is just a simlpe wrapper, but these are just ideas.
Comment #20
Wim Leers#19: good ideas, but having them as separate methods keeps the door open for smarter validation in the future — they're conceptually different things, they're just both represented as strings.
Comment #21
Fabianx CreditAttribution: Fabianx for Drupal Association commentedRTBC + 1
Comment #22
Fabianx CreditAttribution: Fabianx for Drupal Association commentedFor the record:
I am not yet happy about the special casing of the headers here.
I think we should always set the headers in the response subscriber and never in the main ContentHtml Thing, but that is out of scope for this particular issue, so should be a follow-up.
---
Still RTBC
Comment #23
dawehnerI simply think you don't understand me at all.
Comment #24
Wim LeersTalked to @dawehner in IRC. Conclusion at #2454643-5: Optimize cacheability bubbling (Cache::mergeTags(), ::mergeContexts(), BubbleableMetadata), which is a more appropriate issue to further handle that.
Let's keep this focused on route's access result handling.
Comment #26
dawehnerYeah, no question, all I wanted is to throw out some general thoughts. Sorry
Comment #29
Wim Leers#2453059: Set default render cache contexts: 'theme' + 'languages:' . LanguageInterface::TYPE_INTERFACE conflicted with this. Rebased.
Comment #31
Wim LeersBroken by #2453059: Set default render cache contexts: 'theme' + 'languages:' . LanguageInterface::TYPE_INTERFACE — it introduced new failures. Should be green again now.
Comment #32
Fabianx CreditAttribution: Fabianx for Drupal Association commentedRTBC + 1
Comment #33
alexpottThis issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 6c3303d and pushed to 8.0.x. Thanks!
I think it is a shame to have to explode and reset these but I discussed with @Wim Leers in IRC and he couldn't see a way around this.
Comment #35
Wim LeersThanks! This has unblocked #2453341: Contact forms don't have necessary cache contexts & tags; flood control should work on validate, not on view.
Relevant part of the IRC discussion mentioned in #33 for future reference:
Comment #36
Fabianx CreditAttribution: Fabianx for Drupal Association commentedI created #2463009: Introduce CacheableResponseInterface: consolidate ways of setting X-Drupal-Cache-Tags/Contexts headers as follow-up to #33 and #22 so we don't forget.