Since RouteCacheContext / RouteNameCacheContext implement CacheContextInterface, it could make sense to declare it as on the other cache contexts.

They appear to have been changed last on #2432837: Make cache contexts hierarchical (e.g. 'user' is more specific than 'user.roles').

CommentFileSizeAuthor
#7 0001-Fixed-missing-CacheContextInterface-on-RouteCacheCon.patch1.56 KBfgm
#2 2459975-route-cache-context.patch1014 bytesfgm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Title: Minor, but since RouteCacheContext / RouteNameCacheContext implement CacheContextInterface, it could make sense to declare it as on the other cache contexts. » RouteCacheContext / RouteNameCacheContext implement CacheContextInterface
fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Status: Active » Needs review
FileSize
2459975-route-cache-context.patch1014 bytes

Checking.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Quickfix

+1

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Reviewed & tested by the community » Needs review 
  1. +++ b/core/lib/Drupal/Core/Cache/RouteCacheContext.php
@@ -12,7 +12,7 @@
-class RouteCacheContext {
+class RouteCacheContext implements CacheContextInterface {

    OMG this was just a straight-up bug. Great find! 

  2. +++ b/core/lib/Drupal/Core/Cache/RouteNameCacheContext.php
@@ -10,7 +10,7 @@
-class RouteNameCacheContext extends RouteCacheContext {
+class RouteNameCacheContext extends RouteCacheContext implements CacheContextInterface {

    Why this though? AFAIK We never repeat an "implements interface" statement when extending a class?

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented

That's what I thought initially and why I didn't repeat the "implements" in RNCC since it was already in RCC. However, our practices on this issue appear to vary. Consider just the Cache API itself, copied from the source:

  • UserCacheContext implements CacheContextInterface
  • AccountPermissionsCacheContext extends UserCacheContext implements CacheContextInterface

Seeing how this "implements" was repeated, I trod the same path, not seeing any standard for this question. We might want one, BTW: currently, all we have on interface use appears to be found at https://www.drupal.org/node/608152#interfaces

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Great find :) But AccountPermissionsCacheContext used to implement \Drupal\Core\Cache\CalculatedCacheContextInterface in an earlier iteration of the patch, then was refactored. So let's just remove that extraneous implements CacheContextInterface in AccountPermissionsCacheContext? :)

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
FileSize
0001-Fixed-missing-CacheContextInterface-on-RouteCacheCon.patch1.56 KB

OK, then : rerolled.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Alright

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

This issue is a minor bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 9db6e4e and pushed to 8.0.x. Thanks!

  • alexpott committed 9db6e4e on 8.0.x 
    Issue #2459975 by fgm: RouteCacheContext / RouteNameCacheContext...

Status: Fixed » Closed (fixed)

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