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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markcarver created an issue. See original summary.

markhalliwell’s picture

Issue summary: View changes
Fabianx’s picture

Title: Provide a "route.is_front" cache context and move global "is_front" variable back » Provide a "route.is_front" cache context to allow contributed themes to move global "is_front" variable back
Fabianx’s picture

Category: Feature request » Task
Issue tags: +Contributed project blocker
damiankloip’s picture

Status: Active » Needs review
FileSize
3.44 KB

Here is a first patch to get things going.

Fabianx’s picture

+++ b/core/core.services.yml
@@ -93,6 +93,11 @@ services:
+  cache_context.url.path.front_page:
+    class: Drupal\Core\Cache\Context\PathIsFrontPageCacheContext

Even 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!

Fabianx’s picture

Okay, 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.

damiankloip’s picture

That 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.

damiankloip’s picture

+++ b/core/lib/Drupal/Core/Cache/Context/IsFrontPathCacheContext.php
@@ -19,7 +19,7 @@ class PathIsFrontPageCacheContext implements CacheContextInterface  {
+   * Constructs a IsFrontPathCacheContext object.

an

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/Context/IsFrontPathCacheContext.php
    @@ -0,0 +1,51 @@
    +
    +
    +use Drupal\Core\Cache\CacheableMetadata;
    

    Nitpick alarm: two empty lines :P

  2. +++ b/core/lib/Drupal/Core/Cache/Context/IsFrontPathCacheContext.php
    @@ -0,0 +1,51 @@
    +    return 'is_front.' . (int) $this->pathMatcher->isFrontPage();
    

    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.

  3. +++ b/core/lib/Drupal/Core/Cache/Context/IsFrontPathCacheContext.php
    @@ -0,0 +1,51 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getCacheableMetadata() {
    +    return new CacheableMetadata();
    +  }
    +
    

    This could have a cache tag for the frontpage configuration.

damiankloip’s picture

FileSize
3.45 KB
1.02 KB

Thanks 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..

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thanks Daniel. Always appreciated muchly.

I also appreciate your music recommendations!

Thank you @damiankloip

catch’s picture

Title: Provide a "route.is_front" cache context to allow contributed themes to move global "is_front" variable back » Provide a "url.path.is_front" cache context to allow contributed themes to move global "is_front" variable back
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record

Re-titling. This needs a change record for the API addition but otherwise looks good.

+++ b/core/lib/Drupal/Core/Cache/Context/IsFrontPathCacheContext.php
@@ -0,0 +1,52 @@
+ * Defines an 'is front' path cache context.

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.

damiankloip’s picture

Here is that change. Thanks @catch.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - we still need a change record though

damiankloip’s picture

Thanks Fabian! Small CR here: https://www.drupal.org/node/2830442

Fabianx’s picture

#16: Looks great, I adjusted the CR a little bit and added another complete example.

damiankloip’s picture

Looks good Fabian!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 46ff3bb and pushed to 8.3.x. Thanks!

diff --git a/core/lib/Drupal/Core/Cache/Context/IsFrontPathCacheContext.php b/core/lib/Drupal/Core/Cache/Context/IsFrontPathCacheContext.php
index 7d091a4..141b13a 100644
--- a/core/lib/Drupal/Core/Cache/Context/IsFrontPathCacheContext.php
+++ b/core/lib/Drupal/Core/Cache/Context/IsFrontPathCacheContext.php
@@ -10,7 +10,7 @@
  *
  * Cache context ID: 'url.path.is_front'.
  */
-class IsFrontPathCacheContext implements CacheContextInterface  {
+class IsFrontPathCacheContext implements CacheContextInterface {
 
   /**
    * @var \Drupal\Core\Path\PathMatcherInterface

Fixed on commit.

  • alexpott committed 46ff3bb on 8.3.x
    Issue #2829588 by damiankloip, Fabianx, markcarver, dawehner, catch:...
markhalliwell’s picture

Awesome :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.

Wim Leers’s picture

Wim Leers’s picture

Issue tags: -Needs change record

This now has both a CR and updated docs.

alexpott’s picture

@markcarver the current patch release only receives bug fixes so this isn't eligible for backport.

markhalliwell’s picture

Ok. FWIW, patch does apply cleanly to 8.2.x (for anyone interested), not sure about 8.1.x.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.