Problem/Motivation

The breadcrumb block sets a cache context of URL, this is because we base the breadcrumb on the actual URL, including if it's a path alias.

However, this results in a per-path cache entry for 404s as well.

Proposed resolution

Consider not even attempting to create a breadcrumb if the path itself does not have a route associated. For 404s this would mean a single cache entry instead of one per path. Other pages would still be per-path.

Remaining tasks

Need to check if a valid breadcrumb gets made on a 404 - for example does admin/config/invalid-link include a breadcrumb to admin/config - if it does, then this would be behaviour change which might need more discussion. Although we could still default to no breadcrumb in that case.

User interface changes

API changes

Data model changes

Files: 
CommentFileSizeAuthor
#48 2699627.patch10.67 KBcatch
#45 2699627.patch10.52 KBcatch
#45 interdiff.txt2.05 KBcatch
#43 2699627.patch10.54 KBcatch
#42 interdiff.txt2.28 KBcatch
#42 2699627.patch27.57 KBcatch
#37 interdiff.txt1.45 KBcatch
#36 2699627.patch10.96 KBcatch
#35 2699627.patch9.51 KBcatch
#34 2699627.patch10.25 KBcatch
#32 2699627.patch9.51 KBcatch
#30 2699627.patch10.85 KBcatch
#27 interdiff.txt3.71 KBcatch
#27 2699627.patch10.75 KBcatch
#25 interdiff.txt2.45 KBcatch
#25 2699627.patch10.91 KBcatch
#21 2699627-20.patch10.44 KBcatch
#17 2699627-16.patch1.57 KBdawehner
#15 2699627.patch8.87 KBcatch
#15 interdiff.txt5.36 KBcatch
#13 interdiff.txt5.36 KBcatch
#13 2699627.patch8.87 KBcatch
#10 2699627.patch3.51 KBcatch
#7 2699627.patch3.54 KBcatch
#6 2699627.patch3.67 KBcatch
#4 Screen Shot 2016-04-14 at 2.01.00 PM.png49.7 KBcatch

Comments

catch created an issue. See original summary.

catch’s picture

catch’s picture

Title: Breadcrum URL cache context does not play well with 404s » Breadcrumb URL cache context does not play well with 404s
catch’s picture

Right now we show a usable breadcrumb on 404 pages if there are valid paths before the final forward slash.

So I see a couple of ways of doing this:

1. Drop that feature, don't show a breadcrumb, use an is_a_404 cache context.

2. Add a cache context for path, that supports only a certain number of path parts as well as the is_a_404 cache context.

So for example:

admin/config/xyz

If we're on a 404, we know admin/config/xyz doesn't match a route. We can also tell whether admin/config does or does not.

If admin/config matches a route, add a cache context for path but only taking into account the first path parts - so it'll be admin/config for both admin/config/abc and admin/config/xyz

If there is no route match, there'll be no breadcrumb at all, so the is_a_404 page cache context since it'll be no breadcrumb for everything.

catch’s picture

Discussed in irc with Fabianx. For now at least, we think we should do the above #2 logic, but he suggested a dedicated cache context like url.path_based_breadcrumb which makes sense to me - can keep all the logic in one place that way and don't see separate contexts being useful elsewhere anyway.

catch’s picture

Status: Active » Needs review
FileSize
3.67 KB

Here's a start.

I don't think we want to perfectly replicate the logic in the breadcrumb builder itself, since that would require matching multiple paths to routes again which could get quite expensive, as well as duplicating a lot of complex logic.

So attached does the following:

- on 404s, strip the last segment of the path so admin/config/abc becomes admin/config and /abc becomes ''

- on all other pages treat it the same as now

In addition to that we could also follow #2699613: Set a shorter TTL for 404 responses in page_cache module and set a shorter max_age on the cache context metadata - haven't done this yet though.

catch’s picture

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.

See what the bot says about that.

Also bumping to critical, I think this is actually the critical part of [##2697795] since nothing will clear the breadcrumb cache from 404s.

Status: Needs review » Needs work

The last submitted patch, 7: 2699627.patch, failed testing.

The last submitted patch, 6: 2699627.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
3.51 KB

The last submitted patch, 7: 2699627.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 10: 2699627.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
8.87 KB
5.36 KB

All good test failures.

Status: Needs review » Needs work

The last submitted patch, 13: 2699627.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
5.36 KB
8.87 KB
catch’s picture

Title: Breadcrumb URL cache context does not play well with 404s » Introduce url.breadcrumb cache context to use instead of url.path

Re-titling given the patch no longer deals with 404s.

Depending on the site this could save (tens of) thousands of cache entries.

dawehner’s picture

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.

catch’s picture

Title: Introduce url.breadcrumb cache context to use instead of url.path » url.path cache context for breadcrumbs is unnecessarily granular

How about this title?

dawehner’s picture

+1

Status: Needs review » Needs work

The last submitted patch, 17: 2699627-16.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
10.44 KB

Combining #15 and #17.

dawehner’s picture

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?

catch’s picture

Well to take casey's example from the issue that spawned this:

/0/d8ba5d9bad8c81b4c12573be00361c77!OpenDocument&ExpandSection=4,2,5,8,3,9,7 

The code here would make that:

/0

Now if you have URLs like

/0/d8ba5d9bad8c81b4c12573be00361c77/OpenDocument&ExpandSection=4,2,5,8,3,9,7 

Then it won't help much.

If all your URLs are of the structure:

/articles/the-article-title--2016-04-12.html

then it'll help a lot.

My original idea here was to copy the logic from the breadcrumb builder and check up the path directory whether we're on a 404 or not, and then use the non-404 portion for the cache context. That would mean more efficient caching for the completely random URL case, but it's going to make the cache context itself much more expensive to calculate, and then I realised we can improve things for non-404 responses anyway with simpler/cheaper logic.

If we want to do something special for 404s, I'd rather keep the logic simple still, but add something like #2699613: Set a shorter TTL for 404 responses in page_cache module and set a shorter TTL for the breadcrumbs on 404s. That's still cheap to calculate compared to matching routes, but would prevent random URLs being cached forever.

Wim Leers’s picture

Status: Needs review » Needs work

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.

catch’s picture

Status: Needs work » Needs review
FileSize
10.91 KB
2.45 KB

#24-2:

It already does:

    $breadcrumb->addCacheContexts(['url.path']);
    while (count($path_elements) > 1) {
      array_pop($path_elements);

url.path.parent makes sense. Updated patch for that.

Status: Needs review » Needs work

The last submitted patch, 25: 2699627.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
10.75 KB
3.71 KB

Forgot to update things for the new cache context name.

Status: Needs review » Needs work

The last submitted patch, 27: 2699627.patch, failed testing.

Wim Leers’s picture

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

catch’s picture

Status: Needs work » Needs review
FileSize
10.85 KB

Ouch. See how this does.

Status: Needs review » Needs work

The last submitted patch, 30: 2699627.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
9.51 KB

CacheTagsIntegrationTest now needs no changes due to the context getting collapsed.

Status: Needs review » Needs work

The last submitted patch, 32: 2699627.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
10.25 KB

Sorry. That should be it for easy-to-avoid test fails.

diff --git a/core/modules/node/src/Tests/NodeTranslationUITest.php b/core/modules/node/src/Tests/NodeTranslationUITest.php
index bdadde5..b4dce56 100644
--- a/core/modules/node/src/Tests/NodeTranslationUITest.php
+++ b/core/modules/node/src/Tests/NodeTranslationUITest.php
@@ -25,7 +25,7 @@ class NodeTranslationUITest extends ContentTranslationUITestBase {
     'theme',
     'route',
     'timezone',
-    'url.breadcrumb',
+    'url.path.parent',
     'url.query_args:_wrapper_format',
     'user'
   ];
catch’s picture

And for unrelated hunks in the patch.

catch’s picture

Addding a unit test for the new cache context.

catch’s picture

FileSize
1.45 KB

And the interdiff.

catch’s picture

Double checked and the only place we use url.path after this patch is in FormBuilder for the #action placeholder. That's only removable with #2503429: Allow both AJAX and non-AJAX forms to POST to dedicated URLs.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#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'
catch’s picture

Status: Reviewed & tested by the community » Needs work

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.

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.

#39 is all good feedback so marking CNW for that.

Wim Leers’s picture

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)
catch’s picture

Status: Needs work » Needs review
FileSize
27.57 KB
2.28 KB

Should address #39.

catch’s picture

Needed a rebase.

Wim Leers’s picture

Status: Needs review » Needs work
  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

catch’s picture

Status: Needs work » Needs review
FileSize
2.05 KB
10.52 KB

Addressing #44.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready now.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
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.

catch’s picture

Status: Needs work » Needs review
FileSize
10.67 KB

Draft CR at https://www.drupal.org/node/2713593

Updated patch including #47. Should be RTBC again assuming the bot is happy.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Moving back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed ebe946b on 8.2.x
    Issue #2699627 by catch, dawehner, Wim Leers: url.path cache context for...

  • alexpott committed 9c9b459 on 8.1.x
    Issue #2699627 by catch, dawehner, Wim Leers: url.path cache context for...
Wim Leers’s picture

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.

jhodgdon’s picture

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

Status: Fixed » Closed (fixed)

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