Make cache contexts hierarchical (e.g. 'user' is more specific than 'user.roles')

Regarding hierarchy, it's a bit counter-intuitive that 'user' is more granular than 'user.roles'. From the naming you'd expect the opposite (foo.bar looks more specific than foo)

Yes that's been in the back of my mind as well, I think it goes both ways though.

Specifically in terms of user roles, it's easy to see that 'user' includes 'user.roles' - i.e. that roles is a property of user. Therefore if you vary on user you're also varying by role.

I'm wondering if that works for enough other things though - say page and page.active_trail? Then timezone - iirc it's possible to prevent users from selecting a timezone so that this would always be the timezone of the site.

Another thing here, I had a brief irc discussion with Fabianx about (longer term) exposing some of this logic to reverse proxies (i.e. for authenticated user caching in Varnish).

If we're able to group cache contexts into things like page vs. user, then that ought to help reverse proxy implementations figure out which are handled by the path and which aren't.

Bumping to major as that is a big enabler of:

a) Caching at HttpMiddleware level (authenticated user)
b) ESI / auto-AJAX-replacement -- The less cache contexts, the easier the request logic of those.
c) Cache context comparison.

So authenticated user caching in Varnish in reality also solves most of the problems around ESI (at least those around the cache granularity).

The same lazy-two-tier-approach works for Varnish, too! :)

An ESI request is just a request that does not have 'cache_context.page', which is req.url in 'varnish speak'.

I think first of all we should name them like that, because in the end this is like class chaining:

Possible core examples

- user
- user.roles
- user.roles.has_role:'administrator'
- user.access_grants
- user.timezone (TZ is configured by a user in Drupal even if its the same always)
- user.language (the users preferred language, when e.g. user language neg is used on a content piece)

- url
- url.pager:0
- url.menu.active_trail:'tools'
- url.query_parameter:'campaign_code'
- url.book_navigation (While other schemes are possible the core scheme is to variate the book navigation by url.)

- url.main_request (need to call the main route for page assembling)

- url.route (hash of the route with parameters, used e.g. by SmartCache)
- url.route.format (negotiated format after routing, e.g. 'json', 'dialog', etc.)
- url.route.language (negotiated language derived from the url after routing)

( Here route is a sub-set of url based on path based language and content negotiation with the exception of allowing uncacheable pages to be everything. )

Possible Contrib examples

- user.organic_groups.has_og:'some-group'

- request (Hash of whole incoming request, not that useful)
- request.cookie:'device'
- request.header:'X-Drupal-Device'

Conclusion

Naming things appropriately is probably the easiest to show the relationships and then 'parent' is probably fine.

Yes, the . based hierarchy makes sense to me, and we have similar patterns elsewhere, eg in validation message paths.
So given this varies from the pure service tags approach, where do we keep the logic - will you just use simple strpos === 0 style comparisons or do you need more complex helper methods?

The longer names seem OK.

In irc Wim suggested:

- rename existing cache contexts to match the tree
- follow-up issues to add missing contexts

Also seems OK.

#11.6: I am all for long names, because while discoverability is not as good as for long classnames (where you have auto completion - "What IDE, I don't need an IDE. I have ex!'), we are generally moving towards:

- explicit is better than implicit

in core.

Also for language e.g. we have a user.language cache context and a url.route.language cache context, where this makes it also more clear.

But the session doesn't necessarily come from a cookie ? (er, or does it ?)

Also, discoverability is a concern. How do I find the correct full tree name for "cache by roles" ?

#14: Yes, it does in Drupal :).

#15: You look into either the services tree like for any other service or read the cache context documentation that we start in a sister issue.

Also user.roles is not more magical, than what we currently have, which is, uhm, 'user.roles'.

So my proposal was to document the whole tree, but don't have:

request.cookies.session.user.roles

But start a new tree with 'user' as user is a really special concept.

So the two root 'aliases' would then be:

- url (special, aliased from request.url)
- user (special, aliased from request.cookies.session.user)

Both are special concepts for a CMS like Drupal.

The two natural root aliases are:

- server (like server.value:HTTP_HOST)
- request (like request.header)

--

So my long names proposal is actually a:

- as long as needed for practical purposes proposal

and to shorten the hierarchy, where we don't have something meaningful as parents that can be cached.
(and while session might be valid at times, I don't feel it deserves it's own cache context, because it gives horrible cacheability).

--

TL;DR:

Have 4 root elements:

- server
- request

- user
- url

for the cache context hierarchy for practical purposes, have child elements of those use the '.' notation.

Also see #8 for concrete examples.

Note that this roughly matches what varnish has internally as variables (this comes naturally as both describe the hierarchy of headers and values):

e.g.

request == req
server == req.backend.value (IIRC)
url == req.url
user == // There is no concept of a user in varnish.
request.cookie == req.http.cookie (with regexp to filter out)
request.header:X == req.http.X

Is there any chance we need a cache context for the actual route as a thing as opposed to route + params?

#25: If you mean the route name, the context for that would be url.route.name instead.

url.route.name is what I was thinking of. And indeed url.route is the parent of that so that works fine.

+++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
@@ -147,7 +158,7 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
-      'X-Drupal-Cache-Contexts' => implode(' ', $cache_contexts),
+      'X-Drupal-Cache-Contexts' => implode(' ', $this->cacheContexts->optimizeTokens($cache_contexts)),

Can be follow-up, but someone / something should set the 'url' cache context, so that things get optimized out that only depend on url.

Also means the page cache has the 'url' cache context.

--

Continuing review later ...

1. +++ b/core/lib/Drupal/Core/Cache/IsSuperUserCacheContext.php
   @@ -0,0 +1,29 @@
   +  public function getContext() {
   +    return $this->user->id() === 1;
   +  }
   

   I think better to do:

   return 's.' . $this->user->id() === 1 ? 1 : 0;

   Else you end up with:

   '::' potentially, which is confusing.

2. +++ b/core/lib/Drupal/Core/Cache/LanguagesCacheContext.php
   @@ -41,17 +41,22 @@ public static function getLabel() {
   +    if ($type === NULL) {
   ...
   -      $context_parts[] = $this->languageManager->getCurrentLanguage()->getId();
   +      return $this->languageManager->getCurrentLanguage($type)->getId();
   

   I think it would be great to type-hint somewhere that this is of one of the types defined in:

   Drupal\Core\Language\LanguageInterface

3. +++ b/core/lib/Drupal/Core/Cache/LanguagesCacheContext.php
   @@ -41,17 +41,22 @@ public static function getLabel() {
   +      else {
   +        $context_parts[] = $this->languageManager->getCurrentLanguage()->getId();
   +      }
   

   So if its not multi-lingual, then we use LanguageInterface::TYPE_INTERFACE, which is the default.

4. +++ b/core/lib/Drupal/Core/Cache/LanguagesCacheContext.php
   @@ -41,17 +41,22 @@ public static function getLabel() {
   +      return implode(':', $context_parts);
   

   I think we should delimite with '.' or ',' here as its not a new CID part.

5. +++ b/core/lib/Drupal/Core/Cache/PagersCacheContext.php
   @@ -22,7 +22,11 @@ public static function getLabel() {
   +    if ($pager_id === NULL) {
   +      return 'pager' . $this->requestStack->getCurrentRequest()->query->get('page', '');
   +    }
   

   This could use a comment that 'page' contains all pagers on a page separated by comma, or an @see pager_find_page() or such.

   It confused me first.

6. +++ b/core/lib/Drupal/Core/Cache/QueryArgsCacheContext.php
   @@ -0,0 +1,40 @@
   +      parse_str($this->requestStack->getCurrentRequest()->getQueryString(), $query_args);
   +      return $query_args[$query_arg];
   

   This needs an isset() check.

7. +++ b/core/modules/block/src/BlockViewBuilder.php
   @@ -85,7 +85,7 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
   -          'language',
   +          'languages',
   

   Are we sure the block depends on all language types?

   Not know too much about this, just wondering.

8. +++ b/core/modules/filter/src/Tests/FilterAPITest.php
   @@ -259,7 +259,7 @@ function testProcessedTextElement() {
   -      'language',
   +      'languages:content',
   

   According to https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Language%... this needs to be language_content.

Could find nothing more, RTBC if tests pass :).

We need another issue for e.g. user roles to consider throwing an exception, too if the argument is invalid to prevent:

user.roles:auhtenticated

or other typos :).

Great work!

Back to RTBC

RTBC + 1

1. +++ b/core/lib/Drupal/Core/Cache/CacheContexts.php
   @@ -118,6 +119,69 @@ public function convertTokensToKeys(array $context_tokens) {
   +   * - ['a', 'a.b', 'a.b.c'] -> ['a.b']
   

   Why is this a.b and not a? I could be plain wrong here.

2. +++ b/core/lib/Drupal/Core/Cache/CacheContexts.php
   @@ -118,6 +119,69 @@ public function convertTokensToKeys(array $context_tokens) {
   +      // - a period mean they don't have a parent
   +      // - a colon mean they're not a specific value of a cache context
   

   nitpick, should this be means - not mean

3. +++ b/core/lib/Drupal/Core/Cache/CacheContexts.php
   @@ -118,6 +119,69 @@ public function convertTokensToKeys(array $context_tokens) {
   +      // hence no  optimizations are possible.
   

   nitpick two spaces

4. +++ b/core/lib/Drupal/Core/Cache/IsSuperUserCacheContext.php
   @@ -0,0 +1,29 @@
   +    return $this->user->id() === 1 ? '1' : '0';
   

   I know in comment we had issues with $comment->getOwnerId() returning '1' as in a string, we had to add a cast to int in that method.
   We might need this here too.

5. +++ b/core/lib/Drupal/Core/Cache/RouteCacheContext.php
   @@ -0,0 +1,48 @@
   +    return $this->routeMatch->getRouteName() . serialize($this->routeMatch->getRawParameters()->all());
   

   could we hash this to reduce the size of the context? Just checking we definitely don't unserialize this, as that'd be a sechole.

6. +++ b/core/lib/Drupal/Core/Cache/RouteNameCacheContext.php
   @@ -0,0 +1,28 @@
   +class RouteNameCacheContext extends RouteCacheContext {
   +  /**
   

   Nitpick missing blank line

7. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheContextsTest.php
   @@ -20,6 +20,63 @@
   +      [['a', 'a.b', 'a.b.c'], ['a']],
   

   This would indicate the comment earlier (see point 1) is incorrect.

Great review, back to RTBC.

Needs work for:

1. +++ b/core/lib/Drupal/Core/Cache/QueryArgsCacheContext.php
   @@ -0,0 +1,40 @@
   + * Defines the QueryArgsCacheContext service, for "per host" caching.
   + *
   + * A "host" is defined as the combination of URI scheme, domain name and port.
   + *
   + * @see Symfony\Component\HttpFoundation::getSchemeAndHttpHost()
   

   What does "host" have to do with QueryArgs?

2. +++ b/core/lib/Drupal/Core/Cache/RequestStackCacheContextBase.php
   @@ -0,0 +1,38 @@
   + * Defines the HostCacheContext service, for "per host" caching.
   + *
   + * A "host" is defined as the combination of URI scheme, domain name and port.
   + *
   + * @see Symfony\Component\HttpFoundation::getSchemeAndHttpHost()
   + */
   +abstract class RequestStackCacheContextBase implements CacheContextInterface {
   

   Wrong docs for this class.

Additionally, but could be follow-up:

1. +++ b/core/core.services.yml
   @@ -6,26 +6,88 @@ parameters:
   +  # Simple cache contexts, directly derived from the request context.
   

   url.route and its children aren't exactly "simple" and "directly derived", so I think could be given a separate section with their own comment.

2. +++ b/core/core.services.yml
   @@ -6,26 +6,88 @@ parameters:
   +  cache_context.url.pagers:
   

   Why is 'pagers' at this level and not under 'query_args'? I suppose this allows an alternate implementation that uses some other portion of the URL, but if that's the intent, or if there's a different intent, let's add a comment about that.

And on a more substantial note, overall I like the naming approach a lot. But, for things like cache_context.url.route.menu_active_trails and cache_context.url.route.format, it means we have an implicit requirement that implementations of those services must only depend on the URL. So, no ability for FancyMenuActiveTrails to depend on user or phase of moon, and no ability for contrib to implement header-based format negotiation. In other words, what we're sacrificing by using service ID to have hierarchy meaning is the ability to override the service implementation with something that would put it in a different part of the hierarchy. Is that something we're willing to accept? Or, should we address it via something like using an optional service tag or a service alias? For example, if contrib implements a FancyMenuActiveTrail override of core's MenuActiveTrail, then it can also tag or alias the cache_context.url.route.menu_active_trails service in a way that makes the system (e.g., CacheContexts::optimizeTokens()) aware that its parent is no longer url.route.

[Edit: BTW, I'm fine to punt this to a follow-up.]

How about this to address #74: we embed the hierarchy in the context name / service ID only when that hierarchy is intrinsic to the meaning of the context, not merely an implementation choice.

What I mean by that is url.host is a good name, because the semantics of "host" makes it intrinsically a child of "url". Same for url.query_args and user.roles.

However, I suggest that timezone, format, and pagers each move to the root level, because those concepts are not intrinsically bound to users, routes, and URLs, respectively.

Additionally, I think route should move to the root level as well. Routing could in principle depend on IP and headers, even if we don't want core's to do so.

I'm unsure about book_navigation and menu_active_trails. I think they might make sense to keep under route (i.e., route.book_navigation and route.menu_active_trails), because I don't think it makes semantic sense for those things to vary on anything other than the route. However, I suspect there are sites that customize breadcrumbs and the rendered structure of some menus based on things like role or organic group, but what I wonder is whether it ever makes sense to implement such customizations via the return value of getActiveTrailIds() or whether such customizations should only ever be done in the display layer (which would then also be responsible for adding the additional contexts there). If there are legitimate use cases for the former, then those context names probably need to move to the root level as well (or we can group them under something like navigation).

For this issue, I think it would be sufficient to simply rename accordingly.

In a followup, I think we can add a service tag to convey an implementation-driven parent relationship. For example:

  cache_context.pagers:
   ...
    tags:
      - { name: cache.context, parent: cache_context.url.query_args:page }
  cache_context.route:
    ...
    tags:
      - { name: cache.context, parent: cache_context.url }

And thereby allow CacheContexts::optimizeTokens() to optimize for implementations where pagers are solely determined by a single URL query argument and routing is done exclusively by URL.

While I can understand the point of not making everything alterable, I think encoding hierarchy into the service IDs is wrong,
because its a new pattern we came up with. ... What about things like module.installer, there is no hierarchy, its simply a group of things.

Tags are the way how you add additional metainformation to the services.

To #77 for the route argument:

If a site chooses to alter the meaning of url.route and exchange the cache context, so its dependent on more than the url naturally they will need to exchange the 'url' cache context as well.

Even if there was no hierarchy and it was not simplified, as soon as something uses 'url' instead of route, but this url depends on something else in the request it is wrong.

That is why core enforces an url based hierarchy on things that is true for core.

Because with this we give the guarantee of cacheability in external proxies.

Therefore it is good that we choose to have url.route as dependent on the url, because it gives clarity that you need to exchange both to get correct and proper caching.

--

Let's go through the list again:

cookies -- dependent on request, straightforward OK
headers -- dependent on the request headers, OK
ip -- a property of the request, OK
languages -- a top level drupal concept, needs special casing, OK
  :type
theme -- a top level drupal concept, can be added by anything that renders, it is dependent on the route in Core
url -- dependent on request, OK
  .host -- dependent on request, OK
  .query_args
    :key
    .pagers -- Core's implementation is bound to query_args, the implementation has the cache contexts, OK
      :pager_id
  .route -- Core's route is determined out of the URL for cacheability reasons in external proxies, redirects are not cached, OK (see above).
    .book_navigation -- Interfacing on the route, if you need another navigation, switch out the navigation AND the context, OK
    .format -- determined by the route negotiation, OK
    .menu_active_trails -- Interfaced on route, if you switch out the implementation, switch out the implementation and the contexts, OK
      :menu_name
    .name -- determined by the route, OK
user -- Arbitrary Drupal concept, OK
  .is_super_user -- Arbitrary Drupal concept, OK
  .node_grants -- Grants are per user in core, other things use other contexts, OK
    :operation
  .roles -- A user has one or more roles, OK
    :role
  .timezone -- Timezone is in Core a property of the user, but timezone being a global thing, it might be tricky, SO SO

So therefore timezone from this list remains as its added by e.g. a DateFormatter and is understood as a global concept.

And maybe explicit parent relationship as a tag is the right thing for timezone and theme, so it can still be simplified in core. (though that could indeed be follow-up).

Because usually timezone is user and given:

[timezone, user] can be simplified to user, but that might not always be true.

Similarly theme can easily be simplified to url.route in Core by default given the interface of the theme negotiator and we don't do this right now.

Will discuss timezone with Wim again.

To write #80 again from a different light (also for my own understanding).

1. Intrinsic, it is a property of the parent - always, mostly request based things.

+Main property: Not negotiated, but taken mostly raw.
+Swappability: It can be swapped, but there is no use case without also having to swap the parents in the hierarchy.

2. Implementation specific, the cache context is bound to a specific implementation and by swapping out the implementation the cache contexts are swapped out as well.

Those are cache contexts that are not negotiated, but hard coded to their specific implementation. (Core's pager do use ?page)

+Main property: You do not use this cache context outside of the things it is on. It is bubbled up.
+Swappability: You do not swap the cache context, but the implementation and use a different cache context.

3. Dependent on calculated state, the cache context is negotiated based on other properties that can change. Because it is a cache context that others specify it MUST be swappable.

+Main property: Complex logic to calculate, can depend on several other cache contexts itself.
+Swappability: MUST be swappable, because it is specified by something outside, e.g. a date formatter specifies a timezone CC.

These are the top level contexts and what #2453835: Allow non-intrinsic (implementation-dependent) cache context services to specify their parents via DynamicParentContextInterface::getParents() is for.

All feedback has been addressed, #76 was additionally discussed in IRC.

Therefore back to RTBC.

If a site chooses to alter the meaning of url.route and exchange the cache context, so its dependent on more than the url naturally they will need to exchange the 'url' cache context as well.

I don't follow this reasoning. If I want to write a contrib/custom module that determines a route parameter from a header (because I choose to not care about reverse proxy caching, or can configure my reverse proxy to vary by that header), why would that require changing the meaning or implementation of the 'url' context? Vary-by-URL still means exactly that. It's only that in that case, vary-by-route can't be treated as a strict subset of vary-by-url, so how to deal with that? I suppose #2453835: Allow non-intrinsic (implementation-dependent) cache context services to specify their parents via DynamicParentContextInterface::getParents() will allow that even if we preserve the context name as url.route?

Not un-RTBC'ing for above, since that can be debated in a followup. But, setting to NR for:
Should url.route.format be changed to url.format? Because a single route can serve multiple formats AND a single format can be used by many routes, so I don't see in which sense format is a child of route.

Yes, #2453835: Allow non-intrinsic (implementation-dependent) cache context services to specify their parents via DynamicParentContextInterface::getParents() will allow to set a parent explicitly even if the semantic meaning is changed then from our recommendation / choice in core.

Naming ain't easy :).

I also wondered before on url.route.format, but atm. format neg is done in middle ware (or at least soon), so yes, that is not a property of the route itself.

Thanks.

We are choosing to standardize on the URL as being the only thing that determines the route. It's a uniform resource locator, after all.

I promised not to un-RTBC this on this, so I won't, but I'd just like to make a final plea to reconsider putting format and route at the root levels, not inside url. Here's why: URL stands for uniform resource locator, not uniform representation locator. So I don't believe there is anything semantically that justifies thinking about format and route as children of url. I don't think '#contexts' => ['url.format'] is better DX over '#contexts' => ['format'], especially when 'languages' and 'theme' are at the root level, and at least superficially, format is kind of like those things.

I can appreciate the desire to optimize format and route away when we're caching on url, but we'll be able to do that in #2453835: Allow non-intrinsic (implementation-dependent) cache context services to specify their parents via DynamicParentContextInterface::getParents(), so I don't see why we need to make that implementation decision leak into the names.

#86: Thanks, I think our understanding of cache contexts has started to grow and as its a new concept it takes some tries to get right.

What I don't want is: All things are top-level, hence just a list as that is problematic for discoverability and DX.

I am pretty happy with my latest table / decision chart from #81 and I think thats a good addition to the developer docs on cache contexts.

Lets judge 'format' and 'route' based on that:

1. format is not intrisic itself as its not a simple property on the url, it is a calculated property of the request and in Drupal's implementation of the url, but we choose to omit 'request.' prefix due to shortening. It certainly fails 'not negotiated', but raw.

2. format would definitely be set by something else, like a request / response listener, hence it is not specific to the implementation of its thing. (though the cache contexts could live on the request object, but that is still != url).

3. format is negotiated and must be swappable for REST support in contrib.

=> Top level

route

- 1. Derived from the request, not a simple property, multiple routes per url
- 2. Smart Cache is after routing and wants to use this, so set by something else.
- 3. Definitely a negotiated context

=> Top level

A non-shortened tree would probably look like this:

languages -- a top level drupal concept, needs special casing, OK | Core Parent: url (???)
request -- the hash of the full incoming request
  .cookies -- dependent on request, straightforward OK
  .format -- property of the request ($request->getFormat()), OK
  .headers -- dependent on the request headers, OK
  .ip -- a property of the request, OK
  .url -- dependent on request, OK
    .host -- dependent on url, OK
    .query_args
      :key
      .pagers -- Core's implementation is bound to query_args, the implementation has the cache contexts, OK
        :pager_id
route -- Drupal concept, can also be negotiated theoretically by phase-of-the-moon | Core Parent: 'url'
  .book_navigation -- Interfacing on the route, if you need another navigation, switch out the navigation AND the context, OK
  .menu_active_trails -- Interfaced on route, if you switch out the implementation, switch out the implementation and the contexts, OK
    :menu_name
  .name -- determined by the route, OK
theme -- a top level drupal concept, can be added by anything that renders, it is dependent on the route in Core, | Core Parent: 'route'
timezone -- top level negotiated concept | Core Parent: 'user'
user -- Arbitrary Drupal concept, OK
  .is_super_user -- Arbitrary Drupal concept of a user, OK
  .node_grants -- Grants are per user in core, other things use other contexts, OK
    :operation
  .roles -- A user has one or more roles, OK
    :role

If we omit request, we should probably still use request_format instead of just 'format' as it is a little too common.

Without explanations:

languages
  :type
request
  .cookies
    :name
  .format
  .headers
    :name
  .ip
  .url
    .host
    .query_args
      :key
      .pagers
        :pager_id
route
  .book_navigation
  .menu_active_trails
    :menu_name
  .name
theme
timezone
user
  .is_super_user
  .node_grants
    :operation
  .roles
    :role

@Wim: Thoughts?

+1 to #87. Either option (with or without request as its own context / prefix) is fine with me. If we omit that prefix, I'm fine with either format or request_format.

Final decision, 1,2,3:

cookies
  :name
headers
  :name
ip
languages
  :type
request_format
route
  .book_navigation
  .menu_active_trails
    :menu_name
  .name
theme
timezone
url
  .host
  .query_args
    :key
    .pagers
      :pager_id
user
  .is_super_user
  .node_grants
    :operation
  .roles
    :role

And finally back to RTBC, see you in the follow-ups! (hopefully)

Looks great. Thanks!

Can we have a follow up to add test coverage for all the new cache contexts?

1. +++ b/core/lib/Drupal/Core/Cache/IsSuperUserCacheContext.php
   @@ -0,0 +1,29 @@
   +/**
   + * Defines the IsSuperUserCacheContext service, for "super user or not" caching.
   + */
   +class IsSuperUserCacheContext extends UserCacheContext {
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getContext() {
   +    return ((int) $this->user->id()) === 1 ? '1' : '0';
   +  }
   +
   

   I'm curios which behaviour relies on the admin user still? don't we rely on admin roles somehow already?

2. +++ b/core/lib/Drupal/Core/Cache/PagersCacheContext.php
   @@ -21,8 +21,16 @@ public static function getLabel() {
   +  public function getContext($pager_id = NULL) {
   +    // The value of the 'page' query argument contains the information that
   +    // controls *all* pagers.
   +    if ($pager_id === NULL) {
   +      return 'pager' . $this->requestStack->getCurrentRequest()->query->get('page', '');
   +    }
   

   Should we not instead fallback to the pager with element 0? I think this is more what is actually intended. On top we then would need 'all', but I think the usecases for that are really rare.

3. +++ b/core/lib/Drupal/Core/Cache/QueryArgsCacheContext.php
   @@ -0,0 +1,40 @@
   +      $query_args = [];
   +      parse_str($this->requestStack->getCurrentRequest()->getQueryString(), $query_args);
   +      return $query_arg . '.' . (isset($query_args[$query_arg]) ? $query_args[$query_arg] : '');
   

   Huch? Why do you not just use $request->query->get($query_arg)?

4. +++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
   @@ -1208,7 +1208,7 @@ public function getCacheContexts() {
        //   the information from there.
   -    $contexts[] = 'cache.context.url';
   +    $contexts[] = 'url';
    
        // Asks all subplugins (argument defaults, argument validator and styles).
        if (($plugin = $this->getPlugin('argument_default')) && $plugin instanceof CacheablePluginInterface) {
   diff --git a/core/modules/views/src/Plugin/views/argument_default/QueryParameter.php b/core/modules/views/src/Plugin/views/argument_default/QueryParameter.php
   
   diff --git a/core/modules/views/src/Plugin/views/argument_default/QueryParameter.php b/core/modules/views/src/Plugin/views/argument_default/QueryParameter.php
   index 337b722..feccd21 100644
   
   index 337b722..feccd21 100644
   --- a/core/modules/views/src/Plugin/views/argument_default/QueryParameter.php
   
   --- a/core/modules/views/src/Plugin/views/argument_default/QueryParameter.php
   +++ b/core/modules/views/src/Plugin/views/argument_default/QueryParameter.php
   
   +++ b/core/modules/views/src/Plugin/views/argument_default/QueryParameter.php
   +++ b/core/modules/views/src/Plugin/views/argument_default/QueryParameter.php
   @@ -95,7 +95,7 @@ public function isCacheable() {
   
   @@ -95,7 +95,7 @@ public function isCacheable() {
       * {@inheritdoc}
       */
      public function getCacheContexts() {
   -    return ['cache.context.url'];
   +    return ['url'];
      }
    
    }
   diff --git a/core/modules/views/src/Plugin/views/argument_default/Raw.php b/core/modules/views/src/Plugin/views/argument_default/Raw.php
   
   diff --git a/core/modules/views/src/Plugin/views/argument_default/Raw.php b/core/modules/views/src/Plugin/views/argument_default/Raw.php
   index 4239c50..40bddf3 100644
   
   index 4239c50..40bddf3 100644
   --- a/core/modules/views/src/Plugin/views/argument_default/Raw.php
   
   --- a/core/modules/views/src/Plugin/views/argument_default/Raw.php
   +++ b/core/modules/views/src/Plugin/views/argument_default/Raw.php
   
   +++ b/core/modules/views/src/Plugin/views/argument_default/Raw.php
   +++ b/core/modules/views/src/Plugin/views/argument_default/Raw.php
   @@ -124,7 +124,7 @@ public function isCacheable() {
   
   @@ -124,7 +124,7 @@ public function isCacheable() {
       * {@inheritdoc}
       */
      public function getCacheContexts() {
   -    return ['cache.context.url'];
   +    return ['url'];
      }
    
    }
   diff --git a/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   
   diff --git a/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   index b5f8bf8..9abf63e 100644
   
   index b5f8bf8..9abf63e 100644
   --- a/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   
   --- a/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   
   +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -1477,7 +1477,7 @@ public function getCacheContexts() {
   
   @@ -1477,7 +1477,7 @@ public function getCacheContexts() {
        // input from GET parameters, which are part of the URL. Hence a view with
        // an exposed filter is cacheable per URL.
        if ($this->isExposed()) {
   -      $cache_contexts[] = 'cache.context.url';
   +      $cache_contexts[] = 'url';
        }
        return $cache_contexts;
      }
   diff --git a/core/modules/views/src/Plugin/views/sort/SortPluginBase.php b/core/modules/views/src/Plugin/views/sort/SortPluginBase.php
   
   diff --git a/core/modules/views/src/Plugin/views/sort/SortPluginBase.php b/core/modules/views/src/Plugin/views/sort/SortPluginBase.php
   index 45b86be..8fe2b43 100644
   
   index 45b86be..8fe2b43 100644
   --- a/core/modules/views/src/Plugin/views/sort/SortPluginBase.php
   
   --- a/core/modules/views/src/Plugin/views/sort/SortPluginBase.php
   +++ b/core/modules/views/src/Plugin/views/sort/SortPluginBase.php
   
   +++ b/core/modules/views/src/Plugin/views/sort/SortPluginBase.php
   +++ b/core/modules/views/src/Plugin/views/sort/SortPluginBase.php
   @@ -241,7 +241,7 @@ public function getCacheContexts() {
   
   @@ -241,7 +241,7 @@ public function getCacheContexts() {
        $cache_contexts = [];
        // Exposed sorts use GET parameters, so it depends on the current URL.
        if ($this->isExposed()) {
   -      $cache_contexts[] = 'cache.context.url';
   +      $cache_contexts[] = 'url';
        }
        return $cache_contexts;
      }
   diff --git a/core/modules/views/src/Tests/GlossaryTest.php b/core/modules/views/src/Tests/GlossaryTest.php
   
   diff --git a/core/modules/views/src/Tests/GlossaryTest.php b/core/modules/views/src/Tests/GlossaryTest.php
   index fd97978..1df4701 100644
   
   index fd97978..1df4701 100644
   --- a/core/modules/views/src/Tests/GlossaryTest.php
   
   --- a/core/modules/views/src/Tests/GlossaryTest.php
   +++ b/core/modules/views/src/Tests/GlossaryTest.php
   
   +++ b/core/modules/views/src/Tests/GlossaryTest.php
   +++ b/core/modules/views/src/Tests/GlossaryTest.php
   @@ -87,7 +87,7 @@ public function testGlossaryView() {
   
   @@ -87,7 +87,7 @@ public function testGlossaryView() {
    
        // Verify cache tags.
        $this->enablePageCaching();
   -    $this->assertPageCacheContextsAndTags(Url::fromRoute('view.glossary.page_1'), ['cache.context.url', 'node_view_grants', 'language', 'user'], [
   +    $this->assertPageCacheContextsAndTags(Url::fromRoute('view.glossary.page_1'), ['languages', 'url', 'user'], [
          'config:views.view.glossary',
          'node:' . $nodes_by_char['a'][0]->id(), 'node:' . $nodes_by_char['a'][1]->id(), 'node:' . $nodes_by_char['a'][2]->id(),
          'node_list',
   diff --git a/core/modules/views/src/Tests/RenderCacheIntegrationTest.php b/core/modules/views/src/Tests/RenderCacheIntegrationTest.php
   
   diff --git a/core/modules/views/src/Tests/RenderCacheIntegrationTest.php b/core/modules/views/src/Tests/RenderCacheIntegrationTest.php
   index da94ab4..4a9276f 100644
   
   index da94ab4..4a9276f 100644
   --- a/core/modules/views/src/Tests/RenderCacheIntegrationTest.php
   
   --- a/core/modules/views/src/Tests/RenderCacheIntegrationTest.php
   +++ b/core/modules/views/src/Tests/RenderCacheIntegrationTest.php
   
   +++ b/core/modules/views/src/Tests/RenderCacheIntegrationTest.php
   +++ b/core/modules/views/src/Tests/RenderCacheIntegrationTest.php
   @@ -221,7 +221,7 @@ public function testViewAddCacheMetadata() {
   
   @@ -221,7 +221,7 @@ public function testViewAddCacheMetadata() {
        $view = View::load('test_display');
        $view->save();
    
   -    $this->assertEqual(['node_view_grants', 'user', 'language'], $view->getDisplay('default')['cache_metadata']['contexts']);
   +    $this->assertEqual(['languages', 'user', 'user.node_grants:view'], $view->getDisplay('default')['cache_metadata']['contexts']);
      }
   

   Isn't that part of the other patch?

No! That's exactly not what's intended. The pager uses the page query arg. It encodes the data for pagers 0, 1, 2, 3 … N into that query arg's value, separated by commas. Therefore, "all possible pager IDs" are contained by that query arg, therefore this is correct.

You probably did not understood my point at all. I doubt people keep in their head that there are more than one pager on a page (tell me about it, I've been lurking in the views support queue for years). Based upon that, people will always just use 'pager', even they technically actually should have used pager:0. Based upon that,
I'd argued before, that 0 should be the default, and you explicitly opt IN to show all.

Only developers will ever deal with the pager cache context.

Shock, so users will have to deal with cache contexts ? ;)
Right, if the render array already propagates that, there are just a few people which have to set it.

The point of the hierarchy is that we have the ability to "fold", "collapse", "simplify", "optimize" a set of cache contexts, by omitting more granular cache contexts. That's why we cannot assign a special behavior to pagers: making pagers === pagers:0 would break the hierarchy.

Alright, well, I just tried to think about it, and how to improve stuff.

So yeah, there is a reason I did not touched the RTBC state of this issue.

Few questions, not changing status yet.

1. +++ b/core/core.services.yml
   @@ -6,26 +6,86 @@ parameters:
   +  cache_context.ip:
   

   In terms of figuring out the list/hierarchy in the issue IP makes complete sense, but I'm not sure about actually providing it. Also why not a child of headers?

2. +++ b/core/core.services.yml
   @@ -6,26 +6,86 @@ parameters:
   +  cache_context.cookies:
   

   Why isn't cookies a child of headers?

3. +++ b/core/core.services.yml
   @@ -6,26 +6,86 @@ parameters:
   +  cache_context.user.is_super_user:
   

   So conceptually this is correct. But I wonder whether the logic should be contained in the roles cache context. Wherever you add per-role, you should also be adding this, and not adding it could lead to information disclosure. Role cache context is in 99.9% of cases "includes a permission check".

4. +++ b/core/lib/Drupal/Core/Cache/CacheContexts.php
   @@ -118,6 +119,69 @@ public function convertTokensToKeys(array $context_tokens) {
   +   * given set of cache context tokens that is a more granular context of
   

   Do we want to say more granular, or 'property of'?

5. +++ b/core/lib/Drupal/Core/Cache/CacheContexts.php
   @@ -118,6 +119,69 @@ public function convertTokensToKeys(array $context_tokens) {
   +   *  another cache context token in the set, is removed.
   

   Nit, extra space.

6. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheContextsTest.php
   @@ -20,6 +20,63 @@
   +  public function providerTestOptimizeTokens() {
   

   Nice.

1. OK yes that's $_SERVER + headers so not a true child of header.

2. I could go either on this. I guess it's something we don't need to optimize so OK.

3. that feels more like user.role:super_user too for that usage as well. uid 1 special casing is a pain. At least needs a follow-up even if we don't finalize that here.

The new contexts are handy for reviewing the patch, but there's several in here (IP address, header) that I can't ever see people using. If we end up listing all available contexts somewhere, does the extra choice help people pick the right one(s) to use? Answer to that might be yes.

OK that's a good answer - having the roots of the hierarchy means contrib and custom code can fit into it better.

Happy with everything else here now, so committed/pushed to 8.0.x, thanks!