Follow-up for #2483183: make the Breadcrumb value object use composition instead of inheritance

This is my definition of composition.

Oh btw. the patch is much smaller now because we are using composition which decouples changes in one piece of code from their parent classes.

[edit: crosspost with @dawehner, but I guess I'm basically saying something similar :-) ]

Not 100% familiar yet with the classes and APIs around cacheable metadata, but I would have naively expected a change from "Breadcrumb is a CacheableMetadata" to "... has a CacheableMetadata(Interface)", with CacheableMetadata being the unique and universal value objects for cacheability metadata.

But turns out you can have cacheability metadata carried outside a CM object, with tags, contexts, etc. in raw properties of objects implementing their own, unrelated primary domain interface ?

So cacheability metadata can be carried in pure value objects, available for composition, or directly in other domain objects by interface inheritance & helper traits ?

The only reason this patch is smaller is because it doesn't introduce CacheableDependencyTrait and reverts the clean-up elsewhere I can do that too :). Rerolled, relative to #5 again.

Well I just thought its out of scope :) Anyway great work!

So, given that… what's not to like? We have a Breadcrumb value object that has some of its own data, but then also implements RefinableCacheableDependencyInterface. We don't want to write custom logic for that, so we just compose it using the RefinableCacheableDependencyTrait. That's it!

Well, its still fundamental something different. You implement a breadcrumb against an abstract interface regarding one concrete implementation.
So for example $breadcrumb = new BreadCrumb(new ForumThread()); and boom, the breadcrumb inherits everything from the forum thread (for sure more of a pseudo code).
Traits IMHO are, and really just should be, a tool for c&p code. Once you start thinking about that, you realize that c&p in the first place shows bad design in your software and you
maybe do something better.

One drawback to using composition in place of inheritance is that all of the methods being provided by the composed classes must be implemented in the derived class, even if they are only forwarding methods.

Not sure that's the case here, since we already have the CM value objects that provides the API. No need to forward those, just provide access to the contained value object ?

So instead of :
$breadcrumb->addTag($tag);
you would write :
$breadcrumb->getCacheableMetadata()->addTag($tag);
?

Well yeah, we want to keep implementing the interface this is for sure.

Agreed on the principle, but I think this is the exception. Cacheability metadata is not something vast. It's very specific, very well-defined. And it's something many things want/need. Therefore I think that specifically for the case of value objects that also have cacheability metadata, traits are an excellent choice.

Well, then you could ask why we have the interface kinda in the first place to be honest ... you know what I mean.

Let's make another point. Inheritance is for using a "A is a B" relationship. "A breadcrumb is a CacheableMetadata" ... For me its not the fundamental property of the breadcrumb to be a cacheable metadata enriched thing.

What about asking crell about it? Pinged him on IRC

having a getCacheableMetadata() method would be problematic: it'd mean that the Breadcrumb value object no longer provides cacheability metadata itself, which means it no longer is a CacheableDependencyInterface implementation.

Which means it would break the entire point of CacheableDependencyInterface: objects you use and interact with, if you use them in a computation, their cacheability metadata should be easily trackable. This breaks that ease. Because you wouldn't be able to do ->addCacheableDependency($breadcrumb) anymore

Yeah, I guess that's the crux of what I'm having troubles mapping out exactly, the coexistence of :
- CacheableMatadata : pure value objects holding [contexts, tags, max-age]
- and CacheableDependencyInterface : (other-)domain objects that, on top of their own business logic and properties, directly hold contexts, tags, max-age information without the wrapping value object.

In short, what puzzles me is why CacheableDependencyInterface is :
getCacheContexts()
getCacheTags()
getCacheMaxAge()

rather than specifically "I hold cacheability metadata" (= I "have a" CacheableMetdataInterface, composition), with just :
getCacheableMetadata()
and CM value objects being the universal way to carry or manipulate [contexts, tags, max-age]

(on a related note, CacheableMetadata trips me up each time as implying that it is metadata that you can cache. Rather, it's the metadata describing the cacheability of something else --> CacheabilityMetadata would be more accurate ?
CacheableDependencyInterface or "being a cacheable dependency", well, doesn't really carry a clear meaning to me... :-p)

dawehner asked me to jump in here.

Traits in PHP are compile time copy-paste. That means they are best suited for cases where the alternative is code-time copy-paste. If the code in question is something that logically makes sense on its own, it should be a composed object. If not, and it's just boilerplate you don't want to retype, a trait a great fit.

Class implements interface: "can be treated as a"
Class extends class: "Is a special case of"
Class happens to use the same stock code as Class: Trait

"is a" is too generic a relationship, but "can be treated as" and "is a special case of" tend to work as guidelines.

In this case, then, I would say (and this goes a bit into Breadcrumb, too, since I didn't even see the breadcrumb issue until it got committed):

Breadcrumb is an object until itself. It is not a special case of anything, thus should extend nothing.
Breadcrumb is, conceptually, an ordered collection of links/Link objects. Thus it should be coded as a collection. Maybe with ArrayAccess, or Traversable, or something like that.
Breadcrumb can also expose cache metadata. Thus, it should implement whatever the "Yo, I have cache metadata you should ask about" interface is. (The names here still confuse the heck out of me, but that's what it means semantically.)
If a significant percentage of objects that say "yo, I have cache metadata" have the same implementation of that interface, because it's just boilerplate array property manipulation, then that's totally a good use of a trait.

It sounds like part of the question here is "what interface means 'yo, I have cache metadata you should ask about'?" If that's a hard question it indicates that the interfaces may need to be refactored, if possible, because that really needs to NOT be a hard question. :-)

#21:

"What I'm basically proposing here is..." - Yes please!

@Crell: how is CacheableDependencyInterface + CacheableDependencyTrait different from AttachmentsInterface + AttachmentsTrait, which we added together in #2407195: Move attachment processing to services and per-type response subclasses?

I give up, how? :-) Seems like the exact argument applies here.

Composition would make sense if:
1) The object in question, if it existed, would have dependencies of its own.
2) The object in question, if it existed, would have meaning unto itself as a complete "thing".

Neither is the case here. A trait is the right tool.

Or in other words...

I discussed this with catch almost 2 months ago and he agreed we should rename CacheableMetadata to Cacheability. I fear it's too late now though :(

Yeah :-/ Given the importance render cache has in D8 (currently and even more after SmartCache), and the fact that it's ... complex enough ;-) (see the discussions in #2556889: [policy, no patch] Decide if SmartCache is still in scope for 8.0 and whether remaining risks require additional mitigation), I'd think not having misleading names add to the complexity is worth the disruption. I'll create the issue and ask for release manager feedback.

Composition would make sense if:
1) The object in question, if it existed, would have dependencies of its own.
2) The object in question, if it existed, would have meaning unto itself as a complete "thing".

So that's what trips me up. The object in question does exist, and does have meaning as a complete "thing" : it's CacheableMetadata (which would be better named CacheabilityMetadata or Cacheability, as per beginning of #21), which *are* the value objects holding "cacheability info" (= [contexts, tags, max-age]).

So, unless I'm missing something :
- Those CM objects are used for composition in some places (some things "have a" CM that describes their cacheability)
- And some other objects chose to *not* "have a CM", completely overlook the existence of that "value object" class, and instead directly hold [contexts, tags, max-age] properties among their other business properties (with direct getMaxAge(), getContexts() methods in the middle of their other business methods)
- The latter is what our current "I have cacheability metadata" interface, CacheableDependencyInterface, encourages

Two patterns for carrying "cacheability metadata", sometimes with composition and a CM value object ("I have a"), sometimes with inheritance and direct properties/methods ("I am a"), is what confuses me. It also means we have to duplicate the APIs that provide easy manipulation of the metadata ("add tag", "reduce max age"...) ?

Opened #2561773: CacheableMetadata is misnamed

Thanks @Wim, I replied in #2561773-7: CacheableMetadata is misnamed

Fine with RTBC here then :-)

I nearly asked for a change notice but for something that's only existed for weeks it's probably better to have nothing.

Committed/pushed to 8.0.x, thanks!

This needed an update to this change notice, though : https://www.drupal.org/node/2543936 , because setCacheContexts is replaced by addCacheContexts() with this patch. Added.