Views fields that have attached assets are lost when Views output caching is enabled

Afaik the problem is that Render\Contextual does not add the library.

@effulgentsia figured out whether the problem is: We do have a drupal_render() call in theme().

This is generally a bigger problem, we need to change how fields are rendered in views.

Let's rename it a bit.

Yep. Pretty much what's happening there :)

This is limited to Views' rendering of fields no?

This is limited to Views' rendering of fields no?

Yes it is.

I think we could/should still use this to actually work on field rendering, as mentioned above somewhere.

This is currently blocked by #2342287: Allow Twig in Views token replacement

#11: Yep, that is the CR for #2361681: drupal_render(): invert second argument ($is_recursive_call -> $is_root_call) => more strict, better DX/TX, which should indeed fix all bubbling of render metadata (which includes assets). But I'm not 100% certain yet that truly everything is fixed because Views rendering is so incredibly complex. Looking into this now…

Hurray! We actually already fixed this… by accident! :)

From #2378789-10: Views output cache is broken:

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!

No, I don't think we did.... :(

This will work fine for un cached views, but cached views will not keep the correct attachments. Hence comment in #12, we need fields to return render arrays and keep them as that I think.

Not field rendering but more views cache tags in general: Another problem that I see is that the entity list and views cache tags are only added to the internal views result/query (unless that was changed recently) but not the render array, so they are not bubbling up either.

I tried to enable views caching (I currently have caching enabled on the blocks and not the views) but explodes on serialization issues, as it is serializing the results with the entities, which have a reference to the view, which has all kinds of things...

This will work fine for un cached views, but cached views will not keep the correct attachments. Hence comment in #12, we need fields to return render arrays and keep them as that I think.

Can you:

1. give me steps to reproduce a view in which this occurs?
2. explain me why #2378789: Views output cache is broken would not have solved this for every view?

Another problem that I see is that the entity list and views cache tags are only added to the internal views result/query (unless that was changed recently) but not the render array, so they are not bubbling up either.

Correct; those are the cache tags associated with the View itself, and more specifically: with its query. But that's out of scope here. I'd think that the Tag-based cache plugin for Views already does this, but you're right that it doesn't. We don't have an issue for that yet.

explodes on serialization issues

What kind of view is this?

So my steps to reproduce this are:

- Enable tag based caching on the admin/people view
- Go to admin/people, the first time you should see this:

- Refresh the view again (this time will be retrieved from cache):

Oh, and of course, make sure that your render cache bin is not set to the NullBackend! :)

Assigning to Wim ...

(I was working on this last week, but then got pulled into other stuff. Posting what I had written so far.)

Reproduced #19.

As we'd expect, the problem is that the asset library for dropbutton is not in the Views output cache item, and hence it doesn't get restored on an output cache hit. The question is why?.

@Wim Leers posting the patch will help :)

Did the necessary debugging. The problem lies indeed in the way fields in field-based Views (table views, grid views …) are rendered.

ViewExecutable::render() does "split rendering"

The first trickiness is in DisplayPluginBase::render(). (Note that it is immediately after invoking this function in ViewExecutable::render() that we store its output in the output cache.) In there, if the result set is not empty, we render the style plugin, which returns $rows. And then we return a render array which has '#rows' => $rows but also #attached. It is that #attached that we used to determine which attachments to store in the output cache. But … it's incomplete: we should also have been using $e['#rows']['#attached'], i.e. at the return value of StylePluginBase::render(). Because the current code basically ignores any attachments that the style plugin adds.
But that turns out to be more difficult than it seems…

FieldPluginBase::theme() loses bubbleable rendering metadata

StylePluginBase::render() calls StylePluginBase::renderGrouping(), which calls StylePluginBase::renderFields(). That function renders fields into $this->rendered_fields… which means they're not ending up inside the "main render array" for the view, that they're assembled back in there at a later time.
And the real problem is that StylePluginBase::renderFields() calls FieldPluginBase::theme(). Inside that function, a render array is generated, but only the output of the rendered render array is kept, i.e. we keep the resulting markup, but we get rid of the render array. Hence the bubbleable rendering metadata (#attached, #cache and #post_render_cache) is lost.

Solution

The ideal way to fix this is by "fixing" the Views render pipeline. I say "fixing" between quotes, because it was never really broken. Its only fault is being architected in a time where there was no such thing as bubbleable rendering metadata yet: there were only global statics. We chose to go for bubbleable rendering metadata a long time ago, for many reasons, but we never updated Views accordingly. (Because, let's be honest, that's a very daunting task.) At some point, we should bite the bullet and do this. But a "remove all warts from Views" initiative in the beta phase of Drupal 8 seems like bad timing at minimum.

Therefore, the best solution I can think of, is actually to just leverage an existing wart: $this->view->element. This is already being used to contain the attachments of "the rendered view", while it is being rendered. Simply making FieldPluginBase::theme() update it is all that's necessary!

Discussed this with dawehner in IRC. He asked for an abstraction, so that Views doesn't need to know about those Render API internals (i.e. similarly to #2379741: Add Renderer::getCacheableRenderArray() to encapsulate which data is needed for caching a render array and have views use it). So, did exactly that in this reroll. Took quite a bit of time to figure out the right abstraction, but I think you'll like this. Because it also allows us to simplify code in several other places :)

(Note to self: patch generated with -M10%.)

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -372,12 +371,12 @@ protected function resetStack() {
   -    $frame = self::$stack->top();
   +    $frame = self::$stack->pop();
   

   oh?

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -372,12 +371,12 @@ protected function resetStack() {
   +    self::$stack->push($updated_frame);
   

   Can we open a follow up to use static:: everywhere?

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -398,9 +397,7 @@ protected function bubbleStack() {
   -    $current->tags = Cache::mergeTags($current->tags, $parent->tags);
   -    $current->attached = drupal_merge_attached($current->attached, $parent->attached);
   -    $current->postRenderCache = NestedArray::mergeDeep($current->postRenderCache, $parent->postRenderCache);
   +    $current = $current->merge($parent);
   

   This is really kinda of cleaner, if you ask me.

4. +++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
   @@ -252,9 +253,12 @@ protected function gatherRenderMetadata(array $render_array = []) {
   +    $this->view->element = Renderer::mergeBubbleableMetadata($this->view->element, $storage);
   
   +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1591,7 +1593,14 @@ function theme(ResultRow $values) {
   +    $this->view->element = $this->getRenderer()->mergeBubbleableMetadata($this->view->element, $build);
   

   Don't we partially want to consistent use the static method or not?

5. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -441,6 +441,16 @@ public function __construct(ViewStorageInterface $storage, AccountInterface $use
   +    // Initialize the bubbleable rendering metadata, to facilitate merging of
   +    // bubbleable rendering metadata for Views plugins that need to render parts
   +    // of a view, but need to return output as a string instead of a render
   +    // array. This allows them to continue to return output as a string, but
   +    // also allows them to correctly bubble rendering metadata.
   +    // @see \Drupal\views\Plugin\views\field\FieldPluginBase::theme()
   +    $this->element['#cache']['tags'] = [];
   +    $this->element['#attached'] = [];
   +    $this->element['#post_render_cache'] = [];
   

   DO we still have to do that? Can't we just hide that internally?

(Unpublished #27, which was a duplicate of #26.)

#26

1. Before, we modified $frame. Now, with the new helper functions, we generate $frame. So instead of getting a reference to the top item in the stack, we pop it, and then push the new one.
2. Done: #2404831: self::$stack -> static::$stack in Renderer.
3. I agree :)
4. Oops! Leftover from an earlier iteration. Fixed.
5. Yes, great point! Done.

Also fixed the exceptions.

Better title.

Critical because this blocks #2381217: Views should set cache tags on its render arrays, and bubble the output's cache tags to the cache items written to the Views output cache (which was triaged critical).

dawehner, damiankloip: how would you recommend writing generic test coverage for this? Can you point to tests I can look at as an example?

Added unit tests for the new logic in BubbleableMetadata (formerly RenderStackFrame).

dawehner, damiankloip: I can't add a regression test before you answer #31. I tried to do it myself, but failed :( Writing tests is easy, creating a test view is not :(

I like the general approach.

1. +++ b/core/lib/Drupal/Core/Render/BubbleableMetadata.php
   @@ -0,0 +1,93 @@
   +   * @var array
   ...
   +   * @var array
   +   */
   ...
   +   *
   +   * @var array
   

   Can we use string[] here?

2. +++ b/core/lib/Drupal/Core/Render/BubbleableMetadata.php
   @@ -0,0 +1,93 @@
   +   *
   +   * @codeCoverageIgnore
   +   *
   

   I would drop this here but just keep the todo, this would be more honest. This skip is afaik used for pointless methods.

3. +++ b/core/lib/Drupal/Core/Render/BubbleableMetadata.php
   @@ -0,0 +1,93 @@
   +   * @todo Add unit test for this in
   +   *       \Drupal\Tests\Core\Render\BubbleableMetadataTest when
   +   *       drupal_merge_attached() no longer is a procedural function and remove
   +   *       the '@codeCoverageIgnore' annotation.
   +   */
   

   Note: The issue to unit test drupal_render adds this.

4. +++ b/core/lib/Drupal/Core/Render/BubbleableMetadata.php
   @@ -0,0 +1,93 @@
   +   *   A render array.
   +   *
   +   * @return \Drupal\Core\Render\BubbleableMetadata
   +   */
   

   Can we use @return static ?

5. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -372,12 +371,12 @@ protected function resetStack() {
   +    self::$stack->push($updated_frame);
   

   nope, don't try to readd a self:: call

1. Only for ::tags. The other two are nested arrays.
2. Ok, fine by me! :)
3. Yay!
4. Sure, done.
5. Bad rebase :( Wim--; thank you for spotting this!

Well, then use string[][]

I didn't know that was valid syntax. Cool :) Done.

But that's not really correct for #post_render_cache: for that key, you're not required to use strings at the second level; the context kan include integers or even objects. Using array[] for that instead then.

Overall I think this is looking like a pretty good idea.

1. +++ b/core/lib/Drupal/Core/Render/BubbleableMetadata.php
   @@ -0,0 +1,91 @@
   + * Value object used for bubbleable rendering metadata.
   

   Is this still just a 'value object' when it merges data and applies this to other build arrays? Just asking.

2. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1590,7 +1593,14 @@ function theme(ResultRow $values) {
   +    $output = $this->getRenderer()->render($build);
   ...
   +    $this->view->element = Renderer::mergeBubbleableMetadata($this->view->element, $build);
   

   Seems weird using the service (abstracted, by 'renderer' definition) then the static hardcoded Renderer. This could at least use $renderer->mergeBubbleableMetadata()?

   Also, does that method need to be static? is there another good reason it is? Makes it seem just hacked into the renderer class.

3. +++ b/core/tests/Drupal/Tests/Core/Render/BubbleableMetadataTest.php
   @@ -0,0 +1,126 @@
   +   * Tests the apply() method.
   ...
   +   * Tests the createFromRenderArray() method.
   

   Do we need to bother with these still in unit tests? @covers says the same really? Not sure what the directive is on that at the moment.

4. +++ b/core/tests/Drupal/Tests/Core/Render/BubbleableMetadataTest.php
   @@ -0,0 +1,126 @@
   +   * Provides tests data for apply().
   

   'test data'

1. Good question :) According to https://en.wikipedia.org/wiki/Value_object and http://martinfowler.com/bliki/ValueObject.html, immutability is generally a requirement. That's not the case here. OTOH… seems like most of Drupal's value objects do this: \Drupal\Core\Url, \Drupal\Core\Link, \Drupal\Core\Access\AccessResult …
   Hence I'm not sure it's worth changing here. I could s/Value object used for…/Object used for…/, if that helps? :)
2. Indeed, it could. Fixed.
3. Totally agreed; removed the comments.
4. Hah! Copy/pasted that typo from ElementInfoManagerTest; fixing it there is probably out of scope here, so leaving the typo there.

1. +++ b/core/lib/Drupal/Core/Render/BubbleableMetadata.php
   @@ -0,0 +1,91 @@
   + *
   + * @see drupal_render()
   + */
   

   Would be nice if we point to \Drupal\Core\Render\RendererInterface::render

2. +++ b/core/lib/Drupal/Core/Render/BubbleableMetadata.php
   @@ -0,0 +1,91 @@
   +  public function apply(array $build) {
   

   From the pure name, its not obvious for me, which way round it is, can we rename it maybe to something like BubbleMetadata::applyTo($build) ?

3. +++ b/core/modules/filter/src/Element/ProcessedText.php
   @@ -107,44 +109,20 @@ public static function preRenderText($element) {
   +    $element = $metadata->apply($element);
   

   From the name I would have also expected that $element is massaged by reference.

4. +++ b/core/modules/views/src/Tests/Plugin/CacheTest.php
   @@ -149,8 +149,8 @@ function testHeaderStorage() {
   -    $this->assertTrue(['views_test_data:1'], $output['#cache']['tags']);
   -    $this->assertTrue(['views_test_data_post_render_cache' => [['foo' => 'bar']]], $output['#post_render_cache']);
   +    $this->assertEqual(['views_test_data:1'], $output['#cache']['tags']);
   +    $this->assertEqual(['views_test_data_post_render_cache' => [['foo' => 'bar']]], $output['#post_render_cache']);
   

   Good catch!

First, a straight reroll. #2379741: Add Renderer::getCacheableRenderArray() to encapsulate which data is needed for caching a render array and have views use it broke this.

(And generated with -M10% again, to make it easier to review.)

1. Done.
2. Works for me; done!
3. Hah! I went back and forth on this; I'm fine with either. Done.
4. :)

Thank you!

Added a beta evaluation

Alex Pott remarked that ::merge() could be changed to return a new object, hence making the object immutable. Let's do that.

Done.

Alright, seems fair.

+1, that does seem fair.

Let's go one step further and make this thing actually immutable.

Fixing the fail.

Good fix.

Thanks, those changes look great! Just one nitpick, that can be fixed upon commit:

+++ b/core/lib/Drupal/Core/Render/BubbleableMetadata.php
@@ -0,0 +1,106 @@
+   * @param array $tags

s/array/string[]/

Applied #53

Manually diffed #54 with #51, the interdiff is only what's in #53, so keeping at RTBC :)

Given that my changes to this patch only concern internal implementation details of the value object I think it is okay for me to commit this. Committed 3d556b7 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

+++ b/core/tests/Drupal/Tests/Core/Render/BubbleableMetadataTest.php
@@ -0,0 +1,117 @@
+/**
+ * @coversDefaultClass \Drupal\Core\Render\BubbleableMetadata
+ * @group Render
+ */
+class BubbleableMetadataTest extends UnitTestCase {
+
+  /**
+   * @covers ::apply
+   * @dataProvider providerTestApply
+   */
+  public function testApply(BubbleableMetadata $metadata, array $render_array, array $expected) {

There's no such method as BubbleableMetadata::apply(), so the @cover annotation is incorrect and I can't make a coverage report.

Thanks @Mile23!

Can we file a separate followup for that fix? Generally we don't reopen criticals unless the need a revert. Thanks!

Ba da bing. #2413941: BubbleableMetadataTest::testApply has wrong @covers