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
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 3254714.patch | 448 bytes | zaporylie |
Comments
Comment #2
zaporylieA patch that matches the proposed resolution.
Comment #3
shelaneComment #4
zaporylieI 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.
Comment #5
shelaneThe 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.
Comment #6
zaporylieThis 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.siteSee 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.
Comment #7
shelaneOK. 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?
Comment #8
zaporylieI 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.
Comment #11
shelane