Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Beta phase evaluation
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:
- whether the current route represents a book node at all
- and if so, determines where in the book it is
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 CreditAttribution: 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!