Problem/Motivation
MenuBasedBreadcrumbBuilder::build() turns every ID of the active trail into a plugin instance without checking that the plugin still exists:
foreach (array_reverse($this->menuTrail) as $id) {
$plugin = $this->menuLinkManager->createInstance($id);
The active trail is cached per route in the menu bin by \Drupal\Core\Menu\MenuActiveTrail, which extends CacheCollector. Menu link definitions, on the other hand, are read from the menu tree. The two can disagree: a cache entry may still name a link the manager can no longer resolve. createInstance() then throws a PluginNotFoundException and the whole page render fails.
Two things make the impact worse than it first appears.
It does not recover on its own. A page that cannot be rendered cannot be cached either, so every subsequent request repeats the render and the failure. On the site where we hit this, a public page stayed down for 16 hours, for anonymous visitors, until an administrator flushed the caches.
It is amplified by other modules. schema_metatag calls BreadcrumbManager::build() from hook_entity_view_alter() to emit BreadcrumbList structured data. On a page rendering many entities the breadcrumb builder runs many times per request, so a single stale ID breaks the page reliably.
Evidence from production. Logged on a large Drupal 11 site running 2.0.1: 51 occurrences over two days, all on the same URL, then nothing after a cache flush.
PluginNotFoundException: Plugin ID 'menu_link_content:ed0fd66a-...' was not found.
Drupal\Core\Menu\MenuLinkManager->getDefinition()
Drupal\Core\Menu\MenuLinkManager->createInstance()
Drupal\menu_breadcrumb\MenuBasedBreadcrumbBuilder->build()
Drupal\Core\Breadcrumb\BreadcrumbManager->build()
...\schema_metatag\...\BreadcrumbList->getItems()
- First occurrence: an authenticated editor, right after menu changes.
- Then anonymous visitors, repeatedly, all night.
- Stopped at the exact minute the caches were flushed.
- The database was consistent afterwards: as many menu links as menu tree rows, no orphan either way. Nothing was corrupted; a cache entry simply outlived the link it named.
- The link itself is gone for good: the plugin's UUID appears nowhere in a database dump taken five days later, not in
menu_link_content, not inmenu_tree, not in the revision tables. So this was a deleted link, not a definition transiently missing while the menu tree was being rebuilt.
What we cannot show is when the deletion happened, since the log table is not part of that dump, so we cannot prove the deletion and the failing render overlapped. The timing suggests it, but it stays a hint rather than a demonstration.
Affected versions. The unguarded call is unchanged in 2.0.0-alpha0, 2.0.0, 2.0.1 and in the current 2.0.x branch, so updating does not help. The patch is against 2.0.x.
Steps to reproduce
We were not able to reproduce this by hand. Drupal invalidates config:system.menu.<menu> when a link is deleted, and CacheCollector::updateCache() already backs out when the entry it loaded has disappeared meanwhile:
elseif ($this->cacheCreated) {
// ...some other process must have cleared it. We back out...
return;
}
That guard only applies when the request found an entry on start-up. A request that begins on a cache miss has no cacheCreated, so it can still write a trail it computed before the deletion. That is our best hypothesis for how the stale entry survives, but we could not demonstrate it manually, and we would rather say so than dress up a guess.
The attached unit test reproduces the failure deterministically instead, by putting the builder in the state production reached: an active trail holding one live ID and one the manager no longer knows.
$this->menuLinkManager->method('hasDefinition')
->willReturnCallback(fn($id) => $id === $valid_id);
$this->menuLinkManager->method('createInstance')
->willReturnCallback(function ($id) use ($valid_id, $valid_plugin) {
if ($id !== $valid_id) {
throw new PluginNotFoundException($id);
}
return $valid_plugin;
});
$this->builder->setMenuName('main');
$this->builder->setMenuTrail([$valid_id, $stale_id]);
$breadcrumb = $this->builder->build($route_match);
Against 2.0.x as it stands, both tests fail with the production message:
1) ...\MenuTrailStalePluginTest::testStaleMenuLinkDoesNotBreakBreadcrumb
Drupal\Component\Plugin\Exception\PluginNotFoundException:
Plugin ID 'menu_link_content:ed0fd66a-...' was not found.
2) ...\MenuTrailStalePluginTest::testFullyStaleTrailYieldsEmptyBreadcrumb
Drupal\Component\Plugin\Exception\PluginNotFoundException:
Plugin ID 'menu_link_content:33333333-...' was not found.
ERRORS! Tests: 2, Assertions: 0, Errors: 2.
With the patch applied, the module's whole unit test directory passes:
OK (3 tests, 5 assertions)
Proposed resolution
Skip trail entries the manager cannot resolve rather than letting the exception escape. A breadcrumb missing one level is a cosmetic regression; a fatal error is not.
foreach (array_reverse($this->menuTrail) as $id) {
// The active trail is cached per route, while menu link definitions are
// read from the menu tree. A cached trail may therefore still name a
// link whose definition has gone, a deleted one for instance. Skip the
// missing level rather than let createInstance() throw a fatal
// PluginNotFoundException, which would take the whole page down for
// every visitor until the caches are flushed.
if (!$this->menuLinkManager->hasDefinition($id)) {
continue;
}
$plugin = $this->menuLinkManager->createInstance($id);
MenuLinkManagerInterface::hasDefinition() is part of the public interface, so no new dependency is introduced.
One consequence is worth naming, since it is what a reviewer would ask about: applies() only returns TRUE for a non-empty active trail, but with this guard every item of that trail can now be skipped, leaving no link at all. The second test covers that case and confirms the breadcrumb simply comes back empty, because the if (!empty($links)) already present in 2.0.x around the front page handling deals with it. Anyone backporting this fix to 2.0.1 should take that block from 2.0.x along with it, since there $links[0]->getUrl() is still unguarded.
Remaining tasks
- Review the approach.
- Commit the tests alongside the fix.
User interface changes
None in normal operation. When a link in the trail can no longer be resolved, that level is now missing from the breadcrumb for as long as the cached trail lives, instead of the page failing to render.
API changes
None.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| menu_breadcrumb-stale-active-trail-plugin-2.0.x.patch | 10.26 KB | pitop |
Issue fork menu_breadcrumb-3619434
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:
Comments
Comment #3
pitop commentedMR !43 adds the
hasDefinition()guard and two unit tests. Both fail on 2.0.x with the exact production message, and pass with the guard.The failure and the fix are covered by the tests, but the mechanism that leaves a stale ID in the cached active trail is still conjecture: we could not reproduce it by hand. Happy to dig further if a maintainer has a lead.
The three
phpstanerrors are pre-existing, inMenuBreadcrumbFunctionalTestBase.php, which this MR does not touch.Comment #5
xurizaemonthis looks good @pitop
keen for your thoughts on the open comment