Breadcrumb render cache not invalidated when entity label changes

I could reproduce this in Drupal core by following these steps:

1. Create a node with title 'Title A'.
2. Change the title to 'Title B' and check the 'Create new revision'.
3. Open the Revisions tab > Breadcrumb shows the right title (Title B).
4. Change the title to 'Title C' and check the 'Create new revision'.
5. Open the Revisions tab.

Expected: the title in the breadcrumb is 'Title C'.
Actual: the title in the breadcrumb is 'Title B'.

Here is a screenshot where the breadcrumb shows 'Title I' while the page title shows 'Title J':

Maybe we should add the cacheability metadata of each upcasted object, given that the output might differ depending on it.

@dawehner suggested in IRC to set the cache tag somewhere at PathBasedBreadcrumbBuilder.

Here is a patch containing a test that demonstrates the steps that I listed at comment #3.

This patch fixes it when I test it manually but I must be doing something horribly wrong because the test that I wrote never ends.

Yes, we had a similar bug with local task access when the user had this permission.

While this might make sense, you should use addCacheableDependency($node). I'm also not sure if this is conceptually the right fix or if it's just fixing the problem as a side effect.

Thanks @berdir. Here is a new patch with your suggestion.

Verified the patch 'breadcrumb_shows_the-2607920-12.patch' in comment #12.Below are the observations
1.Reproduced the issue in drupal 8.2.x site
2.Applied the patch 'breadcrumb_shows_the-2607920-12.patch'
3.Cleared cache
4.Create a node with title 'Test 1.
5. Change the title to 'Test 2' and check the 'Create new revision'.
6. Open the Revisions tab > Breadcrumb shows the right title (Test 2).
7. Change the title to 'Test 3' and check the 'Create new revision'.
8. Open the Revisions tab, the breadcrumb now displayed right title (Test 3)

Attached snapshot for reference.

Could still go into 8.0.x, didn't fully review yet.

+++ b/core/modules/system/src/Tests/Menu/BreadcrumbTest.php
@@ -381,4 +381,33 @@ function testBreadCrumbs() {
+    $this->assertNoText('Article Bar');

This could use a positive assertion - i.e. looking for 'Article Baz' as well as the negative one. If we had a bug where breadcrumbs didn't show on cache hits it'd pass too at the moment.

Tagging novice since that should be a one-line addition to the test.

Updated patch as per #16.

+++ b/core/modules/node/src/NodeAccessControlHandler.php
@@ -64,6 +64,7 @@ public function access(EntityInterface $entity, $operation, AccountInterface $ac
+      $result->addCacheableDependency($entity);

I was wondering whether actually this should be added to EVERY EntityAccessControlHandler case instead.

Yes, I think we should do exactly that.

.

Added CacheableDependency on every EntityAccessControlHandler case

This doesn't address #18 yet at all, sorry.

Since I am new at this , can you please elaborate , what needs to be done.

@dawehner Is comment #18 talking about adding+ $result->addCacheableDependency($entity); to all the EntityAccessControlHandler files?

This is a proposed fix by adding the addCacheableDependency() call to the parent access() method.
The access() method is only overridden by the NodeAccessControlHandler class. But the checkAccess() method is implemented by most / all of them and addCacheableDependency() is not called for every AccessResult return object. What determines the use of addCacheableDependency() and why is it not called every time the $entity object exists?

    // Administrators can view/update/delete all user profiles.
    if ($account->hasPermission('administer users')) {
      return AccessResult::allowed()->cachePerPermissions();
    }

    switch ($operation) {
      case 'view':
        // Only allow view access if the account is active.
        if ($account->hasPermission('access user profiles') && $entity->isActive()) {
          return AccessResult::allowed()->cachePerPermissions()->addCacheableDependency($entity);

#2513570: Changing name (label) of content type is not reflected in breadcrumb link text is older than this, but less active.

Also has tests which could be copied here.

Removing "novice" tag. It looks like the "novice" task mentioned in comment #16 was dealt with and the issue has gotten more complicated again.

Re-rolling 27.
Applying patch from #2513570: Changing name (label) of content type is not reflected in breadcrumb link text as mentioned in #29.

Tested manually and it works now.

Reviewing the patch also looks good to me.

Lets see what Testbot thinks about it.

The problem seems to be more generic than the original issue report? Issue needs retitling.

Was about to RTBC but the test failures made me think about this a bit harder. And now I'm convinced the fix is actually incorrect.

The bug that is being fixed here is that if the path-based breadcrumb comes across an entity view page, and so the entity's label ends up being part of the breadcrumb, that breadcrumb is not invalidated when the entity, and in particular the entity label, changes.

What the current patch does is to declare that when checking access for an entity the result of that access-checking always depends on the entity itself. This fixes the bug because part of the breadcrumb building involves access checking for the respective routes, which in turn checks access on the respective entities when encountering any entity view pages.

Fundamentally the assumption the patch makes, however, is wrong. The access result does not necessarily depend on the entity itself. In fact the most common case is that it solely depends on a permission. An example of an access result that does depend on the entity is the status field of nodes. But when checking that in ::checkAccess() NodeAccessControlHandler already adds the entity as cacheability metadata as it should. In the particular case of this bug, the access result actually does not change (!), it's TRUE/allowed both before and after saving the node. So when looking at the access checking in isolation it is actually incorrect to invalidate the cache. In other words the patch introduces completely unwarranted invalidation and it just happens to be the case that that unwarranted invalidation solves the bug where the actual cached content (not the access) of the breadcrumb becomes stale.

So now turning to the breadcrumb bug itself and ignoring the access checking what happens is that when fetching the entity label for the breadcrumb, that entity is not attached as cacheability metadata to that breadcrumb. That is the actual bug and what should be fixed here.

Taking a deeper look at that that is going to be a "fun" problem to solve. The entity label is the result of TitleResolver::getTitle() which does all sorts of magic (from a predictability/cacheability point of view) involving the global request object, so not really sure how we are going to be able to do this. We need some way for (in this specific case) EntityController::title() to provide cacheability metadata about what it is doing and for that do end all the up the stack in PathBasedBreadcrumbBuilder::build(). That will require some creative thinking, I think, at least I'm coming up blank right now.

Oops, did not mean to change status.

So here's something that seems to work: Since the argument resolver looks into $request->attributes for any arguments, simply putting a cacheable_metadata key there will allow title callbacks to have such an argument where they can pass along the data. This passes at least the tests added here (although there was one broken assertion in the test, see the respective interdiff).

Notes:

• It's not possible with this approach for title callbacks to declare the $cacheable_metadata argument by reference, which is a bit unfortunate
• Possibly we should clone the request object before adding the cacheable metadata so it doesn't leak into other places.

Thanks for the feedback and fixing it. Sounds good for me.

Some minor remarks and the fix for these attached. This should fix most of the failing tests.

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -220,12 +221,15 @@ public function addBundleTitle(RouteMatchInterface $route_match, $entity_type_id
   +      $cacheable_metadata->addCacheableDependency($entity);
   

   isset check needed

2. +++ b/core/modules/node/src/Controller/NodeViewController.php
   @@ -101,11 +102,15 @@ public function view(EntityInterface $node, $view_mode = 'full', $langcode = NUL
   +    $cacheable_metadata->addCacheableDependency($node);
   

   isset check needed

3. +++ b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
   @@ -174,7 +176,12 @@ public function build(RouteMatchInterface $route_match) {
   +          $breadcrumb
   +            ->addCacheContexts($cacheable_metadata->getCacheContexts())
   +            ->addCacheTags($cacheable_metadata->getCacheTags())
   +            ->mergeCacheMaxAge($cacheable_metadata->getCacheMaxAge());
   

   This could be done as a one liner with addCacheableDependency. This method takes care of all of these merges.

Fixing the mock in the PathBasedBreadcrumbBuilderTest, tests should pass now.

+++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
@@ -290,8 +296,12 @@ protected function doGetEntity(RouteMatchInterface $route_match, EntityInterface
+      // @todo Add cacheable metadata here.

+++ b/core/modules/node/src/Controller/NodeViewController.php
@@ -101,11 +102,17 @@ public function view(EntityInterface $node, $view_mode = 'full', $langcode = NUL
+      // @todo Add cacheable metadata for ::getTranslationFromContext()

Open then these two todos. Any ideas?

Open then these two todos. Any ideas?

Not really. I was too afraid to look into EntityRepository::getTranslationContext() this morning. Did so now.

So there's already this in there:

$langcode = $this->languageManager->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->getId();
$entity->addCacheContexts(['languages:' . LanguageInterface::TYPE_CONTENT]);

So if we simply move the ::addCacheableDependency() call after the ::getTranslationFromContext() call we should be fine.

Then there's also this:

$candidates = $this->languageManager->getFallbackCandidates($context);

That ends up invoking hook_language_fallback_candidates(_OPERATION|)_alter(). So those hook implementations would need some way to pass cacheable metadata back up the stack, as well. The good thing is that these hooks already get a $context parameter, so I guess we can just add the CacheableMetadata object there.

Interestingly, because Content Translation actually implements that hook, we should be able to reproduce a bug with the missing cache metadata there as well. Let's say there are two languages A and B and A has a higher weight than B and an entity with translations in both languages. If the translation in A is unpublished then any breadcrumb including the view page of that entity should show the title in language B. Then if the translation A is published, the cache will not be invalidated, so the breadcrumb will incorrectly still show the title in language B until the next cache clear. So we should be able to add a test that does exactly that, when fixing this.

All that said, maybe we can make the whole language fallback thing a separate follow-up issue, since this already fixes a legitimate bug.

Appears to me this may be caused, at least in part, by #2699627: url.path cache context for breadcrumbs is unnecessarily granular & #2719721: BreadcrumbBuilder::applies() mismatch with cacheability metadata; possibly related to #2685637: Edited feed item breadcrumb does not reflect the change.

+++ b/core/lib/Drupal/Core/Controller/CacheableTitleResolverInterface.php
@@ -0,0 +1,37 @@
+   * @return array|string|null
+   *   The cacheable title for the route.
+   */
+  public function getCacheableTitle(Request $request, Route $route, CacheableMetadata &$cacheable_metadata);

I'm wondering whether we should have a CacheableString or similar class which encapsulates the result of a computation together with its cacheable metadata, much like we do in other places.

This issue is present in 8.6.x and is not isolated to nodes. I just verified it happens with user entities as well. To reproduce just log in, edit your username, and save. Then return to the edit screen for your user account. You'll see your old username is still utilized in the breadcrumb.

Opened #3005360: Determining language fallback candidates does not account for cacheable metadata for #45.

The patch itself needed a rebase for the ControllerResolver → ArgumentsResolver change in TitleResolver.

I then fixed the getTranslationFromContext() thing mentioned in #45 and also added a @todo referencing the aforementioned issue.

Then I added a CacheableTitle class per #47.

So this issue needs review again. In addition to the issue summary update I guess we also need a change record here to advertise the new method and also a follow-up to convert all remaining title callbacks in core to attach cacheability metadata. Possibly we also want to deprecate TitleResolver::getTitle() (but - if so - certainly in a follow-up).

Fixed coding standards violations and PathBasedBreadcrumBuilderTest.

Can somebody please change the patch to work for 8.7.x? I still have this issue (for CT Articles)

Here is the patch for 8.7.x

Hi!
Thanks to @makskhotynetskyi, patch above works good.

Confirmed patch #52 works with latest Drupal 8.7.5.

This is also an issue with 8.8.x. Attaching a patch, hoping that the tests will pass.

It looks to me like the tests for the 8.7.x patch fails because of a change in how the testbot handles calling of deprecated code. I wonder if it is even possible to make the tests pass for 8.7 as it is.

Fix test fails. The change in core/modules/node/src/Controller/NodeViewController.php need a special notice - tests failed due to a deprecation notice that I removed here. I could not find a separate issue for this, please let me know if I should make one.

Tested the patch in #63 and it works for me with the latest Drupal 8.7.6. Leaving the status as 'Needs review' for now in case someone else would like to review/test before changing to RTBC.

Fix a typo in the patch, @return stringe -> @return string.

Note: I was wrong in my last comment, the deprecation change I thought I added here has already been committed to Drupal Core in #2691675, the test failures happened because the patch did not have that deprecation change in it. This problem wouldn't have happened if we were using pull requests.

Attaching a patch with the fix for 8.7.x too.

Confirmed patch #68 works with Drupal 8.7.5. Thanks!

Nice to see this coming along.

We need an issue summary update, change record and follow up, so NW by definition.

Attempting to update issue title and summary. Feedback welcome.

@badrange Thanks for updating the title and summary! I've removed the "Needs issue summary update" tag.

@jonathanshaw Please can you point us towards what's needed for a change record? And what does "needs followup" need, specifically?

@hugovk see #49 and https://www.drupal.org/contributor-tasks/draft-change-record

This is all about breadcrumbs.

Thanks, added a draft change record: https://www.drupal.org/node/3075372.

What's next?

Setting this to RTBC because this issue has already been set to RTBC in #14, #56 and #69. The patch has also been confirmed to work in #65, and the issue summary has been updated and a change record created as requested in #70.

Before anybody complains about the patch not fixing the bug with outdated breadcrumbs after editing username, here is a fix for 8.8.x with interdiff.

User fix looks good.

Follow-up created:
#3075840: Consider deprecating TitleResolver::getTitle()

1. +++ b/core/lib/Drupal/Core/Controller/CacheableTitle.php
   @@ -0,0 +1,37 @@
   +  public function setTitle($title) {
   +    $this->title = $title;
   

   Do we need this as part of the API? I don't think we do - the only two calls are immediately after a call to new CacheableTitle, should we just make the constructor require a title instead?

   Less api to maintain then

2. +++ b/core/lib/Drupal/Core/Controller/TitleResolver.php
   @@ -48,12 +49,47 @@ public function __construct(ControllerResolverInterface $controller_resolver, Tr
   +  protected function doGetTitle(Request $request, Route $route, CacheableMetadata &$cacheable_metadata = NULL) {
   

   I don't think we need the & here, objects are always passed by reference

3. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -301,7 +307,7 @@ public function deleteTitle(RouteMatchInterface $route_match, EntityInterface $_
   +  protected function doGetEntity(RouteMatchInterface $route_match, EntityInterface $_entity = NULL, CacheableMetadata &$cacheable_metadata = NULL) {
   

   same here for the &

4. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -313,6 +319,9 @@ protected function doGetEntity(RouteMatchInterface $route_match, EntityInterface
   +        $cacheable_metadata->addCacheContexts(['route']);
   

   Are we sure this doesn't force the title to vary on each page its generated on?

   e.g. we generate a title for a breadcrumb in multiple places - if we cache by route does that mean we're not re-using?

   e.g

   node/1/edit
   node/1/revisions

   Does the title resolved for node/1 get recalculated on both pages because the route is different?

5. +++ b/core/modules/system/tests/src/Unit/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php
   @@ -252,6 +258,15 @@ public function testBuildWithThreePathElements() {
   +    $this->titleResolver->expects($this->at(0))
   ...
   +    $this->titleResolver->expects($this->at(1))
   

   can we use willReturnMap here instead of ->at, at is brittle - or even willReturnCallback

   actually, we return the same thing both times, so do we even need the at() calls?

Hi larowlan and thank you for the feedback! Here is an updated patch with some responses to your comments, and two more issues added as a bonus.

Note that I have added a new patch, 81, with interdiff from both 80 and 77 for your convenience (regarding constructor).

1. UPDATED: Please see comment #81 for this
2. Super, fixed
3. Super, fixed
4. You're absolutely right and I have to admit that I have no idea how to fix this properly. Any suggestions are welcome.
5. Again, I need help. I don't think I understand what you mean in practice by "do we even need the at() calls", if I change core/modules/system/tests/src/Unit/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php
   

       $this->titleResolver
         ->method('getCacheableTitle')
         ->with($this->anything(), $route_1)
         ->will($this->returnValue($cacheable_title));
       $this->titleResolver
         ->method('getCacheableTitle')
         ->with($this->anything(), $route_2)
         ->will($this->returnValue($cacheable_title));
   

   the tests will fail. So I'm stuck with this too.

6. --- a/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -302,12 +302,14 @@ public function deleteTitle(RouteMatchInterface $route_match, EntityInterface $_
       * @param \Drupal\Core\Entity\EntityInterface $_entity
       *   (optional) The entity, set in
       *   \Drupal\Core\Entity\Enhancer\EntityRouteEnhancer.
   +   * @param \Drupal\Core\Cache\CacheableMetadata $cacheable_metadata
   +   *   The cacheable metadata to attach the cacheable metadata of the title to.

   Oops the parameter was missing from the docblock

7. --- a/core/modules/system/src/PathBasedBreadcrumbBuilder.php
   +++ b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
   @@ -107,8 +107,8 @@ class PathBasedBreadcrumbBuilder implements BreadcrumbBuilderInterface {
       *   The inbound path processor.
       * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
       *   The config factory service.
   -   * @param \Drupal\Core\Controller\TitleResolverInterface $title_resolver
   -   *   The title resolver service.
   +   * @param \Drupal\Core\Controller\CacheableTitleResolverInterface $title_resolver
   +   *   The cacheable title resolver service.

   Ouch, we hadn't updated the parameter in the docblock

8. +++ b/core/modules/taxonomy/tests/src/Functional/TermTest.php
   @@ -605,6 +605,23 @@ public function testTermBreadcrumbs() {
        $this->assertIdentical($breadcrumbs[0]->getText(), 'Home', 'First breadcrumb text is Home');
        $this->assertIdentical($breadcrumbs[1]->getText(), $term->label(), 'Second breadcrumb text is term name on term delete page.');
        $this->assertEscaped($breadcrumbs[1]->getText(), 'breadcrumbs displayed and escaped.');
   +
   +    // Edit the term and verify that breadcrumb is updated on term edit and
   +    // delete pages.
   +    $editedTerm = [
   +      'name[0][value]' => $this->randomMachineName(14),
   +      'description[0][value]' => $this->randomMachineName(100),
   +      'parent[]' => [0],
   +    ];
   +    $this->drupalPostForm('taxonomy/term/' . $term->id() . '/edit', $edit, t('Save'));
   +    $this->drupalGet('taxonomy/term/' . $term->id() . '/edit');
   +    $breadcrumbs = $this->getSession()->getPage()->findAll('css', 'nav.breadcrumb ol li a');
   +    $this->assertIdentical($breadcrumbs[1]->getText(), $editedTerm['name[0][value]'], 'Edited breadcrumb text is updated on term edit page.');
   +
   +    $this->drupalGet('taxonomy/term/' . $term->id() . '/delete');
   +    $breadcrumbs = $this->getSession()->getPage()->findAll('css', 'nav.breadcrumb ol li a');
   +    $this->assertIdentical($breadcrumbs[1]->getText(), $editedTerm['name[0][value]'], 'Edited breadcrumb text is updated on term edit page.');
   +

   Oh my this is also a problem with term entities. And so far I haven't figured out how to fix it, but this will at least make the tests fail until we nail this. Any pointers?

I added a solution for 1, I hope you intended something like this.

Replies:

1) yep, that's what I meant, do it in the constructor 😎
4) I'd ask for assistance from @WimLeers, I'll ping him in slack for his input
5) just remove the with() from both? it always returns the same thing regardless, so I think that is fine

Good catch on the terms, haven't looked into where to fix that yet.

4. 'url.path.parent' might work. This allows for caching based on the path, excluding everything after the last forward slash.

1,2,3 should be ok

4) Waiting to hear from WimLeers or others. I'm in the Drupal Slack under the same handle as here, badrange

5) Was this how you meant? (see attached patch + interdiff). There was no need for two titleresolver calls anymore after removing with. I really don't know what I'm doing here.

6,7 should be ok

8) What about creating a separate follow up issue for this, scope creep is the best way to ensure that this issue doesn't get merged before Greenland melts.

#80.8

I think the relvant taxonomy code is in core/modules/taxonomy/src/Controller/TaxonomyController.php

  public function termTitle(TermInterface $taxonomy_term) {
    return ['#markup' => $taxonomy_term->getName(), '#allowed_tags' => Xss::getHtmlTagList()];
  }

However, the problem goes far beyond taxonomy terms. In fact it seems to apply in most places that use title callbacks.

A few minutes searching for _title_callback led me to the following (at least) that depend on an entity and probably need to be adjusted for cacheability:
node/src/Controller/NodePreviewController::title
taxonomy\Controller\TaxonomyController::vocabularyTitle
node\Controller\NodeController::revisionPageTitle
filter\Controller\FilterController::getLabel
comment\Controller\CommentController::commentPermalinkTitle
menu_ui\Controller\MenuController::menuTitle

An interesting different case is tracker\Controller\TrackerUserTab::getTitle which has

  /**
   * Title callback for the tracker.user_tab route.
   */
  public function getTitle(UserInterface $user) {
    return $user->getAccountName();
  }

All of this makes me wonder if the fix proposed here is too gentle, relies too much on developers opting in to it.

Could we instead or also intercept the title callback and ALWAYS add the first argument supplied to the callback method as a cacheable dependency? Developers would be free to remove the dependency in the vanishingly unlikely event it wasn't needed, but we wouldn't have hundreds of contrib modules forgetting to add it.

First: Thanks to jibran for the 'url.path.parent' suggestion, it seems to work!

Second: I realised that the change in #81 where we removed setTitle() in favour of a constructor broke the patch: The title stopped updating itself. I reverted that change, and while looking into this realised that the function getCacheableTitle in core/lib/Drupal/Core/Controller/TitleResolver.phpwas actually calling doGetTitle with the parameters

    $title = $this->doGetTitle($request, $route, $cacheable_title);

, so I updated the parameter naming and docblock for doGetTitle().

Why did the conversion from setTitle to __constructor break this? Well, for nodes as an example, public function title() in core/modules/node/src/Controller/NodeViewController.php adds a cacheable dependency on the node in question to $cacheable_metadata. If we use the constructor way, that information gets lost. I don't know what I'm doing wrong.

Third: It does make sense to intercept the title callback as jonathanshaw proposes, although that probably requires comments and feedback from a lot more people (could there any bc breaks?). Wonder if it would be a good idea to split this into several steps so that we can move forward.

#79.4 wrote:

Are we sure this doesn't force the title to vary on each page its generated on?
…
Does the title resolved for node/1 get recalculated on both pages because the route is different?

4 years and 9 days ago, #2483183: Make breadcrumb block cacheable introduced class Breadcrumb implements RenderableInterface, RefinableCacheableDependencyInterface, which brought cacheability to breadcrumbs 😊 #2699627: url.path cache context for breadcrumbs is unnecessarily granular improved its cacheability.

That same issue made the different breadcrumb builders associate different cacheability, based on their strategy:

• \Drupal\book\BookBreadcrumbBuilder associates the route.book_navigation cache context (because it applies only to Node routes that are part of books) and the cache tag of the book
• \Drupal\taxonomy\TermBreadcrumbBuilder associates the route cache context (it applies only to canonical Term routes) and the cache tags of each term in the lineage
• \Drupal\system\PathBasedBreadcrumbBuilder can be used for any route: like it's name says, it's only path-based. Hence it varies by url.path.parent and url.path.is_front plus whatever cacheability route access returns
• … and more

I'm surprised by the bug report and its steps to reproduce, because PathBasedBreadcrumbBuilder does this:

        $route_match = RouteMatch::createFromRequest($route_request);
        $access = $this->accessManager->check($route_match, $this->currentUser, NULL, TRUE);
        // The set of breadcrumb links depends on the access result, so merge
        // the access result's cacheability metadata.
        $breadcrumb = $breadcrumb->addCacheableDependency($access);
        if ($access->isAllowed()) {
          $title = $this->titleResolver->getTitle($route_request, $route_match->getRouteObject());

and while the title indeed doesn't carry cacheability (and this is indeed a problem), the $access alone should still have the node's cache tag. Every node save, for a new revision or not, triggers an invalidation of the node's cache tag. Yet that is somehow insufficient here. Why is that?! 🤔

1. +++ b/core/lib/Drupal/Core/Controller/CacheableTitle.php
   @@ -0,0 +1,46 @@
   +class CacheableTitle extends BubbleableMetadata {
   

   🤔 Can a title also contain attachments, such as assets or HTTP response headers? I don't think so?

   Hence I think this should implement CacheableDependencyInterface and use CacheableDependencyTrait.

2. +++ b/core/lib/Drupal/Core/Controller/TitleResolver.php
   @@ -48,12 +49,47 @@ public function __construct(ControllerResolverInterface $controller_resolver, Tr
   +   * @param \Drupal\Core\Cache\CacheableMetadata $cacheable_metadata
   +   *   The cacheable metadata to attach the cacheable metadata of the title to.
   ...
   +        $request->attributes->set('cacheable_metadata', $cacheable_metadata);
   

   Let's name this $cacheability instead, like we do in many places. Related: #2561773: CacheableMetadata is misnamed.

   Also, let's typehint to RefinableCacheableDependencyInterface, not to a concrete class.

3. +++ b/core/lib/Drupal/Core/Controller/TitleResolver.php
   @@ -68,6 +104,9 @@ public function getTitle(Request $request, Route $route) {
   +        if ($cacheable_metadata) {
   +          $cacheable_metadata->addCacheContexts(['route']);
   +        }
   

   🤔 Ideally, we'd typehint to CacheableDependencyInterface instead of what I wrote in the previous bullet, but this makes that impossible. And this is what I was asked to investigate.

   Every title is dependent on the route it's being generated for. But the code computing a title is not asked to compute a title for many things; it is always given a concrete route match already.

   Cacheability metadata (https://www.drupal.org/docs/8/api/cache-api/cache-api#cacheability-metadata) is meant to indicate that given a certain input, what variations of the input cause a different output. Varying by route only makes sense if the input consists of multiple routes. That's not the case here.

   The path-based breadcrumb builder is what should be doing the necessary varying. And there is a known bug there, that has not seen much movement because clear steps to reproduce are missing: #2719721: BreadcrumbBuilder::applies() mismatch with cacheability metadata. I think this line may be necessary here because that issue has not yet been fixed. Let's fix that first? :)

4. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -243,12 +244,17 @@ public function addBundleTitle(RouteMatchInterface $route_match, $entity_type_id
   +   * @param \Drupal\Core\Cache\CacheableMetadata $cacheable_metadata
   +   *   (optional) The cacheable metadata for the title callback.
   

   Instead of receiving an optional object by reference, only to modify it to pass out-of-band information as a side effect, I think it would be better to follow the pattern that \Drupal\Core\Access\AccessibleInterface::access() also uses: a $return_as_object boolean, defaulting to FALSE, but when opting in to this by passing TRUE instead, it would make this code return a CacheableTitle object.

Looks like intercepting the title callback might be straightforward: in TitleResolver::doGetTitle() maybe we could do something like:

    if ($callback = $route->getDefault('_title_callback')) {
      $callable = $this->controllerResolver->getControllerFromDefinition($callback);
      $arguments = $this->argumentResolver->getArguments($request, $callable);
      $route_title = call_user_func_array($callable, $arguments);
      foreach($arguments as $argument) {
        $routeTitle->addCacheableDependency($argument);
      }
    }

And this seems not unreasonable because the arguments here are not specified by the caller but instead are requested by the callback provider; if a method is requesting an argument then it seems reasonable to assume that the title it calculates varies depending on the arguments is has requested.

Thank you for the extra information, Wim Leers, and for the feedback from you and jonathanshaw. I checked out the 8.8.x branch on my machine, set some breakpoints and stepped through the code trying to figure out why the $access didn't carry the cache tag as it should, and started wondering if I was on to something when I encountered these lines of code in core/modules/node/src/NodeAccessControlHandler.php's public function access():

    if ($account->hasPermission('bypass node access')) {
      $result = AccessResult::allowed()->cachePerPermissions();
      return $return_as_object ? $result : $result->isAllowed();
    }

Neither AccessResult::allowed() nor cachePerPermissions() keep the $entity's cache tag right?

If I add (false &&) to the if so that the if always fails, the error goes away for nodes. If I log in as an editor without the bypass node access permission, the breadcrumb changes itself as it should.

The bug affects both my editor user and administrator/user 1 when editing terms and user accounts. Are we on to something here?

I want to be fully transparent: I'm not able to continue with this patch as I started in a new project at work. I hope (the magic) someone is able to pick up where I left off. I'm sorry about this, I wanted to get it all the way to the end but it turned out to be way out of my league.

@badrange - no need to be sorry - thanks for everything you've done so far! 94 comments indicates this isn't a simple fix, and you've keep this moving, someone else will pick up the baton from here

🏆

Here is a reroll of #88 after a change to function signature in core/modules/system/tests/src/Unit/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php

Poor man's interdiff:

-+   * @var \Drupal\Core\Controller\CacheableTitleResolverInterface|\PHPUnit_Framework_MockObject_MockObject
++   * @var \Drupal\Core\Controller\CacheableTitleResolverInterface|\PHPUnit\Framework\MockObject\MockObject

I still hope "someone" will be able to take Wim Leers comments in #90 and jonathanshaw's in #91 into account.

The patch from #95 wasn't applicable, so a simple reroll.

Fixed unit test.

I'm surprised by the bug report and its steps to reproduce, because PathBasedBreadcrumbBuilder does this:

        $route_match = RouteMatch::createFromRequest($route_request);
        $access = $this->accessManager->check($route_match, $this->currentUser, NULL, TRUE);
        // The set of breadcrumb links depends on the access result, so merge
        // the access result's cacheability metadata.
        $breadcrumb = $breadcrumb->addCacheableDependency($access);
        if ($access->isAllowed()) {
          $title = $this->titleResolver->getTitle($route_request, $route_match->getRouteObject());

and while the title indeed doesn't carry cacheability (and this is indeed a problem), the $access alone should still have the node's cache tag. Every node save, for a new revision or not, triggers an invalidation of the node's cache tag. Yet that is somehow insufficient here. Why is that?! 🤔

@Wim Leers, I just checked what happens in \Drupal\Core\Access\AccessManager::check() when called from within \Drupal\system\PathBasedBreadcrumbBuilder::build() during the generation of the breadcrumb on the term/edit page. See the screenshot from the debugger. It looks like the term/view is represented by a view (views.view.taxonomy_term) even though it has also an own route in case views is not installed. The view itself has only a permission access, which is the reason why the access result does not carry any term cacheability metadata. I am not familiar with views to comment on our possible options from here.

Here is the screenshot:

I've also managed to find out why the term breadcrumb test is failing. The usage post values variable was inconsistent in the test, but the most important part is that the title was retrieved through \Drupal\taxonomy\Controller\TaxonomyController::termTitle(), which didn't had yet the new parameter $cacheable_metadata.

I haven't fully reviewed the patch yet, but it would be great if we could think of a more general solution. I think that the current approach requires too much knowledge when defining a title callback.

Tested the patch given in #99 on 9.1.x-dev. It's working fine on my end.
Please refer to the SS for ref. RTBC+1.

I can't see breadcrumb via authenticated user
Any patch available for 9.1?

Here's a reroll of #99 for Drupal 8.9.2.

Fixed re-roll of #99 in #104.

Tested the patch #106 on 8.9.x-dev. It's working fine, i am attaching screenshots.

Needs a reroll for 9.1.x-dev

Thank you for testing! Updating as RTBC.

The discussion in #90 and #99 is not remotely resolved. The patch may seem to work, but there are major doubts about the correct approach.

Tagging this for visibility

@ hugovk patch #106 failed to apply while patch #99 applied successfully on my drupal 8.9.x site can you please let me know whether it is good to use #99 ? Thanks in advance!

@manish-31 We're using patch #106 on our 8.9.5 site, and it's applying successfully. I originally made #106 as a re-roll of #99 for 8.9.2 (most likely because #99 didn't apply for 8.9.2).

After apply #114 my Drupal 8 environment broken.

Fatal error: Interface 'Drupal\Core\Controller\CacheableTitleResolverInterface' not found in /app/core/lib/Drupal/Core/Controller/TitleResolver.php on line 14

Call Stack:
    0.0008     412520   1. {main}() /app/index.php:0
    0.0073     536640   2. Drupal\Core\DrupalKernel->handle() /app/index.php:19
    0.0268    1612488   3. Stack\StackedHttpKernel->handle() /app/core/lib/Drupal/Core/DrupalKernel.php:706
    0.0268    1612488   4. Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() /app/vendor/stack/builder/src/Stack/StackedHttpKernel.php:23
    0.0269    1613184   5. Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() /app/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php:52
    0.0269    1613184   6. Drupal\page_cache\StackMiddleware\PageCache->handle() /app/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php:47
    0.0272    1615800   7. Drupal\page_cache\StackMiddleware\PageCache->pass() /app/core/modules/page_cache/src/StackMiddleware/PageCache.php:85
    0.0272    1615800   8. Drupal\Core\StackMiddleware\KernelPreHandle->handle() /app/core/modules/page_cache/src/StackMiddleware/PageCache.php:106
    0.0333    1750448   9. Drupal\Core\StackMiddleware\Session->handle() /app/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php:47
    0.0380    1852520  10. Symfony\Component\HttpKernel\HttpKernel->handle() /app/core/lib/Drupal/Core/StackMiddleware/Session.php:57
    0.0380    1853576  11. Symfony\Component\HttpKernel\HttpKernel->handleRaw() /app/vendor/symfony/http-kernel/HttpKernel.php:80
    0.1283    7703808  12. Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch() /app/vendor/symfony/http-kernel/HttpKernel.php:163
    0.1286    7779568  13. call_user_func:{/app/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:142}() /app/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:142
    0.1286    7779568  14. Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray() /app/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:142
    0.1286    7779568  15. Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition() /app/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php:89
    0.1286    7779568  16. Drupal\Core\DependencyInjection\Container->get() /app/core/lib/Drupal/Core/DependencyInjection/ClassResolver.php:20
    0.1287    7785224  17. Drupal\Core\DependencyInjection\Container->createService() /app/core/lib/Drupal/Component/DependencyInjection/Container.php:171
    0.1287    7785224  18. Drupal\Core\DependencyInjection\Container->resolveServicesAndParameters() /app/core/lib/Drupal/Component/DependencyInjection/Container.php:235
    0.1287    7785224  19. Drupal\Core\DependencyInjection\Container->get() /app/core/lib/Drupal/Component/DependencyInjection/Container.php:432
    0.1287    7788432  20. Drupal\Core\DependencyInjection\Container->createService() /app/core/lib/Drupal/Component/DependencyInjection/Container.php:171
    0.1287    7788872  21. spl_autoload_call() /app/core/lib/Drupal/Component/DependencyInjection/Container.php:257
    0.1287    7788936  22. Composer\Autoload\ClassLoader->loadClass() /app/core/lib/Drupal/Component/DependencyInjection/Container.php:257
    0.1287    7789048  23. Composer\Autoload\includeFile() /app/vendor/composer/ClassLoader.php:322
    0.1290    7790032  24. include('/app/core/lib/Drupal/Core/Controller/TitleResolver.php') /app/vendor/composer/ClassLoader.php:444

Reroll for 9.2.x

Reroll for latest 9.1.x (9.1.3 as of this writing). All previous patches from #106 have line conflicts in one of two test files.

(Patch #114 is missing new Interface, hence error outlined in #118).

#131 works for me on 9.1.x branch
Moving it to RTBC

The discussion in #90 and #99 is not remotely resolved. The patch may seem to work, but there are major doubts about the correct approach.

Re-rolling existing patch for latest stable 9.1.10 release.

Verified and tested patch#135 on the drupal 9.3.x-dev version. Patch applied successfully and looks good to me.Need +1 RTBC.

Seems the patch is not working when there is a `_title_callback:` key in `*.routing.yml` title breadcrumb is not changed after title callback is changed.

my_module.response_controller:
  path: '/my-module-api/get/response/{api_id}'
  defaults:
    _title_callback: '\Drupal\my_module\Controller\MyModuleController::getTitle'
    _controller: '\Drupal\my_module\Controller\MyModuleController::build'
    api_id: '12345685'
  requirements:
    _permission: 'administer my-module api configuration'

/**
   * Builds the API response.
   */
  public function getTitle(string $api_id) {
    return $this->t('API Response for : @data', ['@data' => $api_id]);
  }

Rerolled #135. I worked against 9.4.x because that's what my project is using, so testing the patch against that for now.

I did a couple of changes, explained next comment.

1. +++ b/core/lib/Drupal/Core/Controller/CacheableTitleResolverInterface.php
   @@ -0,0 +1,35 @@
   +interface CacheableTitleResolverInterface extends TitleResolverInterface {
   

   I made the cacheable title resolver interface extend title resolver interface, to ensure we can replace that one without API changes.

2. +++ b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
   @@ -7,6 +7,7 @@
   @@ -174,7 +175,14 @@ public function build(RouteMatchInterface $route_match) {
   
   @@ -174,7 +175,14 @@ public function build(RouteMatchInterface $route_match) {
   +          if ($this->titleResolver instanceof CacheableTitleResolverInterface) {
   +            $cacheable_title = $this->titleResolver->getCacheableTitle($route_request, $route_match->getRouteObject());
   +            $breadcrumb->addCacheableDependency($cacheable_title);
   +            $title = $cacheable_title->getTitle();
   +          }
   

   We cannot change the interface being injected for BC, so we need this check here and if someone replaced the injected service use the previous getTitle method.

3. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -304,7 +305,10 @@ public function userEditPage() {
   +    if ($user && $cacheable_metadata) {
   

   Simplified this clause.

I still think this needs work, as some namespaces don't make sense to me, but maybe only moving things around.

Fixed code standards errors.

Another code standards error.

This was missing a use statement.

Removed deprecations that made tests fail.

The failure is a known random failure, so tests are green-ish.

This is a bug, so I think it can be targeted to 9.5.x

However, #90 and #99 still need to be addressed.

We have the same issue on fairly recent built website with D9 from the get-go, this issue only seems to be happening when using render cache to cache.backend.memcache, this happens on both memcache 1.5 and 1.6 with default memcache parameters on AWS.

We tried patch #142 and #152, but without success.

The issue is very straightforward to replicate, on an admin logged session editing the title that also changes the breadcrumb and going to a non-logged session, the title will reflect the change and also the URL, but the breadcrumb will stay the same. Viewing the page on the logged-in session shows up the updated breadcrumb right away, due to logged-in not being cacheable.

Using telnet to login into the memcache instance and running a full flush (FLUSH_ALL) and refreshing the non-logged-in session, the breadcrumb will update to reflect the correct changes.

Using render cache set to cache.backend.memory or even cache.backend.null the issue is not present.

Rerolling #152 for Drupal 9.5.x

Fixes CS error, how was coding standard test passing with patch #152?

Re Rolled MR #3196 for 10.x branch and added TimeInterface argument for constructor and depreciation for REQUEST_TIME .

Seems the patch is not resolving the issue, I created a test controller based on #138 refer image below.
but the issue is not resolved. Still different breadcrumb than that of title.

MR has many test failures.

Also I tried to reproduce following the IS on Drupal 10 and was not able to replicate. Title updates just fine for me, but maybe it's just me. Will rely on the tests to prove me wrong.

Created a test fail patch based on #138 to prove issue exists.

Added some feedback.

Rule of thumb I've been trying to use for new functions on interfaces or classes we should try to typehint.

Tests look good!

Fixed phpstan-baseline error in the fail test patch in #164.

Interdiff-164-166

diff --git a/core/phpstan-baseline.neon b/core/phpstan-baseline.neon
index 1e5797ced0..7ca7501bfd 100644
--- a/core/phpstan-baseline.neon
+++ b/core/phpstan-baseline.neon
@@ -2177,7 +2177,7 @@ parameters:
 
 		-
 			message: "#^Variable \\$goto in isset\\(\\) always exists and is not nullable\\.$#"
-			count: 1
+			count: 2
 			path: modules/system/tests/src/Functional/Menu/AssertBreadcrumbTrait.php
 

As @gustavowal mentioned in #155

The issue is very straightforward to replicate, on an admin logged session editing the title that also changes the breadcrumb and going to a non-logged session, the title will reflect the change and also the URL, but the breadcrumb will stay the same. Viewing the page on the logged-in session shows up the updated breadcrumb right away, due to logged-in not being cacheable.

I think we need to update fail test patch to consider logged in user as well. Which is currently not included.