Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add extra Caching context for breadcrumbs to avoid Drupal 10/11 changes in breadcrumb manager #328

Open
DiegoPino opened this issue Jun 10, 2024 · 1 comment
Assignees
Labels
Danger Mr Robinson Things so core to us that need extra care. Please submit automated tests? Drupal 10 Upgrade Economy (capitalism) External Bug We can help! release duties That thing we do every few months where we loose some brain cells UI/UX Experience
Milestone

Comments

@DiegoPino
Copy link
Member

What?

Drupal is such a hack sometimes. The story is quite simple. Breadcrumbs have two functions, one that is called to see if a Breadcrumb "applies" to the current Route and one to actual calculate the breadcrumb. Our own ADO implementation uses Node Context and the parent ADOs are extra cache tags when processing, but when the block is set without restrictions (at the block configuration level, e.g show on all pages) our "applies" function will Many times simply not generate any breadcrumbs. When that happens, the Block itself will get cached globally without any Caching context making any ADO breadcrumbs for anonymous users to end being "empty" (basically never running).

This is related to a "performance" improvement added for Drupal 10 that uses the "parent" route as cache context, which basically means, specially for "aliased ADOS... like /my-pretty-collection" to have the same cache context as the home page or anything else. Also reason why this almost never happens in the wild in our Archipelago deployments bc we put the breadcrumb block using conditionals and we don't use aliases (/do/uuid has a pre-folder named /do that aids with processing caches)

The solution (site building one) is to have a block setup that only acts on our Bundles to ensure that specific block does not share cache context with the "aliases/core" breadcrumb strategy as we normally do, but the actual solution on our side is to first always let the "apply" return TRUE... even if we know it does not.. and once we end with the actual "we can process" call, make the decision there if we try to calculate or not, but even if not, add

 $breadcrumb->addCacheContexts(['route', 'url.path', 'languages']);

Before returning the breadcrumb, overriding Drupal's new super strategy that basically breaks non-folder based caching

See as reference:
https://www.drupal.org/project/drupal/issues/2719721
https://www.drupal.org/forum/support/post-installation/2024-03-15/breadcrumb-block-shows-only-homepage-link-in-drupal-10

@DiegoPino DiegoPino added release duties That thing we do every few months where we loose some brain cells External Bug We can help! Danger Mr Robinson Things so core to us that need extra care. Please submit automated tests? UI/UX Experience Drupal 10 Upgrade Economy (capitalism) labels Jun 10, 2024
@DiegoPino DiegoPino added this to the 1.4.0 milestone Jun 10, 2024
@DiegoPino DiegoPino self-assigned this Jun 10, 2024
@DiegoPino
Copy link
Member Author

As additional reference. The Path Based Breadcrumb builder (\Drupal\system\breadcrumb) which will be called if ours does not match:

Does this:

 $breadcrumb = new Breadcrumb();
    $links = [];

    // Add the url.path.parent cache context. This code ignores the last path
    // part so the result only depends on the path parents.
    $breadcrumb->addCacheContexts(['url.path.parent', 'url.path.is_front']);

    // Do not display a breadcrumb on the frontpage.
    if ($this->pathMatcher->isFrontPage()) {
      return $breadcrumb;
    }

    // General path-based breadcrumbs. Use the actual request path, prior to
    // resolving path aliases, so the breadcrumb can be defined by simply
    // creating a hierarchy of path aliases.
    $path = trim($this->context->getPathInfo(), '/');
    $path_elements = explode('/', $path);
    $exclude = [];
    // Don't show a link to the front-page path.
    $front = $this->config->get('page.front');
    $exclude[$front] = TRUE;

Notice how the "parent" URL (in the case of an alias like /my-pretty-collection is basically the whole site) will be used as context, basically making any future processing of breadcrumbs for anonymous users to be a cached/empty response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Danger Mr Robinson Things so core to us that need extra care. Please submit automated tests? Drupal 10 Upgrade Economy (capitalism) External Bug We can help! release duties That thing we do every few months where we loose some brain cells UI/UX Experience
Projects
None yet
Development

No branches or pull requests

1 participant