API page: https://api.drupal.org/api/drupal/core%21core.api.php/group/cache/8

This page says nothing about Cache Context, which is an important part of the cache system.

We should add a short section, and point readers to:
https://www.drupal.org/developing/api/8/cache/contexts
https://www.drupal.org/developing/api/8/render/arrays/cacheability
for more details.

This came up on #2429287-78: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance and the following few comments.

Beta phase: This is only API docs

CommentFileSizeAuthor
#15 interdiff.txt1.14 KBjhodgdon
#15 2500443-cache-context-docs-14.patch3.34 KBjhodgdon
#8 interdiff.txt2.26 KBjhodgdon
#8 2500443-cache-context-docs-8.patch3.07 KBjhodgdon
#4 2500443-cache-context-docs-4.patch3.08 KBjhodgdon

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Assigned: Unassigned » jhodgdon

I plan to do this in a week or so, but if someone else has time to get to it first, please assign it to yourself and I'll review. Either way, let's just get it done. ;)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I'll be happy to take this on when I'm back from vacation, next Friday, unless you beat me to it.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

OK I'm writing this today...

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 2500443-cache-context-docs-4.patch3.08 KB

How's this? Hopefully not too out of scope, I fixed 1 wrapping issue and 2 broken method reference issues (methods moved from one interface to another and the docs were not updated) in the same topic.

wim leers’s picture

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

Needs reroll after #2401843: Move all *.api.php files except system.api.php out of system module directory. Updated the IS to have a working link too.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Looks great!

All remarks beside the first two are about the same thing: in the current writing, it seems that the contextual data is only relevant when caching, but that's not true; it's precisely because of the computed data depending on the context, that we must carefully take the used contextual data into account when caching. Otherwise, you end up with cache poisoning!
That's the part I believe needs to be addressed, after that, this is good to go :)

  1. +++ b/core/modules/system/core.api.php
@@ -374,9 +374,9 @@
+ * Caching can be permanent, temporary, or valid for a certain timespan, and

    What's the difference between "temporary" and "for a certain timespan"? AFAICT they're the same. 

  2. +++ b/core/modules/system/core.api.php
@@ -478,7 +478,7 @@
- * \Drupal\Core\Entity\EntityInterface::getCacheTags() and
+ * \Drupal\Core\Cache\CacheableDependencyInterface::getCacheTags() and

@@ -519,11 +519,28 @@
- * See \Drupal\Core\Entity\EntityInterface::getCacheTags(),
+ * See \Drupal\Core\Cache\CacheableDepenencyInterface::getCacheTags(),

    Good call :) 

  3. +++ b/core/modules/system/core.api.php
@@ -519,11 +519,28 @@
+ * Some data that needs to be cached depends on contextual data, such as the

    s/cached/computed/ 

  4. +++ b/core/modules/system/core.api.php
@@ -519,11 +519,28 @@
+ * is being rendered in, the theme being used, etc. The basic idea is that you

    s/The basic idea is that you/When caching such data, you/ 

  5. +++ b/core/modules/system/core.api.php
@@ -519,11 +519,28 @@
+ * can cache several variations of the data, along with information about the

    s/can cache several variations/must cache every variations separately/

    s/data/computed data/

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Parent issue: » #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance
Related issues: -#2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new 2500443-cache-context-docs-8.patch3.07 KB
new interdiff.txt2.26 KB

Thanks for the review -- good points! How's this?

[The core.api.php file moved to core/core.api.php so had to reroll for that. Interdiff is from that reroll. Though the interdiff is nearly as long as the patch. :) ]

wim leers’s picture

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

(Yeah, interdiff is quite useless here :D)

Looks excellent!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2500443-cache-context-docs-8.patch, failed testing.

Wim Leers queued 8: 2500443-cache-context-docs-8.patch for re-testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

It looks like we should probably add a line or two about how to discover the available cache contexts without going to a drupal.org page that might not be available (on a plane etc.). See #2443323-31: New convention: CacheContextInterface implementations should mention their ID in their class-level docblock

Needs to say that the classes implement CacheContextInterface (with the namespace) and the services are tagged cache.context, and the ID that you need is the service ID minus the prefix.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Isn't that out of scope for those docs? It links to a page that explains it.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 2500443-cache-context-docs-14.patch3.34 KB
new interdiff.txt1.14 KB

Well, on the other issue I think we concluded (or at least I did ;) ) that it is essential information for developers to have, so I think it merits a sentence at least. Probably a sentence at most also. ;)

Here's a patch/interdiff, to illustrate just how brief I think it can be.

wim leers’s picture

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

Oh, that is wonderfully concise :)

alexpott’s picture

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

Documentation improvements are permitted in beta. Committed 9812bb2 and pushed to 8.0.x. Thanks!

  • alexpott committed 9812bb2 on 8.0.x 
    Issue #2500443 by jhodgdon, Wim Leers: Cache API topic says nothing...

Status: Fixed » Closed (fixed)

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