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.
Problem/Motivation
We add the url cache context in the access check
This is because we want to vary the result for 'checking access on the preview route' as opposed to 'checking it elsewhere'
But this is too granular as the result varies per url for nodes in menu trees etc.
Proposed resolution
Add a 'is preview link route' cache context.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#11 | 3037859-10.patch | 4.06 KB | larowlan |
#11 | 3037859-10-interdiff.txt | 451 bytes | larowlan |
#6 | 3037859-2.patch | 4.04 KB | larowlan |
#5 | 3037859.patch | 4.04 KB | larowlan |
Comments
Comment #2
jibranComment #3
Wim Leers+1 — that could even be a useful addition to core — see https://www.drupal.org/docs/8/api/cache-api/cache-contexts.
But … are you even absolutely sure you need a cache context? Route access results are assumed to be per-route: that's why access checks are defined on a route (as a route
_requirements
entry).Comment #4
larowlanThis code is also invoked in general entity access, to allow view access to paragraphs that are attached to unpublished nodes, so that's where we need the context added, it won't add route by default.
Comment #5
larowlanLike so
Comment #6
larowlanComment #7
larowlan@dpi/@jibran reckon you could run this on your site and repeat profiling?
Comment #8
dpiWill be prioritised shortly.
Comment #9
dpiPatch looks good and tests well.
Its probably implied, but should the 'routes' tag be added? RouteProvider uses this tag.
Comment #10
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedJust ran into this. The 'url' cache context was added to all teasers of an entity type, which were rendered on a large number of pages. The result was a render cache table that grew indefinitely.
Comment #11
larowlanComment #12
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedRunning this in production now. It seems to have had a dramatic effect on overall site performance. I think going through access checks for every entity linked from the menu was really dragging the site down.
Thanks @dpi and @larowlan for the fix :)
Comment #13
larowlanComment #14
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext for Brisbane City Council commentedComment #16
larowlanComment #18
jibranIs it suppose to be present on every page because I'm seeing it on all pages with empty value?
Comment #19
jibranIgnore that. I was looking at the wrong field.
Comment #20
jibranThis certainly the case. @Sam152 or @larowlan can you confirm?
Also getRouteObject() can return NULL. I'll create a followup for that.
Comment #21
fenstratI hit the case where the route object is null from #20, created a follow up #3100475: Cache context doesn't handle NULL route object.