BreadcrumbBuilder::applies() mismatch with cacheability metadata

That issue only optimized the path-based breadcrumb builder, so I highly doubt that that broke anything.

which has a custom breadcrumb class/service

I bet this service is not specifying the correct cache context :)

+1 for better documentation!

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbBuilderInterface.php
@@ -28,7 +28,9 @@ public function applies(RouteMatchInterface $route_match);
+   *   breadcrumb, which needs to take into account what information is needed
+   *   to see if this breadcrumb builder applies.

Not "to see if this bread crumb builder applies", but instead what the cacheability of the built breadcrumbs is.

What core decided is that the breadcrumb block cache was sufficient for the new patch. So it never tried to apply the builders because never rebuilds the block.

However, you do have a valid point. The problem is that only the builder that was chosen for the page is capable of adding cacheability metadata. Not those that decided that they don't apply.

As always, we also need cacheability metadata for negative results. So we need a way a way for a breadcrumb builder to say under which conditions he might or might not apply.

Changing the issue title to what I think the bug is here. applies() affects when the builder will run, but it doesn't stop the results of build() getting cached in such a way that it might apply to another page. That's not how you'd normally think a method called 'applies' would work.

Yes. The only reason we haven't this yet and why the mentioned fix for @jhodgdon's problem is fixing it for that use cases is because the chosen builder's cacheablity metadata sufficiently covers the previous builders.

But that's not always the case. #2699627: url.path cache context for breadcrumbs is unnecessarily granular is triggering the first problems I guess, I have a feeling I will have to fix some of my custom breadcrumb builders as well.

Here is a scenario where I think will fail right now with book module:

1. Create a node that could be in a book but isn't ( guess you need some other node that is a book parent to actually see something later on)
2. You should see the standard path based breadcrumb, because book applies() returns false.
3. Then edit the node and put it in a book with some hierarchy
4. Save, you should still get the old breadcrumb because that is cached and the path didn't change.

And book.module can do nothing to fix it.

What we could do to fix this is \Drupal\Core\Cache\CacheableDependencyInterface on builders and if they implement it, collect cacheability metadata of all builders we asked and add that to the breadcrumb returned by the one we actually chose.

One problem that the example above also shows is that book needs information in those methods that is not directly available. Specifically, it needs the $node object from the route match as it needs the cache tag from it (probably that and the path?). As a quick fix, we could introduce some state into those services and have getCacheTags() only work when called after applies. The only other way that I see is that we introduce a new applies() method on a new interfac that can return something like AccessResult. A binary result + cacheablity metadata.

The only other way that I see is that we introduce a new applies() method on a new interfac that can return something like AccessResult. A binary result + cacheablity metadata.

This was my first thought here - new interface, extend the old interface. We could also pass in an optional CacheableMetadata object into the applies method so the return doesn't need to change.

As always, we also need cacheability metadata for negative results. So we need a way a way for a breadcrumb builder to say under which conditions he might or might not apply.

Does this mean we need to run through all breadcrumb builders?

I believe we are seeing a similar issue with the breadcrumb block but it's reproducible by clicking around the site.

The problem (probably) stems from having url aliases generated by menu structure. As I understand it #2699627: url.path cache context for breadcrumbs is unnecessarily granular changed the block to cache by url parent, wouldn't this mean that siblings (when url aliases are by menu structure) would get the same cached version?

That's basically what we are seeing, we can click through the top level navigation of our site and eventually it'll continue to show the same breadcrumbs across the same level pages.

I ran into a similar issue today where we didn't have a breadcrumb on the homepage, it was using the path based breadcrumb builder which adds the 'url.path.parent' but that isn't sufficient. The breadcrumb gets cached with no links and the cache context is empty since there isn't a parent page, then, on all other top level pages, they also no longer get breadcrumbs because the cache context is the same as the homepage.

The only other way that I see is that we introduce a new applies() method on a new interface that can return something like AccessResult. A binary result + cacheablity metadata.

I agree with this approach, it would solve the issue for me. FilterProcessResult is another example of this pattern in core.

Appears to me this may cause, at least in part, #2607920: Breadcrumb render cache not invalidated when entity label changes; possibly related to #2685637: Edited feed item breadcrumb does not reflect the change.

Added #2932341: Incorrect node title in breadcrumb as related and transferred credit because I've closed that as a duplicate.

This surfaced as part of some extra hardening I'm trying to introduce into VariationCache: #3452181: VariationCache needs to be more defensive about cache context manipulation to avoid broken redirects

Re #11:

Does this mean we need to run through all breadcrumb builders?

No, because they always run in the same order. So as long as we clear the cache when a new builder is introduced into the list, we're fine.

Let's say you have 3 builders, the 1st applies() check varies by route, the 2nd by domain and the 3rd by user roles (for whatever reason). Then, as long as the first in the list applies, all we need to care about is the 'route' cache context. Only when the first one ever fails and we fall back to the second one, do add the url.site cache context. At this point, VariationCache will write a CacheRedirect at the current value for 'route', setting the 'url.site' cache contexts. Then, if both the 1st and 2nd applies() check fail, will another CacheRedirect be created to allow VC to store items from the 3rd builder.

The only other way that I see is that we introduce a new applies() method on a new interface that can return something like AccessResult. A binary result + cacheablity metadata.

This was my first thought here - new interface, extend the old interface. We could also pass in an optional CacheableMetadata object into the applies method so the return doesn't need to change.

Why note use the deprecation policy guideline on introducing a new argument to an existing signature? https://www.drupal.org/about/core/policies/core-change-policies/drupal-d...

I'll try that approach to allow a 2nd argument to be passed to applies().

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Should go green now. From the other issue:

This caused StandardPerformanceTest to fail as there are now 2 more cache gets, which makes sense because we added a cache context.

The issue summary mentions "Revert or fix #2699627: url.path cache context for breadcrumbs is unnecessarily granular" can I ask which one it is? Maybe this can be flushed out more. Don't think this is an API or model change so put those as N/A

The MR is the correct approach, it's just that the issue summary needs severe updating.
The new approach is #10, #36 and #37

In essence: Allow the applies() method to set cacheability. Because the breadcrumb builders run in the same order every time and because only one can build the actual breadcrumb, we don't need to care about ALL builder's cacheable metadata, just the data up until the builder that actually applies.

I'll try to update the IS tomorrow.

Attached a patch that can be used for Drupal 10.2. Thanks for fixing the issue.

Please don't convert MRs into patches as it creates a lot of unnecessary noise. Especially when the MR is still up for review and might change.

@smustgrave Updated the IS to reflect the actual bug and how it should be fixed. The initial IS was just one case of running into it, but #3452181: VariationCache needs to be more defensive about cache context manipulation to avoid broken redirects exposes the problem in more detail as it also causes incorrect cache redirects to be written.

Maybe because I'm tired, but I had a really hard time reading the doc block to try and understand what it was trying to say. Grammatically strange sentences and missing capital letters spotted.

Code wise not much to add to the review. Looks clean and the other comments are easy to read.

Hopefully this is a bit easier on the eyes.

Already better 🙂

The cacheable metadata to add to if your check varies by or depends on something

Can't the "to" be removed? Or specify to what we're adding metadata?

Anything you specified here does not have to be repeated in the build() method as it will be merged in automatically.

Anything you have? ... merged in automatically?

I'm no native english speaker, but with above changes I suggested it would read easier to me. Te addition of "have" is probably not needed.

Can't the "to" be removed?

That would make it broken English. You have the infinite "to add" and then "to" as you're adding to something (the CacheableMetadata).

Or specify to what we're adding metadata?

This documentation is for the $cacheable_metadata parameter, so I think it should be clear that that's the thing you're adding to?

Anything you specified here does not have to be repeated in the build() method as it will be merged in automatically.

You can use "Anything you specify here" or "Anything you specified here", depending on whether you want it to read like a forward-looking suggestion or vice versa. E.g.: Anything you say will be used against you (warning, you haven't said anything yet). Anything you said will be used against you (informative, you already said those things).

I wouldn't use "have", though. Present perfect doesn't seem right here. If you want we can change it to the simple present tense.

As for "merged in", it's because you merge whatever was specified into the final result. Because that sentence has no mention of "the final result", the "to" part of "into" is dropped. Example: John was about to walk into the river, as he walked in, he felt something sharp beneath his feet.

Nah, it’s fine then 😇 again, not a native speaker. 😉

I think I understand the reason of the change and it makes sense. Previously you'd have to return true from applies if you _might_ apply and then builder would have to do the actual check and apply the cache-ability. Yeah, that's a pretty big gotcha when building these builders.

Looking at the merge, there's a lot of duplicate adding of the route to the cache-ability. I guess maybe this is a dumb question and probably well understood in the building of BreadcrumbManager but isn't varying by route kinda a baseline implied functionality for breadcrumb builders? That's why we pass the route_manager in right? Should the manager just include that by default and this change would only be used for more exotic cases?

If it helps the discussion, and example where it doesn't cache by route in core is PathBasedBreadcrumbBuilder. I found that's broken on our site and actually decorate the core version with one that adds the route to the cache-ability of the path breadcrumb builder.

No worries :) I'm not a native speaker either but I tend to want to get the grammar right, so I tend to look these things up when in doubt. You did make me think, though, and I believe the simple present is preferred here, as the implementation is arguably still to be written at the time of reading the documentation.

Will change that little detail and leave at RTBC.

left rtbc because this isn't a change this merge makes just something it highlights so could be a follow up.

Should the manager just include that by default and this change would only be used for more exotic cases?

It could, but just like you I'd argue that that is out of scope. For now I would focus on fixing the bugs caused by not correctly setting any cacheable metadata when the applies() check returns FALSE.

RE #54 now it reads better to my mind 😅

I have some BC concerns added to the MR.

This issue also requires a CR as implementations in contrib and custom code will need to change.

Will try to fix these minor issues tomorrow. In the meantime, I answered the question regarding cache operation counts.

• Created a CR here: https://www.drupal.org/node/3459274
• Updated the code to account for BC
• Addressed all feedback

Only needs a follow-up to change the signatures in D12.

Follow-up for D12 here: #3459277: Enforce second parameter in BreadcrumbBuilderInterface::applies

Reviewed the CR and both the MR. I could only nitpicking on consistency regarding whether we have , see [link]. or . See [link], but since PHPCS is happy, I can be happy with the current comments too, they are still readable and understandable.

Committed a75df83 and pushed to 11.x. Thanks!

Discussed with @catch and we agreed to backport this to 11.0.x and 10.4.x.

Thanks for this improvement! I have though found a bug for a follow-up: the cacheability is ignored if no breadcrumb builders apply, which can cause breadcrumbs to disappear across a whole site. Reported as #3482691: BreadcrumbManager ignores cacheability when no builders apply.

This has broken several contrib modules that inherit from PathBasedBreadcrumbBuilder --

- #3497976: BusinessRulesBreadcrumb must be compatible with PathBasedBreadcrumbBuilder on Drupal 10.4.0 and above
- #3507998: Fatal error: Declaration of Drupal\entity_ui\Breadcrumb\AdminBreadcrumbBuilder::applies

They're tagged services, which I think means they fall under this rule of not being covered by BC.
https://www.drupal.org/about/core/policies/core-change-policies/bc-polic...