Decouple cache implementation from the renderer and expose as renderCache service

This makes sense to me to use for multiple get/set. #pre_render just doesn't work at all for that.

First, adding formatting/structure to the IS, to make it more parseable. Then going to analyze it.

#2469277: Changing #cache keys during #pre_render or anywhere else leads to cache redirect corruption has been committed!

1. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -0,0 +1,309 @@
   +class RenderCache implements RenderCacheInterface {
   

   Just an idea, but… I think that in an ideal world, we wouldn't have a RenderCache service that is used directly by the Renderer service, but we'd instead have a service decorating the Renderer service, and in that way adding support for render caching.

   That'd mean Renderer would only have to care about rendering itself, not about render caching.

2. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -0,0 +1,309 @@
   +  protected $cacheContexts;
   

   $cacheContextsManager

More complete review to follow.

Haven't reviewed in depth yet but general +1 to splitting this out, and what I did review was encouraging.

Just a quick look to get some familiarity with this code.

1. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -0,0 +1,309 @@
   +    $bin = isset($elements['#cache']['bin']) ? $elements['#cache']['bin'] : 'render';
   

   Would it make sense to have a dedicated method for this?

2. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -0,0 +1,309 @@
   +      // @see ::doRender()
   ...
   +    // @see ::doRender()
   

   These need to be updated.

3. +++ b/core/lib/Drupal/Core/Render/RenderCacheInterface.php
   @@ -0,0 +1,78 @@
   + * Defines an interface for caching rendered data.
   

   This could be a great place to add some documentation about the key concepts of render caching: keys, contexts, tags and so on. Sorry, if they are already documented in a better place :)

4. +++ b/core/lib/Drupal/Core/Render/RenderCacheInterface.php
   @@ -0,0 +1,78 @@
   +   * Given a render array and its rendered output (HTML string), return an array
   

   Is there any value in assuming we are caching HTML markup?

5. +++ b/core/lib/Drupal/Core/Render/RenderCacheInterface.php
   @@ -0,0 +1,78 @@
   +   * If Drupal needs additional rendering metadata to be cached at some point,
   +   * consumers of this method will continue to work. Those who only cache
   +   * certain parts of a render array will cease to work.
   

   This paragraph is a bit confusing: it's not clear why Drupal would need additional metadata. As a consequence of a code change? Or new code installed? Maybe an example would help.

6. +++ b/core/lib/Drupal/Core/Render/RenderCacheInterface.php
   @@ -0,0 +1,78 @@
   +   * Gets the cached, pre-rendered element of a renderable element from the cache.
   

   80 chars exceeded, what about:

   "Gets the cached, pre-rendered element of a renderable element from cache."

7. +++ b/core/lib/Drupal/Core/Render/RenderCacheInterface.php
   @@ -0,0 +1,78 @@
   +   * @param array $pre_bubbling_elements
   +   *   A renderable array corresponding to the state (in particular, the
   +   *   cacheability metadata) of $elements prior to the beginning of its
   +   *   rendering process, and therefore before any bubbling of child
   +   *   information has taken place. Only the #cache property is used by this
   +   *   function, so the caller may omit all other properties and children from
   +   *   this array.
   

   Would it make sense to specify only pre-bubbling cacheability metadata then? I guess it would make the contract clearer.

8. +++ b/core/lib/Drupal/Core/Render/RenderCacheInterface.php
   @@ -0,0 +1,78 @@
   +   *  Returns FALSE if no cache item could be created, NULL otherwise.
   +   *
   +   * @see ::cacheSet()
   +   */  ¶
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function cacheSet(array &$elements, array $pre_bubbling_elements);
   

   PHP doc mess :)

#18.3: already documented at https://api.drupal.org/api/drupal/core%21modules%21system%21theme.api.ph... and https://www.drupal.org/developing/api/8/render/arrays/cacheability :)

Thanks, a couple of @see would be great, then :)

I'm going to take a stab at addressing all feedback.

#16:

1. Good point RE: too expensive.
2. Y, I know. I made the same remark there, but it landed anyway.

#18:

1. …
2. Done.
3. Per #19 + #20: referring to the existing docs.
4. Not really, but OTOH everything in Renderer is already HTML-specific anyway. Many concepts, and every template.
5. These are c/p'ed docs — they come from RendererInterface::getCacheableRenderArray(). Also not sure how to improve without making it super wordy.
6. Fixed.
7. Again c/p'd docs. +1 though, but Fabianx wanted to keep this more abstract, so that he wouldn't have to change the API again in #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts.
8. Fixed :)

Review:

+++ b/core/lib/Drupal/Core/Render/RenderCache.php
@@ -0,0 +1,309 @@
+  public function cacheGet(array $elements) {
...
+  public function cacheSet(array &$elements, array $pre_bubbling_elements) {

I'd argue these should just be get() and set().

The "cache" bit is implied by the service name.

+++ b/core/lib/Drupal/Core/Render/RenderCache.php
@@ -14,6 +14,10 @@
+ * @see sec_caching

I was suggesting to put this on the interface, looks fine to me otherwise.

Fixed #22's bottom remark, thanks to the +1 in #23.
Fixed #24.
Fixed #25.

RE: unit testing RenderCache (#25.c)): I think that these are inherently tightly coupled. I think our current test coverage is sufficient.

Another review.

1. createCacheID() is a protected method. But at #2450897-23: Cache Field views row output.1, it is said that part of the reason we're doing this issue, is to allow other code to call this method (to avoid duplication). So… do we still want to make that method public, or…?
2. Regardless of the original need for this issue (i.e. for the Views row caching issue), I think this is a sane improvement. It's two highly coupled classes, but that is fine. It makes both classes/parts more understandable, so I think it's an improvement in any case.

#27.2: LOL, why didn't I think of that? :)

all we need to do is […] check again on the docs for getCacheableMetadata in the Interface of the renderer and we should be done here.

You meant ::getCacheableRenderArray(), but yes, I'd agree that that is all that's left then.

1. The name suggests it should only live on RenderCache.
2. The usages are limited to: 1) the renderer doing render caching, 2) HtmlRenderer, with a very very exceptional edge case, 3) Views' CachePluginBase, which will go away as part of #2381277: Make Views use render caching and remove Views' own "output caching". So disruption of moving it to RenderCache would be very very minimal.

So I say: let's keep it only on RenderCache. Did that in this reroll. Up to you guys to judge now.

Dammit, both #26 and #28 contain an accidental change to UserData, causing those test failures.

Added beta evaluation. Updated the remaining tasks/UI changes/API changes sections too.

Forgot to update one bit.

1. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -0,0 +1,310 @@
   +  public function get(array $elements) {
   

   It would be great if we could provide also a ::getMultiple() method, this should allow to make things even faster in #2450897: Cache Field views row output.

2. +++ b/core/lib/Drupal/Core/Render/RenderCacheInterface.php
   @@ -0,0 +1,77 @@
   +   * @see ::get()
   +   */
   +  public function get(array $elements);
   

   Why is this pointing to itself?

3. +++ b/core/lib/Drupal/Core/Render/RenderCacheInterface.php
   @@ -0,0 +1,77 @@
   +   * @see ::set()
   +   */
   +  public function cacheSet(array &$elements, array $pre_bubbling_elements);
   

   I thought we were going to rename this to set, weren't we?

1. Out of scope. This makes it easier to add that, at least/at last :)
2. LOL, pre-existing problem, but fixed.
3. LOL! Fixed. Can't believe I simply missed that earlier :(

Sorry for those bad rerolls :(

Wim and Fabian are happy with this, so who am I to make them sad?

This is actually blocking a critical

I went ahead and applied #37 in #2450897-74: Cache Field views row output, but I have some troubles using the new API and make things work. I had to perform some adjustments. Please check whether I'm using it the intended way or I'm doing something wrong before setting this back to RTBC.

Committing this to unblock further work. We can still make any tweaks necessary if #2450897: Cache Field views row output finds it to be necessary. Committed 83c5370 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

diff --git a/core/lib/Drupal/Core/Render/Renderer.php b/core/lib/Drupal/Core/Render/Renderer.php
index e1113c9..ac1bd6e 100644
--- a/core/lib/Drupal/Core/Render/Renderer.php
+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -12,11 +12,9 @@
 use Drupal\Core\Cache\Cache;
 use Drupal\Core\Cache\CacheableDependencyInterface;
 use Drupal\Core\Cache\CacheableMetadata;
-use Drupal\Core\Cache\CacheContextsManager;
 use Drupal\Core\Controller\ControllerResolverInterface;
 use Drupal\Core\Theme\ThemeManagerInterface;
 use Drupal\Component\Utility\SafeMarkup;
-use Symfony\Component\HttpFoundation\RequestStack;
 
 /**
  * Turns a render array into a HTML string.

Fixed unused use statements on commit.

I would say this needs a change record : using the cache API directly will be the obvious choice for many.

@fgm: Except that only in the extremest of extreme edge cases, one would even be tempted to implement their own render caching. 99.9% of code should just use render caching, which already is thoroughly documented: https://www.drupal.org/developing/api/8/render/arrays/cacheability. Otherwise, I would agree.

Makes sense : I added a note about this on the Cache API intro at https://www.drupal.org/developing/api/8/cache so that devs looking at the Cache API can be made aware of this.

Good call! :) Tried to clarify it further.