BreadcrumbBuilder::applies() mismatch with cacheability metadata

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

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 🐛 VariationCache needs to be more defensive about cache context manipulation to avoid broken redirects Active 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 as well.

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 automatically?

I'm no native english speaker, but with above changes I suggested it would read easier to me.

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. I wouldn't use "have", though. If you want we can change it to the 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 😅