url.path cache context for breadcrumbs is unnecessarily granular

Here is a basic test.

I'm not entirely sure about the title change, given that it let's us think less of other potential solutions, but nevermind.

+1

When I understand the problem of too many cached entries correctly, this is more about random URLs, so just dropping the last bit wouldn't help?

Also I wonder whether we can get away with always stripping the last part of the path including on non-404s. If we can that'd be a huge improvement.

+1

1. +++ b/core/modules/system/src/Cache/Context/PathBreadcrumbCacheContext.php
   @@ -0,0 +1,63 @@
   +    return  new CacheableMetadata();
   

   Nit: two spaces, should be one.

2. +++ b/core/modules/system/src/Cache/Context/PathBreadcrumbCacheContext.php
   --- a/core/modules/system/src/PathBasedBreadcrumbBuilder.php
   +++ b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
   

   I don't see any logic changes in here to match the cache context: the path-based breadcrumb builder is not yet omitting the last path component?

3. +++ b/core/modules/system/system.services.yml
   @@ -7,6 +7,11 @@ services:
   +  cache_context.url.breadcrumb:
   

   This should be url.path.breadcrumb, not url.breadcrumb.

   Per https://www.drupal.org/developing/api/8/cache/contexts, that would mean that if something on the current page already varies by url.path, that url.path.breadcrumb can be optimized away. Because it is a more specific subset, with fewer variations: it is implied.

   And actually, none of this is breadcrumb-specific, I'd call it url.path.parent.

+++ b/core/core.services.yml
@@ -80,6 +80,11 @@ services:
+  cache_context.url.breadcrumb:

+++ b/core/lib/Drupal/Core/Cache/Context/PathParentCacheContext.php
@@ -0,0 +1,60 @@
+class PathParentCacheContext implements CacheContextInterface {

interdiff doesn't match the patch: hence this patch failed tests.

#8: No, there's one more place where we use the url.path cache context: in the RequestPath condition plugin: https://api.drupal.org/api/drupal/core%21modules%21system%21src%21Plugin....

This looks tantalizingly close to ready. Marking RTBC because 8.1.0 is imminent and AFAIK catch would like to see this land. There's only nits basically, all of which can be addressed in a follow-up.

1. +++ b/core/lib/Drupal/Core/Cache/Context/PathParentCacheContext.php
   @@ -0,0 +1,60 @@
   +  /**
   +   * The request stack.
   +   *
   +   * @var \Symfony\Component\HttpFoundation\RequestStack
   +   */
   +  protected $requestStack;
   +
   +  /**
   +   * Constructs a new BookNavigationCacheContext service.
   +   *
   +   * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
   +   *   The request stack.
   +   */
   +  public function __construct(RequestStack $request_stack) {
   +    $this->requestStack = $request_stack;
   +  }
   +
   

   Nit: we have a base class that does this stuff.

   Also: the comment is still for BookNavigationCacheContext.

2. +++ b/core/lib/Drupal/Core/Cache/Context/PathParentCacheContext.php
   @@ -0,0 +1,60 @@
   +    return t('Path-based breadcrumb');
   

   This is not breadcrumb-specific.

3. +++ b/core/tests/Drupal/Tests/Core/Cache/Context/PathParentCacheContextTest.php
   @@ -0,0 +1,43 @@
   +   * Provides a list of context token sets.
   

   Comment does not match.

4. +++ b/core/tests/Drupal/Tests/Core/Cache/Context/PathParentCacheContextTest.php
   @@ -0,0 +1,43 @@
   +      ['/some/path', 'some'],
   +      ['/some/other-path', 'some'],
   +      ['/some/other/path', 'some/other'],
   +      ['/some/other/path?q=foo&b=bar', 'some/other'],
   

   I'm still missing two edge cases:

   1. '/'
   2. '/path'

I don't think this can land before 8.1.0. I mainly wanted to get some critical issues moving forward while we were in rc commit-freeze, also nice excuse to work on actual patches a bit more.

Ok, even better. I'd have been okay with this landing because this is a very low-risk patch, and even if it broke something, it'd have minimal consequences.

We need to decide if the new class + service definition makes this patch release ineligible. For me this isn't really an API addition/feature as such - it's a new implementation / minor refinement of an existing API.

+1 — it is making the cache context actually match. It's not an API addition. It's fixing a problem by adding one more implementation of an API.

Nevertheless, when this lands, we need:

1. a documentation update (of https://www.drupal.org/developing/api/8/cache/contexts)
2. a change record (to make people aware of this new cache context)

1. +++ b/core/lib/Drupal/Core/Cache/Context/PathParentCacheContext.php
   @@ -14,30 +12,13 @@
   +    return t('Path parent cache context');
   

   This must omit "cache context".

   I think just "Parent path" would actually be the best human-readable name.

2. +++ b/core/tests/Drupal/Tests/Core/Cache/Context/PathParentCacheContextTest.php
   @@ -29,7 +29,7 @@ public function testgetContext($original_path, $context) {
   +   * Provides a list of paths and expected cache contexts. ¶
   

   Nit: Trailing space.

3. +++ b/core/lib/Drupal/Core/Cache/Context/PathParentCacheContext.php
   @@ -0,0 +1,41 @@
   + * Defines the path-based breadcrumb cache context service.
   

   This has not yet been updated.

4. +++ b/core/tests/Drupal/FunctionalTests/Breadcrumb/Breadcrumb404Test.php
   @@ -0,0 +1,48 @@
   +    // Prone the cache first.
   

   Prone? Prime? :P

I think this is ready now.

diff --git a/core/tests/Drupal/FunctionalTests/Breadcrumb/Breadcrumb404Test.php b/core/tests/Drupal/FunctionalTests/Breadcrumb/Breadcrumb404Test.php
index 3b1338e..7dcafbf 100644
--- a/core/tests/Drupal/FunctionalTests/Breadcrumb/Breadcrumb404Test.php
+++ b/core/tests/Drupal/FunctionalTests/Breadcrumb/Breadcrumb404Test.php
@@ -19,6 +19,9 @@ class Breadcrumb404Test extends BrowserTestBase {
    */
   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']);
 
@@ -35,6 +38,12 @@ public function testBreadcrumbOn404Pages() {
     $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')
diff --git a/core/tests/Drupal/Tests/Core/Cache/Context/PathParentCacheContextTest.php b/core/tests/Drupal/Tests/Core/Cache/Context/PathParentCacheContextTest.php
index dc62c5e..2c44735 100644
--- a/core/tests/Drupal/Tests/Core/Cache/Context/PathParentCacheContextTest.php
+++ b/core/tests/Drupal/Tests/Core/Cache/Context/PathParentCacheContextTest.php
@@ -2,13 +2,11 @@
 
 namespace Drupal\Tests\Core\Cache\Context;
 
-use Drupal\Core\Cache\CacheableMetadata;
 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

A few things to fix on commit. We still haven't agreed that tests should have documentation.

Also before we can commit we need a CR.

Committed 9c9b459 and pushed to 8.1.x and 8.2.x. Thanks!

Updated the documentation at https://www.drupal.org/developing/api/8/cache/contexts.

The change record was already published: https://www.drupal.org/node/2713593.

I think this patch broke breadcrumbs for me... see #2719721: BreadcrumbBuilder::applies() mismatch with cacheability metadata. This is most likely the change that broke it...