Problem/Motivation

Bootstrap adds url cache context if breadcrumb block is present on the page (Introduced in #2722093: Cache issue for page title append to breadcrumb). This is an issue ex. when campaign tracking unique query arguments are added to the url, since the page gets cached for each url variant.

Steps to reproduce

- turn on the cache debug with `http.response.debug_cacheability_headers: true` in services.yml
- place breadcrumb block on the page and visit the page
- go to the browser's inspector network tab and check that the X-Drupal-Cache-Contexts response header contains `url` cache context
- refresh the page
- see the X-Drupal-Dynamic-Cache is HIT
- add ?test=test to url and load the page with inspector open
- see the X-Drupal-Dynamic-Cache is MISS

Proposed resolution

At the very least replace url cache context with the url.path

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#2 3254714.patch448 byteszaporylie

Comments

zaporylie created an issue. See original summary.

zaporylie’s picture

Status: Active » Needs review
StatusFileSize
new448 bytes

A patch that matches the proposed resolution.

shelane’s picture

Status: Needs review » Closed (duplicate)
Related issues: +#3240459: Breadcrumbs title cache issue - Breadcrumb title stuck on "Access Denied"
zaporylie’s picture

Status: Closed (duplicate) » Needs review

I don't believe this issue is a duplicate of the linked issue. This issue is about replacing url cache context with url.path but the issue you've linked only extended list of cache contexts.

shelane’s picture

Status: Needs review » Closed (won't fix)

The patch that you provided for this issue changes the contexts in line 55 to:

$variables->addCacheContexts(['url.path']);

The patch that was provided in the related issue made this change to the same line:

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

I'm not sure how they can NOT be related when it's the same line of code asking to be changed. I see that you're wanting to replace, but the extended context was just as valid of an issue.

zaporylie’s picture

Status: Closed (won't fix) » Needs review

This issue is about limiting the caching scope, while #3240459: Breadcrumbs title cache issue - Breadcrumb title stuck on "Access Denied" is actually doing quite the opposite.

1. Having both url and url.path cache contexts is redundant. If url.path were replacing url (as suggested in THIS issue) then it would make sense - the scope would be limited to the path only, but since url cache context includes the scope of url.path and extends it then having the former and the latter together makes very little sense.

url = url.path + url.query_args + url.site
See documentation

2. Those 2 issues are only related because they are touching the same line, but the scope for both issues is separate.

3. The problem where this issue originates is that caching is insufficient. Any query arg added to the path makes the cache invalid and creates a new cache entry which is a problem for three reasons:
- not pulling data from the cache is expensive
- building new cache entry is more expensive than if the cache was disabled altogether
- in the cache backend with constrained memory (ex. redis) such cache entries which can be only used once just take up the space

But if in your opinion #3240459: Breadcrumbs title cache issue - Breadcrumb title stuck on "Access Denied" solves the problems I have now highlighted please don't hesitate to explain how.

shelane’s picture

OK. I see your point on the url and url.path. So, do you feel that having route and languages as context would cause any of those listed problems?

zaporylie’s picture

I don't see how adding route or language context prevents THIS issue from happening, hence I would definitely not call it a duplicate and close as such. I don't negate the need for those contexts as I didn't have time to fully review the linked issue.

  • shelane committed c1d09af on 8.x-4.x authored by zaporylie
    Issue #3254714 by zaporylie: Limit the cache scope for the breadcrumbs...

  • shelane committed e59ad6b on 8.x-3.x authored by zaporylie
    Issue #3254714 by zaporylie: Limit the cache scope for the breadcrumbs...
shelane’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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