Problem/Motivation
#2370147: Move is_front variable to template_preprocess_page() severely crippled FE development. It is very common practice to have a template that differentiates what classes or markup to add/change based on if it's the front page.
Proposed resolution
Add a route.is_front
cache context that simply returns a boolean. This, at most, would only ever create "front" variations of content, which is perfectly OK and doesn't cause the performance issue the related issue solved.
Remaining tasks
- Create a patch
- Create tests
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#14 | interdiff-2829588-14.txt | 1.44 KB | damiankloip |
#14 | 2829588-14.patch | 3.47 KB | damiankloip |
#11 | interdiff-2829588-11.txt | 1.02 KB | damiankloip |
#11 | 2829588-11.patch | 3.45 KB | damiankloip |
#8 | interdiff-2829588-6.txt | 2.18 KB | damiankloip |
Comments
Comment #2
markhalliwellComment #3
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedComment #4
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedComment #5
damiankloip CreditAttribution: damiankloip commentedHere is a first patch to get things going.
Comment #6
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedEven though the name is misleading the path matcher depends on the current_route context and hence this is a route cache context, not a path one.
So I think a good name would be:
route.is_front_page
And a good name for the class would be:
IsFrontPageRouteCacheContext
Besides that looks great!
Comment #7
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedOkay, as this depends on the path matcher I think the original is right, lets use:
- cache_context.url.path.is_front
And therefore IsFrontPathCacheContext as class name.
Comment #8
damiankloip CreditAttribution: damiankloip commentedThat works for me! Thanks Fabian.
To summarise for others: Route is not technically correct for this as PathMatcherInterface does not rely directly on the (current) route. The default PathMatcher relies on the route match service, but this could be switched out for anything implementing the interface. So we decided keeping it as 'path' makes sense.
EDIT: yes, I messed up the naming of the interdiff file.
Comment #9
damiankloip CreditAttribution: damiankloip commentedan
Comment #10
dawehnerNitpick alarm: two empty lines :P
It is pretty sad that this actually triggers a url generation, see
core/lib/Drupal/Core/Path/PathMatcher.php:99
, but sure that's not a problem from this particular patch.This could have a cache tag for the frontpage configuration.
Comment #11
damiankloip CreditAttribution: damiankloip commentedThanks Daniel. Always appreciated muchly.
1. Fixed!
2. Mhh, yeah. Not much we can do about that here. PathMatcher should sort itself out a bit if it's a problem
3. Added. However, it's kind of crappy as it ties this class to the default PathMatcher implementation but apparently we do that in tons of other places already. So I guess that ship has sailed..
Comment #12
dawehnerI also appreciate your music recommendations!
Thank you @damiankloip
Comment #13
catchRe-titling. This needs a change record for the API addition but otherwise looks good.
Can we say something like. Defines a cache context for whether the URL is the front page of the site or not? If I didn't already know the history if is_front this would confuse me.
Comment #14
damiankloip CreditAttribution: damiankloip commentedHere is that change. Thanks @catch.
Comment #15
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC - we still need a change record though
Comment #16
damiankloip CreditAttribution: damiankloip commentedThanks Fabian! Small CR here: https://www.drupal.org/node/2830442
Comment #17
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#16: Looks great, I adjusted the CR a little bit and added another complete example.
Comment #18
damiankloip CreditAttribution: damiankloip commentedLooks good Fabian!
Comment #19
alexpottCommitted 46ff3bb and pushed to 8.3.x. Thanks!
Fixed on commit.
Comment #21
markhalliwellAwesome :D Thanks everyone!
I went ahead and added this to https://www.drupal.org/docs/8/api/cache-api/cache-contexts as well.
edit: is this something that could be backported? It's a simple enough addition that doesn't actually change anything.
Comment #22
Wim LeersThanks for also updating https://www.drupal.org/docs/8/api/cache-api/cache-contexts!
Comment #23
Wim LeersThis now has both a CR and updated docs.
Comment #24
alexpott@markcarver the current patch release only receives bug fixes so this isn't eligible for backport.
Comment #25
markhalliwellOk. FWIW, patch does apply cleanly to 8.2.x (for anyone interested), not sure about 8.1.x.