Fix CacheableInterface so it makes sense (was: "Make Config objects & Entities implement CacheableInterface + add BubbleableMetadata::createFromCacheableObject()")

I don't think there's already an issue, I can only find #2351847-14: Rename getCacheTag() to getCacheTags() and comments 16, 27 and 28 there. It was discussed there, but no follow-up was actually filed.

#2429257: Bubble cache contexts landed, rough initial patch.

Because I thought a ViewUI object is intended to never ever be cached, but only be a runtime representation? If that's wrong, happy to change :)

Oh, ok :)

This should've been part of #13, but I forgot.

#2407765: Wherever simple config is used to render output to end user, associate their cache tags is in, now we should include the fix for #2407765-46: Wherever simple config is used to render output to end user, associate their cache tags.

In the original proposal, I presented

BubbleableMetadata::createFromCacheableObject($config)->applyTo($build);

as sufficient. But I was mistaken. What is necessary is this:

    BubbleableMetadata::createFromCacheableObject($config)
      ->merge(BubbleableMetadata::createFromRenderArray($form))
      ->applyTo($form);

Because applyTo() doesn't automatically merge what's already in the render array. Know that this was originally introduced only to be used inside Renderer itself, where the merging already happened in an earlier step.

I think there's something to be said to make applyTo() perform merging with the render array's bubbleable metadata automatically. If others agree, I'll make applyTo() do $merged = $this->merge(static::createFromRenderArray($build)); internally.

Here's what the code looks like without doing that.

#15:

1. Right, ConfigBase is unaware of the collection it's in. Because only StorableConfigBase (a subclass) does know that, and the other subclass (ThemeSettings) cannot be stored (it's a runtime-only object).
2. So this is what that currently looks like:
   

     protected function buildCacheId($id) {
       return "values:{$this->entityTypeId}:$id";
     }
   

   but more importantly, these are the cache tags used for caching entities:

       $cache_tags = array(
         $this->entityTypeId . '_values',
         'entity_field_info',
       );
   

   Ideally, we'd return those as the cache tags for the entity object also. And I see no reason not to. The "_values"-specific cache tag can go away, since it serves no more purpose. This will cause many test failures though. Plus, in theory, we should no longer need the bit of code in SqlContentEntityStorage::resetCache() that invalidates an entity type's entire entity cache. That should only be necessary when field info is modified, and by associating that cache tag with all entities, that will simply always be true.

But this:

Also, we can't actually access this method on cache get, so no, we can't... thinking about it, this the usefulness of this method is a bit questionable.. :)

… I have no words for this, I can't believe nobody even thought of that since we introduced this almost a year ago in #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags.

You're right. Which I guess means CacheableInterface actually is specifically designed for render cacheable objects.

This again makes the case for the CacheabilityAffectorInterface I wanted to add in #2287071: Add cacheability metadata to access checks (see the first patch there, and the discussion following it): an interface that only allows you to define affecting cacheability metadata (contexts + tags + max-age), i.e. the metadata for an object (A) that affects the cacheability of another object that contains it (P contains A).

Further indication that getCacheKeys() is useless: there's only one implementation of it in core that doesn't return the empty array: ForumBlockBase. And that one should probably be converted to use a cache context instead.

So I guess I'm saying: we should ditch getCacheKeys() because as Berdir so simply pointed out: it's impossible to use!

There goes the dream of having a single interface used to indicate the cacheability of anything and everything.

#18 should fail majestically and be slower than most test runs, because I'm stupid:

berdir:: WimLeers: also, your Entity::getCacheTags() change is also wrong. that means that we invalidate the entire entity cache and field definitions when an entity is saved :)
WimLeers: berdir: HAHA!
WimLeers: berdir: indeed
WimLeers: berdir: Wim--
WimLeers: Seems this is WimFailDay

… so just ignore that patch.

The summary from the chat discussion that Fabianx mentioned, we had this chat last Friday (March 6, 2015).

(Sorry for the delay in posting it — working on too many issues at once at the moment.)

For the sake of simplicity and transparency, I'm just posting the entire chat log, I only omitted messages that were unrelated. I did split it up according to trains of thought, so that it's easier to follow.

1: ::getCacheKeys() doesn't make sense, berdir++

10:50:34 WimLeers: Fabianx-screen: morning
10:50:50 WimLeers: berdir: you were right.
10:51:01 WimLeers: catch: Fabianx-screen: berdir: thoughts most welcome on https://www.drupal.org/node/2444231#comment-9687973
10:51:03 Druplicon: https://www.drupal.org/node/2444231 => Make Config objects & Entities implement CacheableInterface + add BubbleableMetadata::createFromCacheableObject() #2444231: Fix CacheableInterface so it makes sense (was: "Make Config objects & Entities implement CacheableInterface + add BubbleableMetadata::createFromCacheableObject()") => 18 comments, 1 IRC mention
10:51:18 WimLeers: Basically: CacheableInterface::getCacheKeys() is nonsensical (as pointed out by berdir).
10:52:09 WimLeers: I guess we're only discovering that now because we've only implemented it for 2 classes in all this time
10:52:25 WimLeers: We're trying to add 2 more (Entity & Config), and the flaw is immediately obvious :P
10:52:34 berdir: WimLeers: "Plus, in theory, we should no longer need the bit of code in SqlContentEntityStorage::resetCache() that invalidates an entity type's entire entity cache". That's the theory, and then there are the 1 million calls to resetCache() in tests :)
10:52:46 WimLeers: berdir: yes, let's see how hard it fails :D
10:53:33 catch: WimLeers: I remember talking about this before.
10:53:51 Fabianx-screen: WimLeers: looking
10:54:02 WimLeers: berdir++
10:54:03 WimLeers: berdir++
10:54:11 catch: So we'd have tags and contexts and no keys.
10:54:19 WimLeers: yes
10:54:21 WimLeers: with one caveat
10:54:21 catch: Or does keys move to a different interface?
10:54:28 WimLeers: Keys never make sense
10:54:34 WimLeers: If you want to get a cached object
10:54:45 WimLeers: … then how can you call getCacheKeys(), if it's on the object you're trying to retrieve from cache?
10:54:52 berdir: WimLeers: sorry for crushing your dream ;)
10:55:07 WimLeers: the CID of a cached object by definition must be generated outside of the object
10:55:10 WimLeers: berdir: np :)

2. Thinking about CacheableInterface

10:55:40 WimLeers: catch: I almost think we want to rename this to RenderCacheableInterface
10:55:55 WimLeers: to indicate the narrower scope
10:56:05 Fabianx-screen: WimLeers: oh, I wanted to know what the difference between isCacheable == FALSE and max-age=0 is and how that would convert to render arrays for quite some time now :).
10:56:14 WimLeers: Fabianx-screen: yes, I wondered that too now
10:56:31 berdir: WimLeers: also, your Entity::getCacheTags() change is also wrong. that means that we invalidate the entire entity cache and field definitions when an entity is saved :)
10:56:36 WimLeers: Fabianx-screen: we only have isCacheable() because of blocks; some blocks want to have custom logic to determine when they are cacheable
10:56:50 WimLeers: Fabianx-screen: … that was the reasoning msonnabaum, moshe and probably also I applied a year ago 
10:57:01 WimLeers: Fabianx-screen: but I also think ::isCacheable() is pointless now
10:57:18 WimLeers: berdir: HAHA!
10:57:23 WimLeers: berdir: indeed
10:57:26 WimLeers: berdir: Wim--
10:57:38 WimLeers: Seems this is WimFailDay
10:58:35 Fabianx-screen: WimLeers: I think isCacheable might be useful in combination with a potential #pre_render_cache though as an indication for post rendering this.
10:58:51 Fabianx-screen: But before that we should ensure CacheableInterface matches #cache.
10:59:29 WimLeers: Fabianx-screen: if we do that matching, then that's another reason for calling it RenderCacheableInterface
11:00:12 Fabianx-screen: Works for me, all advanced caching we do is tied to therender chain right now
11:00:15 Fabianx-screen: the render
11:00:24 WimLeers: So from:

  BubbleableMetadata::createFromCacheableObject(CacheableInterface $object);

to:

  BubbleableMetadata::createFromCacheableObject(RenderCacheableInterface $object);

That also makes more sense.
11:00:24 Fabianx-screen: but thats what empowers us to do certain things, so this is fine.
11:00:47 Fabianx-screen: Maybe only createFrom() ?
11:00:50 WimLeers: I need to go AFK for a bit, but yeah, would be great if you guys could post your thoughts on this in the issue.
11:00:54 Fabianx-screen: The Interface is in hte parameter.
11:01:04 WimLeers: Fabianx-screen: would love that, but… PHP doesn't have polymorphism
11:01:11 Fabianx-screen: WimLeers: Why does keys never make sense?
11:01:12 WimLeers: Fabianx-screen: we already have ::createFromRenderArray()
11:01:28 Fabianx-screen: WimLeers: okay, in that context, the distinctions makes sense.
11:01:34 Fabianx-screen: And 'mixed' is evil ...
11:01:39 WimLeers: y
11:01:56 WimLeers: Fabianx-screen: so yeah, IF we rename it to RenderCacheableInterface, then we don't need to remove getCacheKeys()
11:02:08 Fabianx-screen: WimLeers: So when you come back you'll need to again explain me why getCacheTags won't work ...
11:02:09 berdir: WimLeers: I like majestical fail almost as much as "very non-ideal" :p
11:02:13 Fabianx-screen: ahhh
11:02:14 WimLeers: Fabianx-screen: but that will just avoid the tendency to have all objects implement CacheableInterface
11:02:14 Fabianx-screen: okay
11:02:28 WimLeers: Because an entity is not renderable
11:02:34 WimLeers: not directly at least
11:02:43 WimLeers: the entity object doesn't have a ->render() method or something like it
11:02:56 WimLeers: so then it also shouldn't implement (Render)CacheableInterface
11:02:58 WimLeers: berdir: :P
11:03:22 WimLeers: So it sounds like that's what we want
11:04:14 WimLeers: RenderCacheableInterface === the exact current CacheableInterface

plus *maybe* (but probably not?):

CacheabilityAffectorInterface === only the bubbleable cacheability metadata
11:04:50 WimLeers: no that still feels wrong
11:04:54 WimLeers: e.g. it never makes sense to render cache an AccessResult
11:05:04 WimLeers: but you DO want to use it to modify the cacheability of a render array
11:05:22 WimLeers: Same goes for Config objects
11:05:58 WimLeers: so we have objects that 1) cause the need for more cache contexts (AccessResult) or 2) cause the need for more cache tags, i.e. dependencies (Config)
11:06:48 WimLeers: and AccessResult falls in both categories
11:07:06 WimLeers: So we really do want something like CacheabilityAffector AFAICT
11:08:11 WimLeers: Note that solving this aspect is crucial: if it's too difficult to do these things, people won't do them.
11:08:32 Fabianx-screen: agree
11:09:30 WimLeers: oh and I guess that if you use language-overridden config, that it also needs a cache context (vary per language)
11:10:09 WimLeers: AFK now.
11:15:42 Fabianx-screen: WimLeers: Lets talk it through with examples and use-cases when you are back :)
…
12:32:55 WimLeers: Fabianx-screen: b
12:41:56 Fabianx-screen: WimLeers: So I am a strong believer in use cases when it comes to API design.
12:42:07 Fabianx-screen: And at the moment I am very confused.
12:42:27 Fabianx-screen: So it would help if we started at the beginning of what we are trying to achieve.
12:42:41 Fabianx-screen: Also I know not much about the new entities, etc.
12:42:52 Fabianx-screen: So pardon me if my knowledge there is lacking about the internals.
12:43:12 WimLeers: np
12:44:12 WimLeers: The idea behind CacheableInterface — IIRC — is to formalize in an interface what cacheability metadata exists for an object when it is cached. And it was designed specifically with blocks in mind, so, really, for an object that is *renderable* and hence render cacheable.
12:45:15 WimLeers: But we have various objects that *affect* rendering, but aren't *renderable* themselves.
12:45:32 WimLeers: So we want an easy way to get the cacheability metadata from those objects and apply them to a render array.
12:45:41 WimLeers: *That* is the current task, and which is why we're talking about all this.
12:45:52 Fabianx-screen: because e.g. entites have a buildContent method but can apply things to #cache directly?
12:46:00 WimLeers: So: checking access → should be easy to associate the necessary cacheability metadata.
12:46:09 WimLeers: Using config objects → same.
12:46:19 WimLeers: So that's the general story.
12:46:33 WimLeers: But if we look at objects which can be rendered — entities, blocks — then we see something interesting.
12:46:34 Fabianx-screen: but those don't deal directly with render arrays.
12:46:40 Fabianx-screen: So can't add to #cache
12:46:47 WimLeers: The responsibility does NOT lie with those objects themselves to render themselves.
12:46:51 WimLeers: They both have "view builders".
12:46:59 WimLeers: So the objects are really just "data".
12:47:05 Fabianx-screen: okay
12:47:10 WimLeers: And that data can be passed on to a "view builder" that transforms them into render arrays.
12:47:45 WimLeers: This is why EntityInterface and ConfigBase have ::getCacheTags() methods: to identify the object that the built render array depends upon.
12:47:58 WimLeers: … or anywhere else where we cache data that uses something from an entity or from config
12:48:22 Fabianx-screen: okay
12:50:09 WimLeers: So we have data objects, and data objects' data may change. So they have a ::getCacheTags() method, so that when the data object changes, caches can be invalidated if they use data from those data objects.
12:50:19 Fabianx-screen: sure
12:50:30 WimLeers: So that is the use case we need an optimized interface for
12:50:36 WimLeers: So what needs to be in it?
12:50:39 WimLeers: ::getCacheTags() for sure
12:50:55 WimLeers: But those data objects may e.g. also be only valid only for a short time. So ::getMaxAge() makes sense too.
12:51:17 Fabianx-screen: WimLeers: So an entity is not responsible for clearing caches itself / invalidating tags, but that controller?
12:51:44 Fabianx-screen: getMaxAge() makes only sense if we have -1 for infinite caching or such.
12:51:50 WimLeers: I don't think we want ::getCacheContexts(), because that is about different representations, and by definition it is so that a data object is just *one* representation. It's only when you combine it with other things, that there can be multiple representations
12:51:56 WimLeers: Fabianx-screen: we have that already: Cache::PERMANENT
12:52:10 Fabianx-screen: Yes, but that is == 0 in D8 IIRC
12:52:13 WimLeers: nope, -1, I fixed that :) Precisely for that reason.
12:52:40 Fabianx-screen: okay. It was 0 in D7 ... Got it!
12:54:25 Fabianx-screen: okay, so getMaxAge() return Cache::PERMANENT; works for me.
12:57:52 Fabianx-screen: WimLeers: okay, so: getCacheTags() for sure, getMaxAge() yes, getCacheContexts() yes too.
12:58:09 Fabianx-screen: What about isCacheable()?

3. How to evolve CacheableInterface: what makes sense and what doesn't?

12:59:01 WimLeers: https://pad.lullabot.com/p/cacheableinterface
12:59:19 WimLeers: (to document and visualize)
12:59:22 WimLeers: let's continue talking here
12:59:39 WimLeers: getCacheContexts() shouldn't be there, see above.
13:00:48 WimLeers: isCacheable() makes no sense if this interface is NOT about whether the object itself is cacheable, but about how it affects the cacheability of things using it
13:01:29 WimLeers:
CacheabilityAffectorInterface
CacheAffectorInterface
CachedDependencyInterface
13:01:38 WimLeers: I don't really like any of them
13:02:08 WimLeers: CacheableDependencyInterface
13:02:13 WimLeers: I kinda like that
13:02:13 Fabianx-screen: WimLeers: So why cache tags, what would be examples of those that cannot be set by the view builder?
13:02:35 WimLeers: because each data object *does* have a cache tag associated with it
13:02:53 WimLeers: it uniquely identifies the inherent thing, which is a blob of data
13:02:55 Fabianx-screen: With the argument for the cache contexts, you can say the same for max-age age and tags ...
13:03:02 WimLeers: Can you?
13:03:04 WimLeers: I don't see how.
13:03:05 Fabianx-screen: WimLeers: But the view builder puts it on the page.
13:03:12 WimLeers: ok
13:03:17 WimLeers: so let's reword it in a story
13:03:19 Fabianx-screen: So its only relevant when its rendered.
13:03:25 WimLeers: I have the EntityViewBuilder, to render entities.
13:03:44 WimLeers: To do its work, I need to give it two things: 1) an Entity, 2) a view mode
13:04:21 WimLeers: so the EVB (Entity View Builder) ***depends*** on multiple bits of data, and then constructs something (a render array) from that
13:04:31 WimLeers: therefore the render array depends on the data it is given
13:04:34 WimLeers: which includes the entity
13:04:39 WimLeers: as in, the entity data blob
13:05:20 WimLeers: </story>
13:06:00 Fabianx-screen: okay
13:06:29 Fabianx-screen: So when I view an entity (and I have a RenderController in D7 that is similar to an entity_view_builder)
13:06:33 Fabianx-screen: Then I put the cache tags:
13:06:38 Fabianx-screen: entity_view:<id>
13:06:44 Fabianx-screen: and entity:<id>
13:06:50 Fabianx-screen: cause its now included in the render array.
13:07:48 WimLeers: basically, yes
13:10:03 Fabianx-screen: So I think EntityViewBuilders, BlockViewBuilders, etc. should implement the CacheableInterface
13:10:14 Fabianx-screen: not the data objects itself.
13:10:29 Fabianx-screen: Cause this is Model View Controller right?
13:11:33 Fabianx-screen: So but getCacheKeys() might or might not be helpful.
13:11:55 Fabianx-screen: e.g. I might want to inherit the cache keys my parent EntityViewBuilder sets.
13:12:21 Fabianx-screen: I think the original interface makes sense, its just on the wrong objects ;).
13:13:17 WimLeers: Having the view builders implement the interface is something we already contemplated.
13:13:20 WimLeers: It's impossible.
13:13:28 WimLeers: For a very simple reason.
13:13:56 WimLeers: the view builders are *services* that process objects; they *themselves* aren't cacheable.
13:14:30 WimLeers: If a view builder would *generate* an object that represents the renderable, e.g. RenderableEntity, *that* could work.
13:14:45 WimLeers: This is why it's fine for blocks
13:14:54 WimLeers: Blocks directly represent renderables
13:15:07 WimLeers: A block config entity merely stores settings for the BlockRenderable, if you will
13:15:16 WimLeers: A block itself doesn't contain *the* data to be rendered.
13:15:20 WimLeers: Only "render instructions"
13:16:04 WimLeers: I personally think a huge part of our objects are confusing because we don't express the dependencies well. In fact, Drupal 8 is the first version where we're getting serious about expressing dependencies.
13:16:20 WimLeers: an entity is a data object
13:16:31 WimLeers: but then we have entity storage, which has its own cache & own cache tags
13:16:40 WimLeers: we have the entity view builder, which again has its own cache & own cache tags
13:16:40 WimLeers: etc
13:16:56 WimLeers: each of those "handlers" actually depend on a specific entity being given to them
13:17:13 WimLeers: they're meaningless on their own
13:18:53 Fabianx-screen: okay
13:19:02 WimLeers: Fabianx-screen: did that make sense?
13:20:45 Fabianx-screen: yes
13:21:23 Fabianx-screen: So entities are probably a bad example, but access is a good one.
13:29:29 WimLeers: Fabianx-screen: https://pad.lullabot.com/p/cacheableinterface
13:30:31 WimLeers: option a: rename CacheableInterface to be less confusing and add a new CacheableDependencyInterface
13:30:45 WimLeers: option b: all of the above, but also remove getCacheKeys from CacheableInterface
13:30:51 WimLeers: With:
13:30:52 WimLeers: interface CacheableDependencyInterface {

		public function getCacheContexts();

		public function getCacheTags();

		public function getCacheMaxAge();

} 

13:30:59 WimLeers: I argued against contexts
13:31:03 WimLeers: but it's necessary for AccessResult
13:31:10 WimLeers: Which made me realize that it's fine
13:31:19 WimLeers: It's useless for Entity
13:31:56 WimLeers: But if the data is dynamically calculated (AccessResult) instead of merely retrieved from storage (Entity), then it actually makes sense
13:32:06 WimLeers: That's wholly consistent imo
13:32:09 WimLeers: And the best part
13:32:19 WimLeers: That's *exactly* the bubbleable cacheability metadata
13:32:27 WimLeers: Which IMO then makes total perfect sense
13:32:32 Fabianx-screen: That sounds good.
13:32:40 WimLeers: and the interface name makes sense too
13:32:43 WimLeers: <story>
13:32:51 WimLeers: I'm building a render array — yay!
13:33:00 WimLeers: But a part of it can only be visible for some users
13:33:13 WimLeers: So I have an AccessResult. And my render array depends on it
13:33:54 WimLeers: And I'm rendering an entity as part of the render array.
13:34:00 WimLeers: So my render array depends on it.
13:34:04 WimLeers: So my render array has 2 dependencies.
13:34:16 WimLeers: So I better ensure the render array is varied by them.
13:34:19 WimLeers: </story>
13:34:23 WimLeers: IMO that makes a lot of sense

4. Making it easier to make the render array reflect dependencies

13:34:35 WimLeers: So then the next question is: how do we make the render array reflect that dependency?
13:34:58 WimLeers: We've been going in the direction of BubbleableMetadata::createFromCacheableObject($something)->applyTo($render_array)
13:35:03 WimLeers: But what if we flip that around?
13:35:08 WimLeers: My render array DEPENDS on this thing
13:35:14 WimLeers: So why not put it INSIDE the render array?
13:35:29 WimLeers: #cache['dependencies'][] = $access_result
13:35:32 WimLeers: #cache['dependencies'][] = $entity
13:35:36 WimLeers: just thinking out loud here
13:35:51 WimLeers: Then the Renderer could automatically absorb those
13:36:12 WimLeers: "absorb" == merge their cacheability metadata
13:37:33 WimLeers: Fabianx-screen: ^^
13:37:52 Fabianx-screen: hm
13:38:10 Fabianx-screen: I don't think that is a good idea as storing large objects never was a good idea.
13:38:23 WimLeers: yeah I'm hesitant because of that too
13:38:34 WimLeers: But BubbleableMetadata::createFromCacheableObject($something)->applyTo($render_array) is relatively scary
13:39:02 Fabianx-screen: I mean we can have #cache['dependencies'][] = $access_result->getCacheableDependencies();
13:39:11 Fabianx-screen: and have a CacheableDependencyTrait
13:39:46 WimLeers: I don't grok ::getCacheableDependencies()
13:39:49 Fabianx-screen: But in the end we need to merge.
13:39:53 WimLeers: y
13:40:01 Fabianx-screen: WimLeers: It just returns the render array properties relevant.
13:40:19 Fabianx-screen: $something->mergeDependencies($render_array)
13:40:20 WimLeers: we don't want that render array property logic littered over all our objects
13:40:36 Fabianx-screen: and mergeCacheableDependendies is a trai then?
13:40:40 Fabianx-screen: trait
13:40:47 WimLeers: yeah I was thinking traits too
13:41:12 Fabianx-screen: Nicer than global ones.
13:41:14 WimLeers: ideally we'd have something like this
13:42:03 WimLeers:
use SomeTrait;

$render_array = […];

$access_result = …;
$config = …;

$this→use($access_result);
$this→use($config);
13:43:01 WimLeers: $this→traitMethod($render_array)→use($access_result)→use($onfig)
13:43:38 WimLeers: $this→vary($render_array)→by($access_result)→by($config)
13:43:41 WimLeers: Fabianx-screen: ^^
13:43:43 WimLeers: that last one!
13:43:52 WimLeers: VaryRenderArrayTrait
13:44:03 Fabianx-screen: That looks promising.
13:44:05 Fabianx-screen: catch: ^^
13:44:06 WimLeers: that's as close to natural language as we can get
13:44:19 WimLeers: catch:
use VaryRenderArrayTrait;

$this→vary($render_array)→by($access_result)→by($config)


13:44:54 catch: That does look promising
13:45:03 WimLeers: that significantly lowers the bar
13:45:05 catch: *not doing a good job of keeping up with all the backscroll today.*
13:45:11 WimLeers: catch: there's a lot of it :)
13:45:17 Fabianx-screen: WimLeers: I like that, a strong reason I use Mockery, too for the natural language part.
13:45:30 WimLeers: I would totally love to use that.
13:45:49 WimLeers: But I wonder if we can somehow detect/disallow render arrays to vary by ANYTHING if they don't use this?
13:46:32 Fabianx-screen: Why does it need $thos?
13:46:35 Fabianx-screen: $this?
13:46:40 WimLeers: oh you're right it doesn't
13:46:51 Fabianx-screen: Cache::varyRenderArray($array)->by($foo)
13:47:11 Fabianx-screen: or RenderCache::vary($array) ;)
13:47:11 WimLeers: I used $this because I proposed it as a trait
13:47:15 Fabianx-screen: yes
13:47:33 WimLeers: hmmm fascinating
13:47:35 Fabianx-screen: But yes its the view builder using the config and the access_result etc.
13:47:41 Fabianx-screen: so itn eeds to apply it.
13:47:43 Fabianx-screen: needs
13:48:14 Fabianx-screen: RenderCache::vary($array)->by($foo, $bar); for the non foreach case.
13:48:32 Fabianx-screen: or
13:50:30 WimLeers: or?
13:50:59 Fabianx-screen: I threw away my or I did not like Render::varyCache($array)->by()
13:51:03 Fabianx-screen: :)
13:51:29 WimLeers: heh :)

5. Wrapping up…

13:52:31 catch: So is anything left on blocks then?
13:57:54 WimLeers: catch: currently, we're not saying anything needs to change there. See option a here:

https://pad.lullabot.com/p/cacheableinterface
14:00:30 Fabianx-screen: WimLeers: Not sure we even would want to rename it.
14:00:47 Fabianx-screen: WimLeers: Its an API change which can be 'fixed' with documentation
14:01:54 WimLeers: Fabianx-screen: That's true. But there would be *zero* disruption.
14:01:54 Fabianx-screen: WimLeers: I added Option C for that.
14:02:08 WimLeers: Fabianx-screen: BlockPluginInterface extends it. So no API change there.
14:02:30 WimLeers: And AccessResult is the only other implementor, and that would need to switch to the other interface anyway
14:02:35 Fabianx-screen: okay
14:02:44 WimLeers: So *only* existing contrib modules implementing this interface would need to change, which is probably zero.
14:02:56 Fabianx-screen: CacheableInterface extends CacheableDependencyInterface
14:02:59 WimLeers: yes
14:03:08 WimLeers: I didn't mention that in the doc, but yes
14:03:31 Fabianx-screen: I think with the extends, the rename makes less sense.
14:03:39 Fabianx-screen: even if there is zero disruption.
14:03:48 WimLeers: ok
14:04:06 WimLeers: The docs of CacheableInterface are surely the most misleading part
14:04:11 WimLeers: they indicate genericness
14:04:17 Fabianx-screen: yes
14:04:41 WimLeers: ok, afk for a bit, food
14:04:51 Fabianx-screen: On the other hand you can build a render array #cache part purely from CacheableInterface.

So, to tie the discussion in this issue together with the one on IRC (transcript in previous comment): we mostly came to to the same conclusions.

And I'd like to +100 this from #23:

CacheableInterface implies that that object is cacheable and that the corresponding methods are to enable caching that object itself. Which makes no sense, since you can't ask an instantiated object how to get itself from cache: you need to get it from cache in order to have the instance to begin with.

Patch proposal

From most important and seemingly-most-agreed-upon to least.

1. Extract the context/tags/max-age bits out of CacheableInterface into CacheableDependencyInterface, let CacheableInterface extend that interface.
   • This is a pure API addition!
   • "CacheableDependencyInterface" name is TBD, and is the same as the "CacheAffectInterface" @effulgentsia mentioned in #22
2. Let EntityInterface and ConfigBase implement CacheableDependencyInterface.
   • This is technically an API change, but we have base classes for both entities and config that are used by the 99%, so effective disruption is zero.
3. Add a nicer API to make render arrays depend on/vary by cache tags/contexts:
   • Again, pure API addition.
   • Renderer::vary(array $render_array), which then allows us to do Renderer::vary($build)->by($access)->by($config). Or Renderer::depends($build)->on($access)->on($config)?
   • Or, with a lesser DX: BubbleableMetadata::createFromCacheableDependency(CacheableDependencyInterface $dep), which then allows us to do
     

     BubbleableMetadata::createFromRenderArray($build)
       ->merge(BubbleableMetadata::createFromCacheableDependency($entity))
       ->merge(BubbleableMetadata::createFromCacheableDependency($config)
       ->applyTo($build)
     

And the lullapad link cited in #26, attached for posterity.

Another idea.

Keeping all patch proposal points in #27 except point 3, replace that with:

1. Cacheability class, which is a value object containing all cacheability metadata (contexts/tags/max-age)
2. Then: $build['#cache'] = new Cacheability()
3. And: $build['#cache']->merge($access_result)->merge($config)

… i.e. if there is a value object in the render array itself, it removes the unavoidable requirement to take the data in the render array, merge some other object's cacheability with it and then re-apply it onto the render array. We can then reduce that to a single step, that doesn't require knowing some static method, and the merging happens right in the context of where you expect it to happen: right where #cache lives.

That's fine, #30 was just another idea. For now, we'll go with #27. Looking forward to @effulgentsia's feedback in particular.

::getCacheTags() has always been about this object, the object that it's called on. Bubbling is done during rendering, where nested dependencies are reflected by nested render arrays, with each level of nesting have its own level in a render array, and its own cache tags/contexts/max-age.

But I see what you're saying: there may be use cases where an object receives a whole bunch of other objects and we want to know the aggregated cache tags/contexts/max-age. For that (currently still hypothetical) use case, we could introduce e.g. a NestedCacheableDependencyInterface in the future, with ::getAggregateCache(Tags|Keys|MaxAge)(), to address the use case you describe. (Where that interface also conveys that the object in question can depend itself on objects implementing (Nested)CacheableDependencyInterface.) But I think that's the least of our problems right now.

I'll get started on a patch for #27 tomorrow then!

Completely new patch. Implemented #27, points 1 and 2.

Implemented #27.3, but based on #38, I went with:

RendererInterface::dependsOn(array &$elements, CacheableDependencyInterface $dependency)

To demonstrate its use, I converted all the calls criticized in #2407765-46: Wherever simple config is used to render output to end user, associate their cache tags (i.e. the very comment that triggered this issue originally). This now leads to calls like

    Renderer::dependsOn($form, $config);
    Renderer::dependsOn($form, $field_definition->getConfig($entity->bundle()));

Quite elegant, I think. I'll let the interdiff speak for itself.

Note: Alex Pott originally said in #2407765-47: Wherever simple config is used to render output to end user, associate their cache tags that he'd prefer to not see it work by reference. I agree with that in principle, but I think it's hard to argue with the excellent DX above.

Spent a lot of time debugging the failures in #39. Started with what seemed the easiest: PathLanguageTest. Turns out that's a bug in language negotiation/language manager, which then naturally causes the cache context to be wrong too, and hence causes these failures. The good news: #2424171 actually fixes this — see #2424171-24: Language module vs. content translation module interaction exposes content translation bug.

The next step was to look at the other test failures. They all have one thing in common: the test failures are triggered by the change below. Basically, this gets rid of the hardcoded key in favor of using the entity's cache contexts.

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -169,13 +169,20 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
       '#cache' => array(
         'tags' => Cache::mergeTags($this->getCacheTags(), $entity->getCacheTags()),
-        'contexts' => [
-          'theme',
-          'user.roles',
-        ],
+        'contexts' => $entity->getCacheContexts(),
+        'max-age' => $entity->getCacheMaxAge(),
       ),
     );
 
+    // @todo This will go away thanks to:
+    //   - https://www.drupal.org/node/2099137 (will remove 'user.roles')
+    //   - https://www.drupal.org/node/2453059 (will remove 'theme')
+    // It's only moved out o/t above code to show what the end result will be.
+    $build['#cache']['contexts'] = Cache::mergeContexts($build['#cache']['contexts'], [
+      'theme',
+      'user.roles',
+    ]);
+
     // Cache the rendered output if permitted by the view mode and global entity
     // type configuration.
     if ($this->isViewModeCacheable($view_mode) && !$entity->isNew() && $entity->isDefaultRevision() && $this->entityType->isRenderCacheable()) {
@@ -188,10 +195,6 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco

@@ -188,10 +195,6 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
         ),
         'bin' => $this->cacheBin,
       );
-
-      if ($entity instanceof TranslatableInterface && count($entity->getTranslationLanguages()) > 1) {
-        $build['#cache']['keys'][] = $langcode;
-      }
     }

This works great in 99% of use cases. It breaks down in one case: where the code determining which entity gets rendered overrides the langcode to use, and thus which variation of an entity is used. In that case, the language cache context doesn't matter. And this is exactly what the failing tests do: they are for Views that are configured to e.g. list Spanish and French translations of nodes. I.e. it doesn't matter what the negotiated content language is; the View ignores that and lists those translations regardless of the content language!

This is of course a perfectly valid use case. So, updated accordingly.

This should thus fix 26 of the 27 failures in #39.

#43: yep, that's wrong. We do have failure because of that: two contact test suites have failures, see #40's test results :)

The problem #43 pointed out was responsible for 52 of the 52 additional failures (additional compared to #39).

This should bring the number of failures down to 1 — that last failure is fixed by #2424171: Language module vs. content translation module interaction exposes content translation bug. It's not in any way caused by this issue; this issue merely surfaces that same problem.

#45 has only 1 failure, as predicted :)

Per #23/#24/#25/#27, removing CacheableInterface altogether. First step: updating AccessResult to use CacheableDependencyInterface instead.

While doing so, I noticed that AccessResult still contained ::getCacheBin(), despite that being removed from CacheableInterface in #2339373: Remove getCacheBin() from CacheableInterface. We forgot to remove it there (almost 6 months ago). Removing that as well as part of this patch.

Looks like this single line that I forgot to update caused 2315 fails :)

Should be back down to 1 fail!

The only remaining user of CacheableInterface left now is BlockPluginInterface. Doing that would become significantly simpler once we've done #2428805: Remove the ability to configure a block's cache contexts. So, doing that first. Just posted an initial patch.

That being said, the current patch here can already be reviewed.

Postponed on #2424171: Language module vs. content translation module interaction exposes content translation bug and #2428805: Remove the ability to configure a block's cache contexts.

#2424171: Language module vs. content translation module interaction exposes content translation bug landed. Re-testing. Should come back green. That allows me to mark this issue as NR. Updating block plugins' use of CacheableInterface still needs to happen, but that doesn't mean this can't be reviewed already.

#2428805: Remove the ability to configure a block's cache contexts landed, now unblocked.

Straight reroll.

Next: quoting #55:

The only remaining user of CacheableInterface left now is BlockPluginInterface. Doing that would become significantly simpler once we've done #2428805: Remove the ability to configure a block's cache contexts. So, doing that first. Just posted an initial patch.

That being said, the current patch here can already be reviewed.

Fixed the todos that were blocked on #2432837: Make cache contexts hierarchical (e.g. 'user' is more specific than 'user.roles'), which has since landed. This should fix at least some of the test failures.

All fails occur when language.module's ConfigurableLanguageManager is in use instead of the default LanguageManager. Turns out the language cache context's code had a small bug due to the misleading LanguageManagerInterface API.

Actually, I propose that we do this in a follow-up issue:

The only remaining user of CacheableInterface left now is BlockPluginInterface. Doing that would become significantly simpler once we've done #2428805: Remove the ability to configure a block's cache contexts. So, doing that first. Just posted an initial patch.

That being said, the current patch here can already be reviewed.

That keeps this patch small & focused. Removing CacheableInterface will require relatively many & tricky changes, unlike the patch here.

To show that it is very doable however, I've already opened an issue that includes a patch to do just that: #2459819-3: Remove CacheableInterface (and no longer let block plugins implement it). Turns out that it's actually easier than expected: only a 10 KB patch is necessary. But I still think it's better to do it in a follow-up issue instead of here, because I suspect there will be quite a bit of discussion/bikeshedding on that, whereas we have agreement on the approach for this patch, so we can continue with the bulk of the work here IMO.

• Did a self-review, cleaned up a bunch of nitpicks.
• Added tests. (For BubbleableMetadata::createFromObject() and Renderer::dependsOn().)
• Made BubbleableMetadata implement CacheableDependencyInterface.

+++ b/core/modules/comment/src/CommentViewBuilder.php
@@ -11,6 +11,7 @@
+use Drupal\Core\Render\Renderer;

This is a remnant that should be removed.

My conclusion from my discussion with @effulgentsia, with a few more details than #68:

1. remove entity cache context stuff, defer to follow-up (#68.1)
2. static ::dependsOn() -> non-static ::addDependency() (#68.2)
3. follow-up: remove the other static methods on Renderer(Interface)

On it, tomorrow :)

• Fixed #67 + #68.1/#69.1.
• Filed issue for #69.3: #2460695: No methods on RendererInterface should be static.

Next: #68.2/#69.2.

Fixed #68.2/#69.2.

Updated IS.

Further improved IS.

1. Added beta evaluation.
2. Added before vs. after to IS:
   

   +++ b/core/modules/forum/src/Controller/ForumController.php
   @@ -191,7 +203,7 @@ protected function build($forums, TermInterface $term, $topics = array(), $paren
   -    $build['#cache']['tags'] = Cache::mergeTags(isset($build['#cache']['tags']) ? $build['#cache']['tags'] : [],  $config->getCacheTags());
   +    $this->renderer->addDependency($build, $config);
   

Testbot fail: General error: 13 database or disk is full

sigh

Testbot-- :(

Now waiting for this testbot to be killed.

After I was able to access the log of the latest failed run, there was actually useful information in the log, unlike after the first failure: this time it had so many failing tests that it somehow was filling up the DB/disk completely.

Turns out I made a typo… and copy/pasted it several times. Interesting consequences :)

I think we actually want to:

1. Update https://www.drupal.org/node/2184553 (CR for CacheableInterface introduction)
2. A new CR for the newly added RendererInterface::addDependency()
3. Update https://www.drupal.org/node/2337377 (CR for AccessResult, which explicitly mentions CacheableInterface)

Already did point 2: https://www.drupal.org/node/2460819. I'll do points 1 and 3 as soon as this lands (I already associated them with this issue).

Thanks!

The test failures stemmed from #2458349: Route's access result's cacheability not applied to the response's cacheability, but now that #2453059: Set default render cache contexts: 'theme' + 'languages:' . LanguageInterface::TYPE_INTERFACE has landed, we can indeed simplify the @todo mentioned in #93.

OMG YAY!!!!

This unblocked #2459819: Remove CacheableInterface (and no longer let block plugins implement it). And did a small bit of the work necessary in #2099137: Entity/field access and node grants not taken into account with core cache contexts, hence forcing the need for a reroll of that one.

Quoting #83:

I think we actually want to:

1. Update https://www.drupal.org/node/2184553 (CR for CacheableInterface introduction)
2. A new CR for the newly added RendererInterface::addDependency()
3. Update https://www.drupal.org/node/2337377 (CR for AccessResult, which explicitly mentions CacheableInterface)

Already did point 2: https://www.drupal.org/node/2460819. I'll do points 1 and 3 as soon as this lands (I already associated them with this issue).

Now did points 1 & 3 and published the new CR in point 2.

Now also updated the handbook page at https://www.drupal.org/developing/api/8/render/arrays/cacheability