Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
4.39 KB

Status: Needs review » Needs work

The last submitted patch, 1: book_navigation_cache_context-2429261-1.patch, failed testing.

Wim Leers’s picture

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
4.43 KB

#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.

Wim Leers’s picture

Status: Needs review » Needs work

I think Fabianx's comment at #2428563-8: Introduce parameter-dependent cache contexts applies here too: . I think making those container aware is better and loading things on demand.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
4.32 KB
2.84 KB
Wim Leers’s picture

Title: Replace the hardcoded key on the book navigation block with a 'book navigation' cache context » Replace the hardcoded cache key on the book navigation block with a 'book navigation' cache context
Related issues:

Note 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.

Wim Leers’s picture

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

This just moves code, so RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

This 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.

Wim Leers’s picture

Category: Task » Bug report
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

  • catch committed 9f0db32 on 8.0.x
    Issue #2429261 by Wim Leers: Replace the hardcoded cache key on the book...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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