Problem/Motivation

Since #2719721: BreadcrumbBuilder::applies() mismatch with cacheability metadata, the cacheability of whether breadcrumb builders should apply is now gathered. That's great - but in custom cases where it's possible for no breadcrumb builder to apply, that cacheability is not merged.

Currently core's path-based breadcrumb builder service that the system module provides (system.breadcrumb.default) always applies, but it's a service that can be decorated or removed.

Steps to reproduce

1. Remove the path-based breadcrumb builder service, e.g. by decorating it with a class that doesn't always return TRUE from its applies() method, or by using a service provider's alter method.
2. Visit a path on which it, and no other builder, would apply (e.g. a 404 page).
3. The breadcrumb is now cached as empty, and with no cacheability metadata.
4. Visit another path on which other breadcrumb builders might apply - the cached empty breadcrumb is now used because there was no cache context etc on it, so core assumes it is valid to use.

Proposed resolution

Merge any cacheability immediately after calling every breadcrumb builder's applies() method.

Remaining tasks

Review change.

User interface changes

None

Introduced terminology

None

API changes

None

Data model changes

None

Release notes snippet

Issue fork drupal-3482691

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

james.williams created an issue. See original summary.

james.williams’s picture

james.williams
Location Coventry, UK
commented
Version: 11.0.x-dev » 11.x-dev

james.williams opened merge request !9913

james.williams’s picture

james.williams
Location Coventry, UK
commented
Status: Active » Needs work
Issue tags: +Needs tests, +Novice

I've opened MR !9913 to demonstrate what probably needs to change; though a test would be needed anyway. Probably using a test module with a service provider to remove the path-based breadcrumb builder, or decorate it with one that won't always apply. The test would then load a page where no breadcrumb builders apply, then another where one should - an assertion should expect breadcrumbs to appear, but the test should fail without this fix as no breadcrumbs would show.

I need to focus on some other work for now, so I reckon maybe that's something a novice could pick up?

arunkumark made their first commit to this issue’s fork.

arunkumark’s picture

arunkumark
Location Coimbatore
commented
Status: Needs work » Needs review

@james.williams
I made a small change in the Cache dependency for the Breadcrumb. To handle the invalid Breadcrumb exception. Kindly check is the made will resolve your issue.

Currently moving to Needs Review.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Was previously tagged for tests which still appear to be needed.
Please read the tests before moving to review.

james.williams’s picture

james.williams
Location Coventry, UK
commented

The new test passes without the fix, which can't be right. I don't really understand what that new test is actually doing - I'm not sure it really represents the problem at hand.

We've also got an issue in that existing core tests fail with the change, with this warning:

Trying to overwrite a cache redirect with one that
has nothing in common, old one at address "languages:language_interface,
theme, user.permissions" was pointing to "url.path.parent,
url.path.is_front", new one points to "route".

Although the test itself doesn't look relevant, that does sound like we've adjusted cacheability metadata in some way that hasn't been expected. More work needed on all fronts!

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

I think we should leave the foreach as is, but change this bit:

      if ($breadcrumb instanceof Breadcrumb) {
        $context['builder'] = $builder;
        $breadcrumb->addCacheableDependency($cacheable_metadata);
        break;
      }
      else {
        throw new \UnexpectedValueException('Invalid breadcrumb returned by ' . get_class($builder) . '::build().');
      }

To drop the addCacheableDependency() call and move that below the loop.

The reason it's failing now is because you're adding the cacheable metadata to a breadcrumb that gets replaced by build() further down the loop.

james.williams’s picture

james.williams
Location Coventry, UK
commented

@kristiaanvandeneynde thank you for spotting that and pointing it out, yes! Next step then is to get the test demonstrating the actual problem. It should fail without the changes, but pass with them - whereas it's now passing in both situations.

james.williams’s picture

james.williams
Location Coventry, UK
commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests, -Novice

Finally got a working test, failing without the fix to demonstrate that it is needed, and passing with it!

Silly me for thinking that this could have been suitable for novice... I took hours fighting to get this to work! So many dark corners in the test system.... e.g. you can't assume the system module is enabled, and it's best to pass proper Url objects to $this->drupalGet(). Ah well, now I know :)

smustgrave’s picture

smustgrave commented

Left some questions/comment on MR, going to leave in review for other eyes.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs review » Needs work

Few minor nitpicks but looks good to me otherwise.

james.williams’s picture

james.williams
Location Coventry, UK
commented
Status: Needs work » Needs review

Thanks for the reviews! Should be good again now 👍

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs review » Reviewed & tested by the community

LGTM

Pipelines seem to be fine aside from the test-only one, which makes sense.

quietone’s picture

quietone commented

All questions have been answered here and the basics are all in order. I updated credit. Leaving at RTBC.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review

Left one comment on the MR, fine to self RTBC after adapting

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs review » Reviewed & tested by the community

Left a comment why we don't need the instanceof check. So back to RTBC

  • larowlan committed a8556f6f on 10.4.x 
    Issue #3482691 by james.williams, arunkumark, kristiaanvandeneynde,...

  • larowlan committed 92e0fdbe on 10.5.x 
    Issue #3482691 by james.williams, arunkumark, kristiaanvandeneynde,...

  • larowlan committed 677bd5a1 on 11.1.x 
    Issue #3482691 by james.williams, arunkumark, kristiaanvandeneynde,...

  • larowlan committed 78bbe4db on 11.x 
    Issue #3482691 by james.williams, arunkumark, kristiaanvandeneynde,...

  • larowlan committed 04bd5613 on 10.4.x 
    Revert "Issue #3482691 by james.williams, arunkumark,...

larowlan closed merge request !9913

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: 11.x-dev » 10.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to 11.x and backported to 10.5.x and 11.1.x

Originally backported to 10.4.x but then reverted because this doesn't meet the requirements for maintenance minors

Thanks all

Status: Fixed » Closed (fixed)

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