Add Renderer::getCacheableRenderArray() to encapsulate which data is needed for caching a render array and have views use it

So #pre_render, #cache is just a pattern, but it might not work for you, so you want custom caching.

The bigger impact however is if views hardcodes this:

I might want to add downstream-ttl as a render array #cache property and I need it propagated upstream.

Therefore I can now exchange the renderer service, but if views has this hardcoded, I can't have this work with views caching.

And that is why this is a bug.

Postponing on #2378789: Views output cache is broken briefly ...

Code needs work to port views over to the new API.

+++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
@@ -235,27 +226,6 @@ public function postRender(&$output) { }
-    $this->view->element['#attached'] = drupal_merge_attached($this->view->element['#attached'], $this->storage['attachments']);
-    $this->view->element['#cache']['tags'] = Cache::mergeTags(isset($this->view->element['#cache']['tags']) ? $this->view->element['#cache']['tags'] : [], $this->storage['cacheTags']);
-    $this->view->element['#post_render_cache'] = NestedArray::mergeDeep(isset($this->view->element['#post_render_cache']) ? $this->view->element['#post_render_cache'] : [], $this->storage['postRenderCache']);

We are losing something here, we need to make sure this is ok. With this patch we will no longer be merging with any other items in $view->element.

I removed the 'output' key on the storage property. If we are not using other keys, I don't think we need to use that either now. Let's just simplify as much as we can?

We should also probably inject the renderer, and not use drupal_render directly anymore.

Just question in #7 I think.

Whoops.

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -387,4 +398,84 @@ protected function processPostRenderCache(array &$elements) {
   +    if (!empty($cid) && $cache = \Drupal::cache($bin)->get($cid)) {
   ...
   +    \Drupal::cache($bin)->set($cid, $data, $expire, $data['#cache']['tags']);
   

   Should we inject the cache services? From an abstract point of view it would be cool to be able to render something without a cache dependency, so maybe make it optional?

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -387,4 +398,84 @@ protected function processPostRenderCache(array &$elements) {
   +    if (!\Drupal::request()->isMethodSafe() || !$cid = drupal_render_cid_create($elements)) {
   

   At least this request() could be replaced with the stack again.

3. +++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
   @@ -143,8 +176,8 @@ public function cacheSet($type) {
   -        $this->storage['output'] = drupal_render($this->view->display_handler->output);
   -        $this->gatherRenderMetadata($this->view->display_handler->output);
   +        $markup = $this->renderer->render($this->view->display_handler->output);
   
   @@ -178,16 +211,7 @@ public function cacheGet($type) {
   -            $this->restoreRenderMetadata();
   -            $this->view->display_handler->output = array(
   -              '#attached' => &$this->view->element['#attached'],
   -              '#cache' => [
   -                'tags' => &$this->view->element['#cache']['tags'],
   -              ],
   -              '#post_render_cache' => &$this->view->element['#post_render_cache'],
   -              '#markup' => $cache->data['output'],
   -            );
   +            $this->view->display_handler->output = $this->storage;
   

   Tis is really nice code now!

The patch looks already really good!

getCacheableRenderArray is great!

cacheSet / cacheGet is probably sufficient for the protected methods.

+++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
@@ -235,27 +259,6 @@ public function postRender(&$output) { }
-    $this->view->element['#attached'] = drupal_merge_attached($this->view->element['#attached'], $this->storage['attachments']);
-    $this->view->element['#cache']['tags'] = Cache::mergeTags(isset($this->view->element['#cache']['tags']) ? $this->view->element['#cache']['tags'] : [], $this->storage['cacheTags']);
-    $this->view->element['#post_render_cache'] = NestedArray::mergeDeep(isset($this->view->element['#post_render_cache']) ? $this->view->element['#post_render_cache'] : [], $this->storage['postRenderCache']);

I would _love_ to RTBC this, but the question remains if there can be something already in $this->view->element[metadata-key] that is not cached.

In D7 RenderCache I circumvented this in two ways:

a) Either adding the whole element as x_render_cache_storage (and removing any #markup other children by using again the getCacheableRenderArray function).
b) Rendering whatever was in there early on the stack.

Needs feedback by Views maintainer I assume to see if there can be something in $this->view->element that was not cached before.

The only thing that's been removed is the comment about ESI, where it's stated that cache backends can replace the actual content with ESI includes. That's been a lie for a very long time, because the cache backend receives the array by value, not by reference, as is dictated by CacheBackendInterface. In D7, cache_set() also worked by value, but in D7, it'd have been possible to replace cache.inc and change the signature, in which case the documented ESI approach would've worked indeed.

I don't think this was ever used by anything, so i think this is fine

Thanks for finding further usages of this and thanks for checking that the "merge" before was no merge, because there was never any content.

RTBC :)

1. +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
   @@ -269,4 +269,23 @@ public function renderPlain(&$elements);
   +   * Gets a cacheable render array for a render array and its rendered output.
   +   *
   +   * Given a render array and its rendered output (HTML string), return an array
   +   * data structure that allows the render array and its associated metadata to
   +   * be cached reliably.
   +   *
   +   * 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.
   +   *
   

   Just in case you want to, you could document that this result can be safely serialized()/unseralized(), not sure whether this is implicit already with "cached"?

2. +++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
   @@ -143,8 +176,8 @@ public function cacheSet($type) {
   +        $this->renderer->render($this->view->display_handler->output);
   +        $this->storage = $this->renderer->getCacheableRenderArray($this->view->display_handler->output);
   

   <3

1. To evaluate whether we should make this change during the beta, we need to update the issue summary according to https://www.drupal.org/core/beta-changes to explain what improvements it makes and what the disruption of the change would be. Can someone add Drupal 8 beta phase evaluation template to the issue summary.
2. Do we need a D8 to D8 change record since we are removing drupal_render_cache_set and drupal_render_cache_get? These are the only runtime usages of drupal_render_cid_create() so perhaps we ought to be moving that too for consistency? And then there is drupal_render_cache_generate_placeholder() - there is no functional usage of this either but that is used in move places - drupal_render_cache_generate_placeholder () looks like followup material to me.

1. +++ b/core/includes/common.inc
   @@ -2369,79 +2369,6 @@ function show(&$element) {
   -  // Create the cache ID for the element.
   -  if (!\Drupal::request()->isMethodSafe() || !$cid = drupal_render_cid_create($elements)) {
   -    return FALSE;
   -  }
   
   +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -437,4 +459,102 @@ protected function processPostRenderCache(array &$elements) {
   +    if (!$this->requestStack->getCurrentRequest()->isMethodSafe() || !$cid = $this->createCacheID($elements)) {
   +      return FALSE;
   +    }
   ...
   +    // Create the cache ID for the element.
   +    if (!$this->requestStack->getCurrentRequest()->isMethodSafe() || !$cid = $this->createCacheID($elements)) {
   +      return FALSE;
   +    }
   

   I know this existed before, but can we document why we don't want to deal with it in case we use https? It feels like quite an detail which is not always necessarily right.

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -437,4 +459,102 @@ protected function processPostRenderCache(array &$elements) {
   +    if (!empty($cid) && $cache = \Drupal::cache($bin)->get($cid)) {
   ...
   +    \Drupal::cache($bin)->set($cid, $data, $expire, $data['#cache']['tags']);
   

   I can haz the cache factory injected?

3. +++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
   @@ -73,6 +74,38 @@
   +    return new static($configuration, $plugin_id, $plugin_definition, $container->get('renderer'));
   

   it would be nice to newline this.

4. +++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
   @@ -143,8 +176,8 @@ public function cacheSet($type) {
          case 'output':
   -        $this->storage['output'] = drupal_render($this->view->display_handler->output);
   -        $this->gatherRenderMetadata($this->view->display_handler->output);
   +        $this->renderer->render($this->view->display_handler->output);
   +        $this->storage = $this->renderer->getCacheableRenderArray($this->view->display_handler->output);
            \Drupal::cache($this->outputBin)->set($this->generateOutputKey(), $this->storage, $this->cacheSetExpire($type), $this->getCacheTags());
   

   Meh, this conflicts with #2378815: DisplayPluginBase::elementPreRender() and View::preRenderViewElement() are called even when the Views output cache is being used, causing notice

5. +++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
   @@ -180,16 +213,7 @@ public function cacheGet($type) {
   -            $this->view->display_handler->output = array(
   -              '#attached' => &$this->view->element['#attached'],
   ...
   -              '#post_render_cache' => &$this->view->element['#post_render_cache'],
   ...
   +            $this->view->display_handler->output = $this->storage;
    
   

   Do we think it is okay to no longer reference the view element entries?

Alright, looks sane now.

Committed 62c31b5 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.