CacheableMetadata is misnamed

@yched++

Quoting a new comment I just posted: #2551907-26: Follow-up for #2483183: make the Breadcrumb value object use composition instead of inheritance:

#25: The sole reason CacheableMetadata exists as its own thing, is to do calculations with cacheability metadata. I.e. for internal uses. Perhaps it should've been marked as @internal. (Perhaps we can do that in #2561773?)

So yes, every class should implement CacheableDependencyInterface, and no, you should generally not ever interact with CacheableMetadata.

The main purpose of CacheableMetadata is for computing the cacheability of various different CacheableDependencyInterface objects (access results, renderables, Entities, Contexts, et cetera) after they have been combined in some way to compute something else.

catch and I prefer Cacheability.

My reasoning: the "metadata" suffix is unnecessary, because "cacheability" already implies metadata: a value object capturing "cacheability" cannot have any other meaning, purpose or interpretation than "metadata". So, including the "Metadata" suffix would make it a pleonasm IMO.

Attached is the minimally viable patch.

The name CacheableDependencyInterface was selected after extensive debate in #2444231: Fix CacheableInterface so it makes sense (was: "Make Config objects & Entities implement CacheableInterface + add BubbleableMetadata::createFromCacheableObject()").

The reasoning is: this object can be a dependency of another object (or some computation), and it's not a pseudo-random object, no, it is an object that, if it is a dependency of something, is cacheable, i.e. it does not prevent caching.

(First it was called CacheableInterface, but we decided that made no sense in #2444231-22: Fix CacheableInterface so it makes sense (was: "Make Config objects & Entities implement CacheableInterface + add BubbleableMetadata::createFromCacheableObject()"), -23, -26 and -27.

RefinableCacheableDependencyInterface is a more recent addition (also selected after extensive debate, in #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity), and is layered on top of the above reasoning. Its reasoning: this object that is a cacheable dependency also has variants (think entity translations, it's always the same underlying entity, but just with the data it contains being different); which variant it is depends on external factors. It is then refinable so you can add cache contexts/tags, so the loaded object can itself contain the reason why that specific variant (e.g. entity translation) was loaded (e.g. the "negotiated content language" cache context).

Also, this is all documented over at https://www.drupal.org/developing/api/8/cache/cacheable-dependency-inter... :)

Would s/CacheableDependencyInterface/HasCacheabilityMetadataInterface be accurate ? (leaving aside "feasibility and impact of such a rename at this point" - just trying to see if I get the concepts and model right)

Theoretically that could work, but it takes a slightly different world view.

With CacheableDependencyInterface, we say that an object that implements it, and when that object is used as a dependency in some computation, that it allows that computation's result to be cached. This is the entire point of cache tags, contexts and max-age. That cacheability metadata is inherent to the object in question (think a Node entity's node:5 cache tag).

With HasCacheabilityMetadataInterface, we say that an object has a value object that contains cacheability metadata. The same things are possible. But it's not as strong a connection: it does not communicate the inherent nature, nor does it communicate the point of this metadata. It almost seems like a "oh, by the way, I have this pile of metadata too".

Patch in #6 did not apply, attached a reroll. I'm also +1 to renaming to Cacheability.

CacheableDependency seems to qualify those objects as "cacheable", while this is not about caching *them*.

Hah! That's actually exactly why it's called cacheable dependency interface, and not cacheable interface! Because it's not about caching them, but they may be a dependency of something that we end up caching! That's what I tried to explain earlier but clearly did not succeed.
Does that make sense now?
Can you think of a better way to explain it?
Great discussion :)

Actually, the real understandability win would be to rename the various getCacheableMetdata() methods accordingly...

Just to make sure I understood that correctly… you mean renaming them to getCacheability(), right?

AFAICT that affects CacheableResponseInterface and CacheContextInterface - the latter of which has many implementations :-/, but maybe not that much in contrib yet ?

Correct, I don't think there's any yet. It's only necessary for advanced use cases anyway (for every custom thing to vary by). So disruption would be very minimal.

Shaping the interface name to qualify the object, rather than the other objects using that object, means you don't need to find ways to convey that some parts of the name qualify the object ("dependency") and some other parts qualify the "other objects using that object" ("cacheable"), which seems quite a grammatical challenge :-)

This. That sentence alone summed up all of my confusion about CacheableDependency*. An interface should describe the object, not the relationship of an object to some undefined and arbitrary other object. That some other object happens to use it a certain way is incidental, by design, because different objects can use it in potentially different ways.

Sure, I got the explanation, but I'm really skeptical that we can force people's brains to understand "CacheableDependencyInterface",
- not as : "a dependency that is cacheable" (which it is not, but is how the name does read IMO)
- but as : "a dependency of something that is cacheable" (which it is) instead

No, it is a dependency that is cacheable.

You indeed don't know how an object is going to be used. The point is that any object that does implement CacheableDependencyInterface is a cacheable dependency (a dependency that is cacheable). So any object depending on it, if it wants to do caching, then it is possible if and only if it depends if all objects it depends on are cacheable dependencies.

they are "a dependency of something cacheable", not "a dependency that is cacheable"

No, it is "a dependency that is cacheable".

In "Entity instanceof CacheableDependencyInterface", it's not the Entity that is cached/cacheable, it's some render output, of which the Entity is a dependency.
Same in "FieldStorgeDefinitionInterface instanceof CacheableDependencyInterface", this is never about caching the field definition.
(those two happen to have their own caches, but that's irrelevant here, and possibly even adds to the confusion...)

100% correct.

CacheableResponseInterface, CacheableMetadata and CacheableDependency cannot use the same meaning of "cacheable"

I disagree.

CacheableMetadata is a misnomer: it's not the metadata that is cacheable, it's (a value object containing) metadata describing cacheability. So there, you are right. But that's also why we have this issue to rename it so it makes sense.

CacheableResponseInterface and CacheableDependencyInterface are consistent and have the same meaning. The first says "a responses that can be cached", the second says "a dependency that can be cached".

I think that this issue needs @effulgentsia, who is a native speaker, was instrumental in arriving at the name CacheableDependencyInterface, and is able to explain these things much more clearly than most of us most of the time :)

it's something with cacheability info so we *could* cache it if we wanted, but it so happens that we're not interested in *actually* caching it, we only use the cacheability info to cache something else that uses it as a dependency

Basically, yes.

I think a rename is still doable, as long as we keep supporting the old name, and just mark it deprecated.

It is too late (and already was when we opened this issue) to rename the methods I think.

Using this approach, we have zero disruption.

#25: no, that would still change the interface.

We could add a new interface extending the old interface for that. But probably in a followup?

I think it's indeed better to do that in a follow-up. This issue covers the most visible part.

This is a catch issue.

There is the thing of renaming, but can we please also get sane documentation?

+++ b/core/lib/Drupal/Core/Cache/Cacheability.php
@@ -0,0 +1,189 @@
+/**
+ * Defines a generic class for passing cacheability metadata.
+ *
+ * @ingroup cache
+ *
+ */
+class Cacheability implements RefinableCacheableDependencyInterface {

This documentation doesn't tell you much. It should be explained in which causes it should be used for example.

Totally agreed, doing that now.

Also, repeating what I quoted in #5, and with emphasis added:

#25: The sole reason CacheableMetadata exists as its own thing, is to do calculations with cacheability metadata. I.e. for internal uses. Perhaps it should've been marked as @internal. (Perhaps we can do that in #2561773?)

So yes, every class should implement CacheableDependencyInterface, and no, you should generally not ever interact with CacheableMetadata.

The main purpose of CacheableMetadata is for computing the cacheability of various different CacheableDependencyInterface objects (access results, renderables, Entities, Contexts, et cetera) after they have been combined in some way to compute something else.

Thoughts?

I rewrote and refined this at least five times. I hope you like it.

+++ b/core/lib/Drupal/Core/Cache/Cacheability.php
@@ -0,0 +1,202 @@
+  public function getCacheMaxAge() {
+    return $this->cacheMaxAge;
+  }
...
+  public function merge(CacheableMetadata $other) {
+    $result = clone $this;

Are we really sure we want to keep an API, which is sometimes immutable and sometimes not.

I (and many others) found this really confusing.

$cacheability->addCacheTags(['foo']);
$cacheability->merge($other_object);

Or even better:

$cacheability->addDependency($other_object);

...

Just some food for thought ...

Thanks a lot

#35: I share your concerns, but that's out of scope here. This is just about renaming. Very few things call this, so *maybe* and *hopefully* we'll be able to still fix that during RC. But we must assume it is too late to change that. We'll see. The reason this rename is possible still is that it doesn't break anything: we keep full BC.

Well, I prefer addDependency() anyway over merge, which is just an addition and way less overloaded in terms of mutability.

And no it is the addition of a new Interface, so we could do changes here - that is why I am asking if we do not want to not copy merge() over and leave merge() just on the old class.

merge() btw. takes only a CacheableMetadata anyway right now, so should that not be Cacheability?

That's a great point!

This still keeps full BC.

In doing that, I also noticed that Cacheability/CacheableMetadata still has getCache(Tags|Contexts|MaxAge)() methods, even though those are already provided by the RefinableCacheableDependencyTrait that it uses. So, simply removed those.

Little nits, can be fixed on commit:

+++ b/core/lib/Drupal/Core/Cache/Cacheability.php
@@ -0,0 +1,135 @@
+   * Applies the values of this CacheableMetadata object to a render array.
...
+   * Creates a CacheableMetadata object with values taken from a render array.
...
+   * Creates a CacheableMetadata object from a depended object.

Cacheability

HEAD still has 300 occurrences of "CacheableMetadata". I don't think we necessarily need to convert all of them in this patch, but I think updating the issue summary with a plan of what we think the eventual end state will be will help us evaluate whether this is a good idea. Is it possible to organize all of HEAD's usages into buckets (e.g., CacheContextInterface::getCacheableMetadata(), usages in \Drupal\Core\Menu, etc.) and figure out how this name change will eventually fit in to those mental models?

We could easily convert the vast majority in this patch, but the idea was to keep it as simple as possible here.

Regarding the several getCacheableMetadata() methods: my understanding is that those are frozen, and cannot be changed anymore, see #24 + #25 + #26.

I'm not quite sure what you want me to do here now?

In reading some of this issue's comments of people struggling with the concept of "cacheable dependency", I looked at the docs of CacheableDependencyInterface and found:

* All cacheability metadata exposed in this interface is bubbled to parent
* objects when they are cached: if a child object needs to be varied by certain
* cache contexts, invalidated by certain cache tags, expire after a certain
* maximum age, then so should any parent object.

I think this adds to the confusion, because the use of the word "bubbled" here I think means that "parent" refers to parent elements in the render array tree. In which case, these docs belong on CacheableMetadata (or with this patch, Cacheability). And the docs on this interface should not be about bubbling, but about how the information benefits / transfers to dependents (e.g., via ->addCacheableDependency()).

Re #42, sure, let's keep getCacheableMetadata() frozen, but change its @return to Cacheability.

And for everywhere where the CacheableMetadata class is still used, let's either convert all of those usages, or if it would simplify this patch, at least ones used in type-hints and docs of non-test code.

Also, something we should figure out here (perhaps via a comment from @catch) is whether we're expecting all of contrib to change their type-hints from CacheableMetadata to Cacheability. Because if not, then does that mean that core will need to keep getCacheableMetadata()'s and other @return types as CacheableMetadata? If so, I'm not sure if this patch really improves the mental model. Instead, it makes people have to remember when to use which name.

Sounds like this needs a decision or at least input from catch to move forward.

If Cacheability extended CacheableMetadata then doesn't #45 become a non-issue?

Also where are those contrib type hints? Why can't they type hint on RefineableCacheableDependencyInterface?

We've passed RC deadline on this one. Is this "rc target" eligible @catch?

Adding the new "rc target triage" tag per https://www.drupal.org/core/d8-allowed-changes#rc so that we triage it on our next triage call.

#47 still needs an answer to determine how much of a bc break this might or might not end up being.

Answering #47:

If Cacheability extended CacheableMetadata then doesn't #45 become a non-issue?

The problem with that would be that the return value from something that returns CacheableMetadata (e.g., all contrib implementations of CacheContextInterface::getCacheableMetadata()) couldn't be passed to something that improved its typehint to Cacheability.

Also where are those contrib type hints? Why can't they type hint on RefineableCacheableDependencyInterface?

I don't know what contrib functions typehint params to CacheableMetadata, but 2 core examples are MenuParentFormSelectorInterface::getParentSelectOptions() and CachePluginBase::alterCacheMetadata(). I don't think either of those examples would make much sense with RefineableCacheableDependencyInterface. There's not a dependency that you're refining (e.g., an entity being refined by a language); there's cache metadata that you're collecting/aggregating. So I'm guessing that there's likely to be some contrib/custom module in a similar situation of having a CacheableMetadata typehint, but I don't know.

Personally, per #45, I think the addition of Cacheability and deprecation of CacheableMetadata should be punted to 8.1 at this point.

I suppose one way to make this all BC would be to make Cacheability extends CacheableMetadata and CacheableMetadata implements CacheabilityInterface. Then @param typehints could be narrowed from CacheableMetadata to CacheabilityInterface while Cacheability return values could still be passed to all places that still typehint on CacheableMetadata.

I still think that's a better thing to do for 8.1 than 8.0, but I'm open to counterarguments or @catch deciding otherwise.

Pinged @catch about this, so he can decide.

This definitely needs to be 8.1.x now, and we should also use an approach like #52 so that there is not a BC break. Thanks!

+++ b/core/lib/Drupal/Core/Cache/CacheableMetadata.php
@@ -7,83 +7,14 @@
+ * @deprecated in Drupal 8.0.x, will be removed before Drupal 9.0.0. Use
+ *   \Drupal\Core\Cache\Cacheability instead.

The deprecation version needs an update in the next patch.

#52 is the sort of thing I was suggesting for this - so assuming it actually works out as non-breaking that sounds good. Un-assigning.

The problem with #52 is that we still can't get rid of the merge() method that is defined on CacheableMetadata: Cacheability extends CacheableMetadata means it inherits it!

The problem with that would be that the return value from something that returns CacheableMetadata (e.g., all contrib implementations of CacheContextInterface::getCacheableMetadata()) couldn't be passed to something that improved its typehint to Cacheability.

Could we turn it around and let Cacheability extend CacheableMetadata, and keep the code in the later for now, while still marking CacheableMetadata as deprecated?

Let's do this before Drupal 9 ships.

Sorry I'm nuking the Drupal 9 tag - this could happen in any minor release whether Drupal 8 or Drupal 9. Doesn't do any harm to do it sooner than later of course.

I think this is a great novice issue not to push across the finish line, but to at least get going again. I'd be happy to assist/help!

I contribute with this patch to this issue.

Thanks @vacho! Unfortunately it's not passing tests yet.

This seems to have gone under the radar again. I'd still like to see this happen, so assigning to myself. Already created one issue that makes this slightly simpler: #3071572: ContextCacheKeys should implement CacheableDependencyInterface rather than extend CacheableMetadata.

I am removing the Novice tag from this issue because there isn't anything to do if @Wim Leers is (still?) on the case. Feel free to unassign and promote the issue back though. :-)

Opinions: The patch, at this point, should probably just implement the new class and deprecate the old one for 10.0.0.

@Wim Leers can you please share your thoughts on #76? What should be the attack plan here?

It's been years since I looked at this. I think I agree with the plan @mradcliffe proposes in #76. Except that deprecating in 9.4 for 10 might be too little notice for something like this — but I can't decide that, only release managers can. Perhaps @catch has thoughts?

Unassigning after the stupid self-assigning I did 3 years ago in #73 🙈

There are about 40 pages of results on http://grep.xnddx.ru/search?text=CacheableMetadata&filename=&page=42, even with some false positives that's quite a lot.

Having said that, it'd be an easy rector rule.

I would probably go for deprecating in Drupal 9.4 for removal in Drupal 11 at this point - but no reason it can't happen in 9.4 with the later removal version.

This is a naming improvement rather than a bug as such, since it'll be a new deprecation, moving to 10.1.x

Quick note: We can use class_alias() to reduce the BC impact of renaming a class.
https://3v4l.org/80D6A

It does _not_ help us with renaming methods, but we can find other solutions for that.