@yched remarked in #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!), and many including myself have also noticed that the name for the cache contexts service is quite unfortunate. It can be pretty confusing when you have something like:

$cache_contexts = ['foo', 'bar'];
$keys = $this->cacheContexts->convertTokensToKeys($cache_contexts);

It'd be better if it had a more distinct name. What about a "manager" suffix? It'd then look like this:

$cache_contexts = ['foo', 'bar'];
$keys = $this->cacheContextsManager->convertTokensToKeys($cache_contexts);
CommentFileSizeAuthor
#15 2468151-15.patch29.48 KBlhangea
#10 2468151-10.patch29.54 KBrpayanm
#10 2468151-interdiff.txt4.26 KBrpayanm
#7 rename_cache_contexts_service-2468151-7.patch29.09 KBlhangea
#5 2468151-5.patch27.7 KBPalashvijay4O

Comments

yched’s picture

yched commented

+1, thanks for opening that issue
(actually I made that comment in #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) without really realizing the CacheContext class was pre-existing HEAD code, my bad)

AFAIK we tend to err between XxxManager and XxxHandler for "class that does hard-to-describe-specifically stuff about Xxx". Not sure in which category that one here is - does it handle, does it manage, that's shakespearian...

At some point I tried to push for consistently naming plugin managers "Xxx*Plugin*Manager", but that failed, so we have plugin managers that are just XxxManager. But we also have YyyManager classes that are not plugin managers, so I guess anything goes :-)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#1: heh :)

In this case, this service is aware of all services tagged with "cache context", and allows developers to easily use those cache context services for typical tasks such as gathering all labels, converting a set of cache contexts into the corresponding keys, and optimizing the cache contexts (removing unnecessary cache contexts by looking at the hierarchy information).

Maybe that description helps you think of a better suffix than "manager"? It didn't help me to think of a better name unfortunately :)

yched’s picture

yched commented

yep, that sounds like "managing" to me :-)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Novice, +php-novice

Alright. :)

Palashvijay4O’s picture

Palashvijay4O commented
Status: Active » Needs review
StatusFileSize
new 2468151-5.patch27.7 KB

A patch. Please review!

Status: Needs review » Needs work

The last submitted patch, 5: 2468151-5.patch, failed testing.

lhangea’s picture

lhangea commented
StatusFileSize
new rename_cache_contexts_service-2468151-7.patch29.09 KB

Let's try with this one

lhangea’s picture

lhangea commented
Status: Needs work » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Cache/CacheableDependencyInterface.php
@@ -24,14 +24,14 @@
+   * the @cache_contexts_manager service. The replacement value depends on the request

    80 cols. 

  2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -92,18 +92,18 @@ class Renderer implements RendererInterface {
    *   The cache contexts service.

    s/service/manager/

    Here, and everywhere else. 

  3. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php
@@ -40,7 +40,7 @@ class RendererTestBase extends UnitTestCase {
   protected $cacheContexts;

    You forgot to update this one :)

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
StatusFileSize
new 2468151-interdiff.txt4.26 KB
new 2468151-10.patch29.54 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

The interdiff is incomplete: it doesn't show that #9.3 is fixed, even though it is :) Thanks!

yched’s picture

yched commented

This novice patch single-handedly fixes the two most complicated things in CS : naming things and cache invalidation, awesome win ;-)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2468151-10.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Needs reroll

This novice patch single-handedly fixes the two most complicated things in CS : naming things and cache invalidation, awesome win ;-)

:D :D :D

lhangea’s picture

lhangea commented
Status: Needs work » Needs review
StatusFileSize
new 2468151-15.patch29.48 KB

Re-rolled.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

Thanks!

fabianx’s picture

fabianx commented

RTBC + 1

  • catch committed 7c88cb7 on 8.0.x 
    Issue #2468151 by lhangea, rpayanm, Palashvijay4O: Rename the...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

This is technically an API change, but it's an API that I wouldn't expect any modules to be using at all and it was only recently added.

So committed/pushed to 8.0.x, and I think a change notice is unnecessary (although probably doesn't harm if someone wanted to add one).

Status: Fixed » Closed (fixed)

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