Split off CacheableTrait

I wanted to RTBC, but hm, neither the phpDoc of this trait nor the phpDoc of the interface really explains how + where + why this is supposed to be used.

The trait just points to the interface, and the interface merely says "interface for objects which are potentially cacheable." — So for example, my entity data object is cacheable, should it implement Cacheable? What's appropriate use, what's not?

IMO, every trait should have very clear docs on it that educate you about intended/proper vs. inappropriate usage, because alas, it's just about compile-time copy/paste.

mmm, I intentionally ignored that issue thus far, because (as usual) I think it's too big of a hammer.

The trait code + methods + basic phpDoc looks perfectly fine in this patch. It follows our existing coding standards, so I don't really see why we need a new "strategy" for documenting traits.

All I was asking for was a general usage advice on the phpDoc of the trait and/or interface itself. That's not strictly related to the topic of traits, it's just a general means of providing high-level documentation for the classes/resources that you're offering/supplying to developers and potential consumers.

It's a general problem in most of our new OO code - there's close to zero documentation on individual resources that educates about intended usage. Thus, to figure that out, you have to grep the whole code base to see where something is used (and how), and whether that usage is "similar" to your use-case, and if so, you can just "hope" that using it for your use-case is appropriate.

I raised the question, because it isn't really clear to me what code is supposed to use this interface/trait (and what code isn't). To answer the question myself, I just did what I just mentioned and checked the current usages in HEAD:

CacheableInterface is implemented by HtmlFragment (which is updated here) and BlockBase (not updated here). It is also part of BlockPluginInterface.

Based on that, the implied purpose appears to be that all HTML fragments and blocks are seemingly supposed to implement the interface and may opt to use the trait. I don't know whether that is right or wrong. It's just the manually derived documentation based on available information.

1. +++ b/core/lib/Drupal/Core/Cache/CacheableTrait.php
   @@ -0,0 +1,77 @@
   +trait CacheableTrait {
   ...
   +  protected function initializeCacheable($cache_info = []) {
   +    $this->cache = $cache_info + array(
   +      'keys' => array(),
   +      'tags' => array(),
   +      'bin' => NULL,
   +      'max_age' => 0,
   +      'is_cacheable' => TRUE,
   +    );
   

   keys, tags, bin, max_age are not really about whether something is cacheable. Is Cacheable really the name - made CacheMetadataTrait or something. How generic and reusable is that?

2. +++ b/core/lib/Drupal/Core/Cache/CacheableTrait.php
   @@ -0,0 +1,77 @@
   +  protected $cache = array();
   

   the $cache property is often used in objects to store the cache backend eg Drupal\Core\Asset\LibraryDiscovery or a static cache eg. Drupal\Core\State\State. Maybe should be $cacheMetadata?

Assigned to catch for a second opinion.

I don't like that #2256373: Factor HtmlFragment out to an interface got away with HtmlFragmentInterface not extending CacheableInterface. Every HTML fragment should indicate its cacheability.

If relatively esoteric things like meta elements are supported by HtmlFragmentInterface, then why is something as fundamental as cacheability not supported?

Please rectify this.

It sounds like you're contradicting yourself? In #2256365: Factor render->fragment code out to a service's issue summary, you wrote:

4) Any cache instructions on the render array should be transferred to the fragment.

not every conceivable HtmlFragment-esque object necessarily needs an understanding of our cache tags system

Wrong. It's true that not every object that implements HtmlFragmentInterface needs cache tags. But CacheableInterface is not solely about cache tags.
Any HTML fragment that is cacheable but needs to be invalidated automatically based on changes made to other things, needs to support cache tags. Those that don't need it… can just return the empty array, to indicate they don't have any cache tags.

It is true that arguments are to be made to split up CacheableInterface in more granular interfaces. I even made that argument. And I lost. Because at least for now, it's simpler to keep a single CacheableInterface, until we have sufficient use cases in core to see how we should split up CacheableInterface.

Surely you want every HTML fragment to have a way to say whether it's cacheable? Surely you want it to have a max-age?

@Crell I think Wim made that argument originally then I repeated it to you in irc...

I think we should look at splitting cache tags out of CacheableInterface to its own. Not sure about anything else yet, although yes ttl/max_age probably makes sense as well.

There's an increasing amount of cases where we have something that needs to specify cache tags, but doesn't have a cid/ttl/bin etc. because they're about communicating information up to the various levels where the HTML will get cached at similar to assets. Is different from the bin/granularity/ttl at the same level.

With CacheableInterface now removed (#2459819: Remove CacheableInterface (and no longer let block plugins implement it)), and this not being touched in >9 months, I think it's fine to now close this.