| Issue category | Bug because this uses a cache key in a place where a cache context should be used. |
|---|---|
| Issue priority | Major because it's important that D8 core sets the right example for contrib developers WRT cache contexts. Part of the critical #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance meta. |
| Disruption | Zero disruption. Only a small bit of disruption in the 0.001% chance that somebody in already written D8 custom or contrib code is actually altering the book navigation block. |
See #2329101-19: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations & #2329101-21: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations.
While working to solve that, I opened #2428563: Introduce parameter-dependent cache contexts, only to discover that the BookNavigationBlock actually doesn't require a parameter; it derives from the request context:
Therefore, this can be converted into a cache context without requiring a parameter.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | book_navigation_cache_context-2429261-8.patch | 4.28 KB | wim leers |
Comments
Comment #1
wim leersComment #3
wim leersComment #4
wim leers#1 didn't apply because it's rolled on top of #2329101: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations. Manually changed a few lines to make it apply.
Comment #5
wim leersI think Fabianx's comment at #2428563-8: Introduce parameter-dependent cache contexts applies here too:
Comment #6
wim leersComment #7
wim leersNote that this issue is similar/related to #2318437: Replace the hardcoded langcode key on blocks with the 'language' cache context in that that issue also replaced a cache key that didn't really identify "the thing", but that indicated a variation. If the cache key serves to indicate a variation, it actually should be a cache context.
Comment #8
wim leers#2329101: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations landed. Reroll for that.
Comment #9
fabianx commentedThis just moves code, so RTBC
Comment #10
alexpottThis issue is a major task. To evaluate whether we should make this change during the beta, we need to update the issue summary according to https://www.drupal.org/core/beta-changes to explain what improvements it makes and what the disruption of the change would be. Can someone add Drupal 8 beta phase evaluation template to the issue summary.
Comment #11
wim leersComment #13
catchCommitted/pushed to 8.0.x, thanks!