Views output cache is broken

I discovered this while working on #2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests. That issue only cares about the assets being handled correctly, but we might as well solve all bubbleable render metadata in one go.

Overall, this points out how badly we want to get rid of Views' output cache in favor of standard render caching.

This is wrong, because Views' output cache does more than what render caching does. E.g. it allows the results to not be cached but to still use the output cache.

Hence removed that from the IS.

Opened an unrelated issue in the same area: #2378815: DisplayPluginBase::elementPreRender() and View::preRenderViewElement() are called even when the Views output cache is being used, causing notice.

#8: only because I don't know what other things want to be stored in there. I don't want to impose render array thinking on Views. Hence I stuck to the original pattern.

catch suggested it by marking #2183017: Views doesn't bubble up rendered entities' cache tags as a related issue, on which #2350551: Views fields that have attached assets are lost when Views output caching is enabled was quite recently marked as a related issue… which sparked an IRC discussion with Fabianx, dawehner, damiankloip and I about this. We thought that #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render might have fixed the bubbling of cache tags of rendered entities (or rendered anything, really). So I set out to investigate…

After thoroughly reading through #2183017, reading the Views render pipeline code, stepping through it with a debugger and verifying it through testing, I'm now absolutely certain that #2273277 fixed it!

It's even easy to convince yourself of this as well. Just install a fresh Drupal 8, create an article node that's promoted to the front page, and load the front page, you should see the node:1 cache tag (amongst others, such as the associated taxonomy term's cache tag, the node owner's cache tag, etc.) listed in the X-Drupal-Cache-Tags header.

If there's any lingering doubt, give it a try on simplytest.me, with the last commit before #2273277 and then the commit that landed #2273277:

• Before #2273277: http://simplytest.me/project/drupal/dcc880bc1081e86b251d833ba6b7c7864483... — cache tags don't bubble… :(
• With #2273277: http://simplytest.me/project/drupal/8c68491896ce0a4a52dce9f8de94d23623c7... — cache tags bubble! :)

BUT! But, the above only verifies that it's working for uncached Views. Since Views has "output caching", which is its own form of render caching, that also needs to support cache tags. So far that wasn't the case. Hence it's not working for cached Views. This issue fixes that too! (Simply by supporting the bubbling of all render metadata, just like drupal_render() bubbles it.)

Conclusion:

1. Views already bubbles up rendered entities' cache tags when rendering an uncached view
2. Views does not yet do that for cached views, but with this patch, it does

Therefore, once this issue lands, #2183017: Views doesn't bubble up rendered entities' cache tags is fully fixed, and #2350551: Views fields that have attached assets are lost when Views output caching is enabled already is!

Views should not need to know about the internal of the render array structure and it is also setting a bad example doing so.

I agree in principle, but please look at what it used to do (i.e. does in HEAD):

• it calls _drupal_add_html_head()
• it copies whatever is in $render_array['#attached']['css']
• it copies whatever is in $render_array['#attached']['js']

So… it already knows the internals of render arrays. We traded 3 details (head, CSS, JS) for 3 others (attached, cache tags, post render cache), but this time 3 that cover everything. My point is: what you aim for is worthy, but is IMO definitely out of scope.

In the long run, I think that we should somehow get rid of Views' output cache, in favor of regular render caching. That'd address your concerns, remove this brittleness, remove duplication and actually even fix Views' output caching. Because Views currently does this:

  /**
   * Calculates and sets a cache ID used for the output cache.
   *
   * @return string
   *   The generated cache ID.
   */
  public function generateOutputKey() {
    if (!isset($this->outputKey)) {
      $user = \Drupal::currentUser();
      $key_data = array(
        'result' => $this->view->result,
        'roles' => $user->getRoles(),
        'super-user' => $user->id() == 1, // special caching for super user.
        'theme' => \Drupal::theme()->getActiveTheme()->getName(),
        'langcode' => \Drupal::languageManager()->getCurrentLanguage()->getId(),
        'base_url' => $GLOBALS['base_url'],
      );

      $this->outputKey = $this->view->storage->id() . ':' . $this->displayHandler->display['id'] . ':output:' . hash('sha256', serialize($key_data));
    }

    return $this->outputKey;
  }

… i.e. cache contexts avant-la-lettre. This is inherently wrong, because it:

1. doesn't support all cache contexts that Drupal core supports (per timezone, per user, per URL are not supported — and contrib can add many more)
2. always uses the same cache contexts, and hence may use too many cache contexts, hence resulting in too many cache items and cache underutilization
3. always uses the same cache contexts, and hence may use too few cache contexts, hence resulting in the same cache item being served to two users that need two different pieces of output

This should inherit the cache contexts of the things it has rendered: those of the View and the things contained within the View. When converting Views to use regular render caching, we'll have to do that.

Discussed #8 & #12 more with Fabianx & damiankloip in IRC; they agree we should do that work in a follow-up. I've created that follow-up: #2379741: Add Renderer::getCacheableRenderArray() to encapsulate which data is needed for caching a render array and have views use it.