BreadcrumbManager ignores cacheability when no builders apply

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?

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

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

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!

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.

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

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 :)

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

Few minor nitpicks but looks good to me otherwise.

Thanks for the reviews! Should be good again now 👍

LGTM

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

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

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

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

larowlan → closed merge request !9913

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

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