Quotes from #2429257: Bubble cache contexts

@Berdir #51

One problem with cache contexts in general I guess is knowing what kind of contexts exist and when you need to add them.

@yched #52

hook_info() in D7, Plugin definitions in D8, provide identifiable patterns for finding which "things of a given kind" (from field types to render/form elements) exist and are defined by who (I'm talking about "human" discoverability). Not sure who defines cache contexts and where developpers can look for them ?

@Wim Leers #53

Yes, we need to improve the DX around that [...] Right now:

  • Look at the cache settings for blocks, all available cache contexts are listed there. That gives you UI labels to grep the code for, to find the corresponding ID in code.
  • Grep YML files for cache_context.* to find them. Command to do that: grep -R --color --include=*.services.yml 'cache_context\..*:$' .. E.g.:
    $ grep -R --color --include=*.services.yml 'cache_context\..*:$' .
    ./core/core.services.yml:  cache_context.url:
    ./core/core.services.yml:  cache_context.pager:
    ./core/core.services.yml:  cache_context.language:
    ./core/core.services.yml:  cache_context.theme:
    ./core/core.services.yml:  cache_context.timezone:
    ./core/core.services.yml:  cache_context.menu.active_trail:
    ./core/modules/book/book.services.yml:  cache_context.book.navigation:
    ./core/modules/node/node.services.yml:  cache_context.node_view_grants:
    ./core/modules/user/user.services.yml:  cache_context.user:
    ./core/modules/user/user.services.yml:  cache_context.user.roles:
    

    I agree we should improve it. Perhaps a drush command to list all cache contexts would also be helpful?

So, is there a way we can formalize an API for "defined cache contexts", allowing easy human discovery ? Would Plugins be too heavy ?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Issue summary: View changes

I guess the user story is :
As a developper writing a piece of render array, I want to know which existing context I can use to accurately describe my content's variations.
- Which contexts exist ?
- For a given context, what does it actually vary by (content language, interface language, day of the month...) ?

yched’s picture

Also, wondering wheher this notion of "varies by", as formulated by the cache API through the concept of cache contexts, matches 1:1 to what the HTTP "Vary" header is about.

I.e, are we in fact looking for a formalization of the latter, and have the internal cache API just leverage the same concept ?

(sorry, not sure that even makes any sense, I'm trying to put a vague and possibly moot idea into words :-p)

yched’s picture

Issue summary: View changes
Fabianx’s picture

So Cache Contexts should not be plugins. They should be available with the Kernel un-booted in the ideal case and such have very low overhead.

I think the main part is that more things in Drupal should implement CacheableInterface, which in itself has getCacheTags(), getCacheContexts(), getCacheKeys() (?) unsure, methods.

It can in a way be compared to Vary headers in browsers, however the difference is the hierarchical part we have here.

The simplest is to think of them as the successor of DRUPAL_CACHE_PER_USER constants and the old granularity setting.

The difference is that you can now have flexible contexts that are registered with cache_context.* in the container.

And yes, for drupal.org itself I missed the timezone cache_context, so it can indeed be easy to miss.

I think saying all that, our best bet is to do:

- Introduce Cache::getCacheContexts() method (or such)
- Add a getDescription() method to the CacheContextInterface so that we can describe in words what cache contexts are and when they should be used.

In the ideal case you however only have to worry about them when writing a field formatter or something like that.

For the 99% case rendering a date should itself give back the cache_context within a render array.

And for things where we still return just strings there are two ways:

a) return a render array instead (API change)
b) add another function that returns a render array with the right cache context.

The same dilemma is there for example for links.

The link generator in the ideal case should return a render array, so the #post_render_cache can be used, but then we have things that need a string.

For such we can theoretically use early rendering, which then pushes the #post_render_cache to the stack, which is another possibility for ensuring e.g. a rendered date adds its cache_context to the stack.

So long story short:

- In the ideal case our APIs are so great that you don't need to worry about cache contexts.
- But in practice just doing:

if ($this->isFrontPage()) {
}

needs either the cache_context.page OR a special cache context varying only for the front page.

So from that perspective we should _ban_ all ifs ;) ...

On the other hand extending that could indeed formalize that:

$cctx = \Drupal::service('cache_context.is_frontpage');
$cctx->applyTo($build);
if ($cctx->get($optional_param) == TRUE) {
  // Version A
}
else {
  // Version B
}

Then the cache context can not only be used for varying, but also for checking, which would be kinda cool :).

yched’s picture

Oh - I actually didn't realize that there is a CacheContextInterface :-) So, well, at least developper discovery is not too bad - just check which classes exist that implement CacheContextInterface, any IDE can do that.

The thing that looks a bit off is that the actual string to put in your #cache array for the context is not held in the CacheContext class itself, but is the name of the service that exposes the class.

So, DX wise, once you have found the CacheContext class that fits your needs, you still have to look up which service.yml refers to it and under which service name it is exposed.

Couldn't we keep the context 'machine names' in CacheContext classes themselves (like, in a static method, if we cannot leverage plugins/annotations ?), and use a compiler pass to collect them ?

larowlan’s picture

We had a similar issue with events where we ended up with an @Event tag in their doc block, maybe we could do the same here so the API site could document/discover them?

Wim Leers’s picture

Added this issue to the parent meta.

[…] matches 1:1 to what the HTTP "Vary" header is about

Conceptually, yes, this is exactly the way you must think about this.

(Fabianx indicates there's a certain hierarchy to it, which is true, but that doesn't matter. You still need to list all the aspects (cache contexts) you vary by. The hierarchy metadata can then be taken into account by Drupal to be clever about omitting some cache contexts. But this isn't implemented yet. It's an optional optimization. See #2432837: Make cache contexts hierarchical (e.g. 'user' is more specific than 'user.roles').)

Couldn't we keep the context 'machine names' in CacheContext classes themselves (like, in a static method, if we cannot leverage plugins/annotations ?), and use a compiler pass to collect them ?

Because then we additional logic to ensure uniqueness. Right now, we just use the service container's requirement of having unique service names to ensure unique service names. Every cache context service must follow a strict naming scheme. This aids discoverability. And it automatically gives us the uniqueness guarantees we need. See #2329101-9: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations. We could change this, if we consider this to be a true problem, or too different from the rest of Drupal.


#6: that sounds like a great idea :) — @CacheContext sounds sensible.

Fabianx’s picture

Agree, @CacheContext would be nice to have :). - edit especially because we have 2 Interfaces for normal and parametrized Cache Contexts.

yched’s picture

A @CacheContext annotation sounds great IMO. It wouldn't keep the "uniqueness guarantee" that @Wim mentions in #7 though ?

(not that I personally find it a big deal, we have that potential non-uniqueness issue wherever we use annotations, nothing prevent two classes from claiming they implement the same plugin id, this hasn't stopped us from using annotations - or hook_info() in D7 - so far ?)

Wim Leers’s picture

I think #2432837: Make cache contexts hierarchical (e.g. 'user' is more specific than 'user.roles') already goes a long way to improving the DX. See the associated change record. Please share your thoughts!

Fabianx’s picture

Wim Leers’s picture

I think #2432837: Make cache contexts hierarchical (e.g. 'user' is more specific than 'user.roles') already is a small step forward to make the DX better, we still need to work on formalization though.

Wim Leers’s picture

Wim Leers’s picture

I'd love to have @CacheContext annotations, but… annotations are for plugins, not for services.

Now, especially with this addition to the handbook page linked in #13 — https://www.drupal.org/node/2459039/revisions/view/8571735/8571749 — I feel like it already is sufficiently formalized: the service tag is how you find it, and alternatively, the implementations of the interface. Depending on your preferred tools, that's two ways of discovering cache contexts.

This is also not a problem unique to cache contexts — many services (and service interfaces) will have such an experience.

Thoughts?

effulgentsia’s picture

From #14's update to the handbook page:

Tip: You can get an up-to-date, complete listing of all cache contexts in Drupal by looking at the services tagged with cache_context!

That's a nice tip, but if I for example use PHPStorm to search for name: cache_context, it just shows me a bunch of identical lines containing only that string, so I then have to click each one to be taken to the corresponding line in the source file, and then look a few lines up to see the context name. However, if I use PHPStorm to search for cache_context. within *.services.yml files, then I get a nice listing containing all the names right there. In other words, taking advantage of our naming convention.

Not sure how to best incorporate that into the handbook page though. I don't think it should mention PHPStorm, so maybe it should be converted into a command-line grep command?

This is also not a problem unique to cache contexts — many services (and service interfaces) will have such an experience.

Yes, but most developers won't need to know the IDs of most tagged services, such as authentication providers or path processors, whereas the IDs of cache contexts are more common to need to set on a render array. That said though, I think the above is just as discoverable as searching for annotations, so other than maybe some more tweaks of the handbook page, I don't think there's anything else that needs to be done before closing this issue.

the service tag is how you find it, and alternatively, the implementations of the interface. Depending on your preferred tools, that's two ways of discovering cache contexts.

Finding all classes that implement an interface is a great technique, but then all you found is the class. It doesn't tell you what the ID is, and therefore, what string to add to your render array. I guess that's where an annotation would be nice, but personally I don't think the benefit of that is worth the downsides (coupling the context classes to annotation discovery) or the effort. But, maybe just a normal comment in the PHPDoc with examples of what to do to your render array to use that context? Especially for classes that provide several variants, like QueryArgsCacheContext.

Wim Leers’s picture

Title: Introduce a formal API for exposing and defining cache contexts ? » New convention: CacheContextInterface implementations should mention their ID in their class-level docblock
Assigned: Unassigned » Wim Leers
Issue tags: -API addition

[…] but if I for example use PHPStorm to search for name: cache_context […]

Again, this is not a problem unique to cache context services. We will need new/different/better tools in D8: it will be a quite common need to list all services that have a certain tag. Perhaps Symfony's existing CLI tools provide that?

[…] maybe it should be converted into a command-line grep command?

But this is always going to be a magical incantation that you have to copy/paste. That's bad.

But, maybe just a normal comment in the PHPDoc with examples of what to do to your render array to use that context? Especially for classes that provide several variants, like QueryArgsCacheContext.

Hrm, but then we're duplicating the service ID from the *.services.yml file in the class that implements the service. That's icky too. But surely helps the DX.

So then I really do think that the best way is to teach developers that all cache contexts implement the CacheContextInterface, and that they can discover all available cache contexts by using their IDE. They'll then have the string they actually need in the classes they've found by using their IDE.

This also then fully answers what @yched said in #5:

[…] just check which classes exist that implement CacheContextInterface, any IDE can do that.

The thing that looks a bit off is that the actual string to put in your #cache array for the context is not held in the CacheContext class itself, but is the name of the service that exposes the class.

So, DX wise, once you have found the CacheContext class that fits your needs, you still have to look up which service.yml refers to it and under which service name it is exposed

So, let's do it! :) Retitling, expect patch soon.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Active » Needs review
FileSize
13.32 KB

Et voila.

Already updated the handbook page linked in #13: https://www.drupal.org/node/2459039/revisions/view/8573267/8573371.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Looks great to me!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2443323-17.patch, failed testing.

Status: Needs work » Needs review

Fabianx queued 17: 2443323-17.patch for re-testing.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Assigned: Unassigned » jhodgdon
Status: Reviewed & tested by the community » Needs review

I'm wondering a bit why we can't use doxygen groups here: http://www.stack.nl/~dimitri/doxygen/manual/grouping.html

Wim Leers’s picture

Ohhhhhh! Yes! We can do that as well. But, we already have something like that: https://api.drupal.org/api/drupal/services/8?tag=cache.context. I guess the advantage of Doxygen groups would be that we could not use it for test-only cache contexts.

But, AFAICT, that is a purely additional kind of thing anyway. The problem is not the discovery of these services per se, but the discovery of the IDs associated with these services. And that's what these comments (this patch) add.

dawehner’s picture

I'm confused, we actively not adding service IDs to class documentation for services, not because you can, but because we want to be able to decouple the actual service IDs from the logic.

Can we ensure that this is pure documentation purposes? The general problem here is also not a special case if you ask me.

Wim Leers’s picture

Please see #5 and #16.

The implementation living at a certain service ID may still change. i.e. contrib could come and override the url cache context.

I personally also don't think this is necessary, but it's a fact that api.d.o does not show whether a certain class is actually a service. It only shows the other way around: which class a service uses.
The entire reason we're doing this, is because it's too difficult with the current tools for developers to either:

  1. list all cache contexts using the cache.context service tag
  2. given CacheContextInterfaceget all implementations, and list the associated service IDs

Number one is completely unsupported by IDEs. For number two, the "get all implementations" part does work, which gets you close, but the "list associated service IDs" part requires one to grep through YML files again. All this patch does, is remove that very last step.

jhodgdon’s picture

Ummm.... I am not sure what the question is here?

The API module supports @defgroup and @ingroup. It doesn't currently support any other form of grouping from Doxygen. We don't actually use a Doxygen parser, so if we want to add a new feature, someone (read: jhodgdon) needs to add it to our code.

So I'm not sure what the idea is in #22.

RE #15 - go to https://api.drupal.org/api/drupal/services/8
There you can look for services with a given tag. (type "cache.context" in the Tag Contains search box and click Apply).

So... What's the question you wanted me to answer?

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
effulgentsia’s picture

The general problem here is also not a special case if you ask me.

I think what makes cache contexts special is that for most service IDs, you shouldn't really have code that deals with them. Instead, the service IDs are ideally only there for setting up your dependencies in services.yml file. Of course, we also have static create() methods all over the place, but still, that's a factory method that serves a similar purpose as a services.yml file. Whereas for cache contexts: blocks, controllers, Views plugins, field formatters, and various other code is expected to use those magic string names, not as part of a factory, but as part of normal fulfillment of their interfaces. So I think there's value in making sure those are very easily discoverable, even by people not wanting or thinking to install Symfony command-line tools. https://api.drupal.org/api/drupal/services/8 is great, but not for people who are offline while writing their code, or for people also wanting to discover cache contexts provided by the contrib modules they're using, or for discovering which ones support parameters after a : and what those parameter values can be.

@jhodgdon: I don't know if catch had a more specific question for you, but I'm curious what your thoughts are in general for if and how to best document this. Look at the current documentation of CacheableDependencyInterface::getCacheContexts(). What's the best way for us to make it so that people implementing that interface know what strings to return? Do you think https://www.drupal.org/developing/api/8/cache/contexts and https://api.drupal.org/api/drupal/services/8 are sufficient, or do you think adding something to the codebase along the lines of #17 is a good idea?

catch’s picture

@jhodgdon yes no specific question I just thought your opinion would be useful.

Wim Leers’s picture

https://api.drupal.org/api/drupal/services/8 is great, but not for people who are offline while writing their code, or for people also wanting to discover cache contexts provided by the contrib modules they're using, or for discovering which ones support parameters after a : and what those parameter values can be.

Yep, and that's exactly what this patch addresses.

Look at the current documentation of CacheableDependencyInterface::getCacheContexts().

Great point! I think improving the docs for that significantly could be a huge help. Do others agree?

jhodgdon’s picture

OK... The problem space defined in the issue summary is, I think, "How should a developer discover what cache contexts are available, what each one of them is, and how to put them into a render array properly?"

Assuming that a developer has found either
https://www.drupal.org/developing/api/8/cache/contexts
or the new documentation about cache contexts from #2500443: Cache API topic says nothing about cache context, add something that should soon be on api.drupal.org, the information they would know is:
- If they are on the d.o page, they would see a list of the Core contexts (which is probably current now, but might not be forever).
- The d.o page mentions that to discover the available cache contexts, you would look for classes implementing \Drupal\Core\Cache\Context\CacheContextInterface (and it tells how to do this in an IDE).
- The api.d.o topic doesn't mention how to discover this list except to go to the d.o page.

Then the "put it into the render array" docs we have are:
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Render!theme.api....
or
https://www.drupal.org/developing/api/8/render/arrays/cacheability

So, here is a list of the missing pieces, I think:

a) If you follow the instructions to find CacheContextInterface implementations, the implementations don't tell you what the context IDs are. The patch on this issue addresses this. We should review it carefully and make sure all the classes make it clear what exactly you'd need to put in there and what it would indicate. I glanced at it and I think it's probably fine but haven't done a careful review yet... maybe others who know the classes/system better can review that?

b) The d.o page doesn't mention how to find these on api.drupal.org. I think it should say that you can go to the Services page and filter by tag cache.context. That should be easy to add.

c) Unfortunately the API module, on an Interface page, only shows classes that directly implement an interface, not including classes that extend those ones or classes that implement an interface that extends the one being shown. Maybe we should make that better? If so we should file a separate issue in the API module. Thoughts? Seems like it might be useful, and it is probably not difficult to do.

d) The api.d.o topic doesn't tell how to discover the cache contexts without going to a d.o page that may not be available. So probably we should add something to the patch on #2500443: Cache API topic says nothing about cache context, add something that says how to discover them. (implementing the interface or finding services tagged cache.context). Should we do that in this issue or set that other one back to needs work? Probably on the other issue? I'll mark it now.

e) Anything else?

Wim Leers’s picture

  • a) Indeed.
  • b) It actually does mention it: by linking to it, not by explaining it. (Look at the "Tip" blockquote.)
  • c) Indeed, that's very, very unfortunate :( But I'm sure that's very doable. +1000 on that, it is currently quite confusing.
  • d) Agreed (after initially disagreeing, you convinced me), you already did it in that issue, and I RTBC'd it.
  • e) Nope, you were very thorough :)

So, then all that is left to do is for a) to happen: that somebody else reviews it. But Fabianx, who knows the system very well too, already did so in #18. Which means — I think — there's nothing left to be done here? :)

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Nope, can get back to RTBC - unless anyone has more objections.

jhodgdon’s picture

For (c) filed #2509084: Show all classes that implement an interface on Interface page

I am in the middle of organizing the Drupal 8 User Manual project right now (!!!) so I will not get to it for a while, but at least there is an issue so when I have time to get back to the API module, it will be on the list.

Wim Leers’s picture

#34: thanks for c), following :)

I am in the middle of organizing the Drupal 8 User Manual project right now (!!!)

OMG!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll

Nitesh Sethia’s picture

Status: Needs work » Needs review
FileSize
11.9 KB

Rerolled the patch as per the latest release.

Nitesh Sethia’s picture

Issue tags: -Needs reroll

Removing the tag of Rerolling the patch.

Fabianx’s picture

Status: Needs review » Needs work
Issue tags: +Novice
  1. +++ b/core/lib/Drupal/Core/Cache/Context/QueryArgsCacheContext.php
    @@ -10,9 +10,9 @@
    - * A "host" is defined as the combination of URI scheme, domain name and port.
    - *
    - * @see Symfony\Component\HttpFoundation::getSchemeAndHttpHost()
    + * Cache context ID: 'url.query_args' (to vary by all query arguments).
    + * Calculated cache context ID: 'url.query_args:%key', e.g.'url.query_args:foo'
    + * (to vary by the 'foo' query argument).
    

    This needs manual fixing.

  2. +++ b/core/lib/Drupal/Core/Cache/Context/UserRolesCacheContext.php
    @@ -10,8 +10,9 @@
    - * Only use this cache context when checking explicitly for certain roles. Use
    - * user.permissions for anything that checks permissions.
    + * Cache context ID: 'user.roles' (to vary by all roles of the current user).
    + * Calculated cache context ID: 'user.roles:%role', e.g. 'user.roles:anonymous'
    + * (to vary by the presence/absence of a specific role).
    

    This needs manual fixing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
13.38 KB
1.47 KB

#37 was a bad reroll, I'm afraid. (FYI: rerolled by applying the patch in #17 using git apply -3, only one conflict, easily resolved.)

Double-checked that no new cache contexts have been added in the mean time.

Since this is a straight reroll, immediately back to RTBC.

Wim Leers’s picture

Issue tags: -Novice

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 40: 2443323-40.patch, failed testing.

Wim Leers’s picture

A docs-only patch is failing? I don't think so.

Segmentation fault
FATAL Drupal\migrate_drupal\Tests\d6\MigrateFieldFormatterSettingsTest: test runner returned a non-zero error code (139).

Testbot server instability?

Wim Leers queued 40: 2443323-40.patch for re-testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Documentation is not frozen in beta. Committed 233c9ea and pushed to 8.0.x. Thanks!

  • alexpott committed 233c9ea on 8.0.x
    Issue #2443323 by Wim Leers, Nitesh Sethia, yched, Fabianx, jhodgdon:...

Status: Fixed » Closed (fixed)

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