Issue #2699627 by catch, dawehner, Wim Leers: url.path cache context for breadcrumbs is unnecessarily granular

core/core.services.yml

@@ -80,6 +80,11 @@ services:
     arguments: ['@request_stack']
     tags:
       - { name: cache.context }
+  cache_context.url.path.parent:
+    class: Drupal\Core\Cache\Context\PathParentCacheContext
+    arguments: ['@request_stack']
+    tags:
+      - { name: cache.context }
   cache_context.url.query_args:
     class: Drupal\Core\Cache\Context\QueryArgsCacheContext
     arguments: ['@request_stack']

core/lib/Drupal/Core/Cache/Context/PathParentCacheContext.php

@@ -0,0 +1,41 @@
+<?php
+
+namespace Drupal\Core\Cache\Context;
+
+use Drupal\Core\Cache\CacheableMetadata;
+
+/**
+ * Defines a cache context service for path parents.
+ *
+ * Cache context ID: 'url.path.parent'.
+ *
+ * This allows for caching based on the path, excluding everything after the
+ * last forward slash.
+ */
+class PathParentCacheContext extends RequestStackCacheContextBase implements CacheContextInterface {
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function getLabel() {
+    return t('Parent path');
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getContext() {
+    $request = $this->requestStack->getCurrentRequest();
+    $path_elements = explode('/', trim($request->getPathInfo(), '/'));
+    array_pop($path_elements);
+    return implode('/', $path_elements);
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getCacheableMetadata() {
+    return new CacheableMetadata();
+  }
+
+}

core/modules/node/src/Tests/NodeTranslationUITest.php

@@ -25,7 +25,7 @@ class NodeTranslationUITest extends ContentTranslationUITestBase {
     'theme',
     'route',
     'timezone',
-    'url.path',
+    'url.path.parent',
     'url.query_args:_wrapper_format',
     'user'
   ];

core/modules/system/src/PathBasedBreadcrumbBuilder.php

@@ -136,9 +136,9 @@ class PathBasedBreadcrumbBuilder implements BreadcrumbBuilderInterface {
     // /user is just a redirect, so skip it.
     // @todo Find a better way to deal with /user.
     $exclude['/user'] = TRUE;
-    // Because this breadcrumb builder is entirely path-based, vary by the
-    // 'url.path' cache context.
-    $breadcrumb->addCacheContexts(['url.path']);
+    // Add the url.path.parent cache context. This code ignores the last path
+    // part so the result only depends on the path parents.
+    $breadcrumb->addCacheContexts(['url.path.parent']);
     while (count($path_elements) > 1) {
       array_pop($path_elements);
       // Copy the path elements for up-casting.

core/modules/system/tests/src/Unit/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php

@@ -142,7 +142,7 @@ class PathBasedBreadcrumbBuilderTest extends UnitTestCase {
 
     $breadcrumb = $this->builder->build($this->getMock('Drupal\Core\Routing\RouteMatchInterface'));
     $this->assertEquals([], $breadcrumb->getLinks());
-    $this->assertEquals(['url.path'], $breadcrumb->getCacheContexts());
+    $this->assertEquals(['url.path.parent'], $breadcrumb->getCacheContexts());
     $this->assertEquals([], $breadcrumb->getCacheTags());
     $this->assertEquals(Cache::PERMANENT, $breadcrumb->getCacheMaxAge());
   }
@@ -159,7 +159,7 @@ class PathBasedBreadcrumbBuilderTest extends UnitTestCase {
 
     $breadcrumb = $this->builder->build($this->getMock('Drupal\Core\Routing\RouteMatchInterface'));
     $this->assertEquals([0 => new Link('Home', new Url('<front>'))], $breadcrumb->getLinks());
-    $this->assertEquals(['url.path'], $breadcrumb->getCacheContexts());
+    $this->assertEquals(['url.path.parent'], $breadcrumb->getCacheContexts());
     $this->assertEquals([], $breadcrumb->getCacheTags());
     $this->assertEquals(Cache::PERMANENT, $breadcrumb->getCacheMaxAge());
   }
@@ -194,7 +194,7 @@ class PathBasedBreadcrumbBuilderTest extends UnitTestCase {
 
     $breadcrumb = $this->builder->build($this->getMock('Drupal\Core\Routing\RouteMatchInterface'));
     $this->assertEquals([0 => new Link('Home', new Url('<front>')), 1 => new Link('Example', new Url('example'))], $breadcrumb->getLinks());
-    $this->assertEquals(['url.path', 'user.permissions'], $breadcrumb->getCacheContexts());
+    $this->assertEquals(['url.path.parent', 'user.permissions'], $breadcrumb->getCacheContexts());
     $this->assertEquals([], $breadcrumb->getCacheTags());
     $this->assertEquals(Cache::PERMANENT, $breadcrumb->getCacheMaxAge());
   }
@@ -245,7 +245,7 @@ class PathBasedBreadcrumbBuilderTest extends UnitTestCase {
       new Link('Example', new Url('example')),
       new Link('Bar', new Url('example_bar')),
     ], $breadcrumb->getLinks());
-    $this->assertEquals(['bar', 'url.path', 'user.permissions'], $breadcrumb->getCacheContexts());
+    $this->assertEquals(['bar', 'url.path.parent', 'user.permissions'], $breadcrumb->getCacheContexts());
     $this->assertEquals(['example'], $breadcrumb->getCacheTags());
     $this->assertEquals(Cache::PERMANENT, $breadcrumb->getCacheMaxAge());
   }
@@ -272,7 +272,7 @@ class PathBasedBreadcrumbBuilderTest extends UnitTestCase {
 
     // No path matched, though at least the frontpage is displayed.
     $this->assertEquals([0 => new Link('Home', new Url('<front>'))], $breadcrumb->getLinks());
-    $this->assertEquals(['url.path'], $breadcrumb->getCacheContexts());
+    $this->assertEquals(['url.path.parent'], $breadcrumb->getCacheContexts());
     $this->assertEquals([], $breadcrumb->getCacheTags());
     $this->assertEquals(Cache::PERMANENT, $breadcrumb->getCacheMaxAge());
   }
@@ -316,7 +316,7 @@ class PathBasedBreadcrumbBuilderTest extends UnitTestCase {
 
     // No path matched, though at least the frontpage is displayed.
     $this->assertEquals([0 => new Link('Home', new Url('<front>'))], $breadcrumb->getLinks());
-    $this->assertEquals(['url.path'], $breadcrumb->getCacheContexts());
+    $this->assertEquals(['url.path.parent'], $breadcrumb->getCacheContexts());
     $this->assertEquals([], $breadcrumb->getCacheTags());
     $this->assertEquals(Cache::PERMANENT, $breadcrumb->getCacheMaxAge());
   }
@@ -364,7 +364,7 @@ class PathBasedBreadcrumbBuilderTest extends UnitTestCase {
 
     $breadcrumb = $this->builder->build($this->getMock('Drupal\Core\Routing\RouteMatchInterface'));
     $this->assertEquals([0 => new Link('Home', new Url('<front>')), 1 => new Link('Admin', new Url('user_page'))], $breadcrumb->getLinks());
-    $this->assertEquals(['url.path', 'user.permissions'], $breadcrumb->getCacheContexts());
+    $this->assertEquals(['url.path.parent', 'user.permissions'], $breadcrumb->getCacheContexts());
     $this->assertEquals([], $breadcrumb->getCacheTags());
     $this->assertEquals(Cache::PERMANENT, $breadcrumb->getCacheMaxAge());
   }

core/tests/Drupal/FunctionalTests/Breadcrumb/Breadcrumb404Test.php

@@ -0,0 +1,57 @@
+<?php
+
+namespace Drupal\FunctionalTests\Breadcrumb;
+
+use Drupal\simpletest\BlockCreationTrait;
+use Drupal\Tests\BrowserTestBase;
+
+/**
+ * Tests the breadcrumb of 404 pages.
+ *
+ * @group breadcrumb
+ */
+class Breadcrumb404Test extends BrowserTestBase {
+
+  use BlockCreationTrait;
+
+  /**
+   * {@inheritdoc}
+   */
+  public static $modules = ['system', 'block'];
+
+  /**
+   * Tests that different 404s don't create unnecessary cache entries.
+   */
+  public function testBreadcrumbOn404Pages() {
+    $this->placeBlock('system_breadcrumb_block', ['id' => 'breadcrumb']);
+
+    // Prime the cache first.
+    $this->drupalGet('/not-found-1');
+    $base_count = count($this->getBreadcrumbCacheEntries());
+
+    $this->drupalGet('/not-found-2');
+    $next_count = count($this->getBreadcrumbCacheEntries());
+    $this->assertEquals($base_count, $next_count);
+
+    $this->drupalGet('/not-found-3');
+    $next_count = count($this->getBreadcrumbCacheEntries());
+    $this->assertEquals($base_count, $next_count);
+  }
+
+  /**
+   * Gets the breadcrumb cache entries.
+   *
+   * @return array
+   *   The breadcrumb cache entries.
+   */
+  protected function getBreadcrumbCacheEntries() {
+    $database = \Drupal::database();
+    $cache_entries = $database->select('cache_render')
+      ->fields('cache_render')
+      ->condition('cid', $database->escapeLike('entity_view:block:breadcrumb') . '%', 'LIKE')
+      ->execute()
+      ->fetchAllAssoc('cid');
+    return $cache_entries;
+  }
+
+}

core/tests/Drupal/Tests/Core/Cache/Context/PathParentCacheContextTest.php

@@ -0,0 +1,43 @@
+<?php
+
+namespace Drupal\Tests\Core\Cache\Context;
+
+use Drupal\Core\Cache\Context\PathParentCacheContext;
+use Drupal\Tests\UnitTestCase;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\RequestStack;
+
+/**
+ * @coversDefaultClass \Drupal\Core\Cache\Context\PathParentCacheContext
+ * @group Cache
+ */
+class PathParentCacheContextTest extends UnitTestCase {
+
+  /**
+   * @covers ::getContext
+   *
+   * @dataProvider providerTestGetContext
+   */
+  public function testgetContext($original_path, $context) {
+    $request_stack = new RequestStack();
+    $request = Request::create($original_path);
+    $request_stack->push($request);
+    $cache_context = new PathParentCacheContext($request_stack);
+    $this->assertSame($cache_context->getContext(), $context);
+  }
+
+  /**
+   * Provides a list of paths and expected cache contexts.
+   */
+  public function providerTestGetContext() {
+    return [
+      ['/some/path', 'some'],
+      ['/some/other-path', 'some'],
+      ['/some/other/path', 'some/other'],
+      ['/some/other/path?q=foo&b=bar', 'some/other'],
+      ['/some', ''],
+      ['/', ''],
+    ];
+  }
+
+}