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

Clarifying the issue title.

#2372855: Add content & config entity dependencies to views is about adding the config/content entity dependencies to views config entities.

This issue is about also applying those dependencies to render arrays, and hence setting the associated cache tags.

#2372855: Add content & config entity dependencies to views landed, this is now only blocked on #2342651: Cache tags for *all* config entities.

I think you're right. I think I might've blocked this issue on that one because right now we don't have guarantees that e.g. NodeType config entities are invalidated when necessary. But as you say, this is in fact part of that issue.

From the IS:

E.g. if a view filters to article nodes, it should depend on the article node type config entity, and associate its cache tag, so that if the article node type is changed, that the render cached view is also invalidated.

The it should depend on the article node type config entity part is already happening, that was done in #2372855: Add content & config entity dependencies to views.

It's the and associate its cache tag part that's still missing. Now investigating.

Again taking this as an example:

E.g. if a view filters to article nodes, it should depend on the article node type config entity, and associate its cache tag, so that if the article node type is changed, that the render cached view is also invalidated.

Having worked on #2372855: Add content & config entity dependencies to views, I'm no longer sure that this is actually true. At least, I'm not sure anymore about this particular example. I think the quoted statement (from the IS) is wrong in multiple ways:

1. if a view filters to article nodes, then there will be a dependency on the bundle entity type config entity (node.type.article in this example), not on the associated view mode (see \Drupal\views\Plugin\views\filter\Bundle::calculateDependencies())
2. if a view is configured to render a list of entities in a certain view mode, then there will be a dependency on the view mode config entity (core.entity_view_mode.node.teaser in this example). Entity View Mode config entities represent the different view modes that are available for a certain entity type. But these are NOT the same as the Entity View Display config entities, which represent the configuration of a given view mode for a certain bundle. (I learned this the hard way in that other issue.) Given that a View is per-entity type, is configured at the entity type level, and can at best filter to a certain bundle, it is only natural that a view only depends on an Entity View Mode, and not on an Entity View Display.
   A direct consequence is that a view cannot actually associate the cache tag of the Entity View Displays that are used by the entities it renders; only the rendered entities themselves can associate those cache tags. But this is actually already what the view builder cache tags are being used for, which already are associated with the rendered entities; we should look into removing those in favor of Entity View Display config entities' cache tags).

Hence, doing what the IS said is impossible and to some extent even nonsensical.

I also always thought that all config/content dependencies for a view should also have the corresponding cache tags set on the rendered view. I no longer think that's actually the case. For example, a Views Page display can optionally have a menu link associated with it. When a certain view uses that, a content dependency on the associated menu_link_content dependency appears. But does it make sense to associate a menu link's cache tag with a View? No, not at all, because a rendered view neither contains the menu link in question, nor is a rendered view going to change if that menu link changes. It's a dependency, but it's completely irrelevant to the rendered view.

Conclusion

I think the goal of this issue — Views should set cache tags on its render arrays for the content and config entities it depends upon — is wrong. I might've missed something though, so please prove me wrong.

However…

…the spirit of this issue really is "make sure View config entities' cache tags are bubbled up" (including test coverage). I tested this, and it's very, very broken — but then again, Views only received cache tag support in a plugin, it was never designed or updated to support cache tags from the ground up.

So I propose to rename this issue to just Views should set cache tags on its render arrays and fix the current brokenness:

1. The view:<view ID> and <entity type>:list cache tags only exist on Views output cache items
2. That means those cache tags don't ever appear on rendered Views (e.g. on the frontpage, there is no view:frontpage cache tag), and they are only ever set if Views' caching is enabled.
3. Views output cache items don't contain the cache tags of the rendered output they contain, which means that if e.g. node 5 is listed in the view and hence is stored in the output cache, and node 5 contains term 3, that it will not be invalidated when term 3 is modified, hence the view will continue to show the old term 3 inside node 5.

This should cause a fair number of test failures.

And since this blocks #2342651: Cache tags for *all* config entities, which was triaged critical, marking this as critical also.

we could make that a method of maybe the view executable and reuse it in the result cache

Absolutely. This is hacky, proof-of-concept code.

Note: This kind of information seems all be possible to determine on view safe time, so should we move this all onto $this->storage->getCacheTags ?

This is interesting. But I think it may be semantically wrong… because e.g. if a view's result cache should be invalidated whenever the list of nodes is changed, that only is relevant for the *result cache* (well, also the output cache), but not for the *view itself*. Consequently, if View::getCacheTags() would return the list cache tags of the entity types that view lists, then every node listing would be invalidated when that view was changed. So if a rarely used node view is modified, all node views' caches would be invalidated. I don't think we want that.
But what we could do, is still determine this at save time, but instead expose it in a getExecutedViewCacheTags() method (whatever the name would be).

That being said, I'd rather focus on fixing cache tags in this issue rather than also making determining the right cache entity list tags faster.

This reminds me of #2318377: Determine whether a view is cacheable and its required contexts, store this i/t config entity, where we did all of the work to save the required cache contexts to the config entity, but then aren't actually using that information yet! (And we didn't add tests :() But that's out of scope for this issue; this issue only deals with cache tags.

I figured I'd actually try to implement your suggestions already. I encountered a few things:

• One problem with moving it into a method right now (i.e. without calculating the entity list cache tags at save time) is that it'd have to run after the query has been built. That'd be fine if it's in a protected method, but if we want the results cache to reuse the method on ViewExecutable, then it of course must be public.
• The results cache apparently also doesn't yet take the cache tags of the entities in the result into account. Fixed that too. (To reuse the example from #7: we only need to care about node 5's cache tag here, not about term 3. The result cache only *references* term 3, it doesn't *contain* term 3. Hence if term 3 is modified, the result cache item is still valid. Only when node 5 is modified, the result cache item needs to be invalidated.

Oh, sorry, I missed #11!

- EntityViewDisplay::postSave() clears the render cache for the whole entity type when the EVD for any bundle + view mode gets updated (a bit harsh, but doesn't happen that often, and also, not sure if there's any way we can be more granular ?)
That should take care of views that display fully rendered entities ?

Correct. Note that #7 was trying to talk about the abstract problem, but used concrete examples (and the one you give in particular because that's the key example listed in the IS).

- Views that display individual fields (like admin/content) are currently out of the regular "entity view" flow completely, so I guess they skip any render cache as well ?

Indeed, but with some nuance! The fields are render cacheable. If they use field formatters — which they do AFAIK — they also have the appropriate cache tags. But they do *not* use render caching individually, as in on a per-field basis, because entities are already render cacheable. It's up to Views to do render caching. And Views actually does that, with its "output cache".

#1867518: Leverage entityDisplay to provide fast rendering for fields has a plan to move "render a row = render a (bunch of) entity(es) and cherry-pick the rendered fields from the resulting render array", but I guess that wouldn't help the render cache stores rendered HTML, and we can't cherry-pick in there, right ?

Ohhhhh! It wouldn't help this issue AFAICT, but I think that'd be very worthwhile to do that.

Should be green now. But the fact that we have so few failures shows how little test coverage we have here.

#18:

Furthermore, if there's a certain kind of config dependency where the contents do matter, then that should get reflected by the appropriate plugin somewhere in the render array that the plugin generates, and the plugin should set the corresponding cache tags there, and those will bubble up. So because of that bubbling, there's no need to automatically add them for every dependency.

Exactly!

However, I wonder what should happen when a dependent config gets deleted. e.g., a View is configured to filter on a certain node type, and then someone deletes that node type. Does the CMI dependency management system prevent the deletion entirely […]

AFAIK: yes. But, as Berdir says, it's only used for uninstall.

#19:

I'd love to integrate that into a standard config entity delete form as well.

Yes!

Discussed with damiankloip and Berdir; we decided to keep this issue focused on the output cache only, not the results cache. Will need to reroll to remove the changes in that area.

As per #21: removed the changes to Views' results cache logic.

IS updated since we have consensus about the new scope.

Here's test coverage.

Two things I dislike about the test coverage:

• I'd like to write generic tests, but I'm not sure where I should do that. So for now I have FrontPageTest with test coverage for entity views, and GlossaryTest for field views.
• Field views don't actually set the right cache tags yet, this might be related to #2350551: Views fields that have attached assets are lost when Views output caching is enabled.

Would be great to get some feedback. I'll be AFK the next 2 weeks, so feel free to push this forward in the mean time.

Ignore #24, this is the right patch.

Updated IS to indicate the remaining tasks.

Done.

With #2350551: Views fields that have attached assets are lost when Views output caching is enabled committed, I can now continue here. Expect an update later today or tomorrow.

First, a straight reroll of #32. #2040135: Caches dependent on simple config are only invalidated on form submissions conflicted with this in several places.

This is going to fail tests.

Now with tests passing.

Next: address #33 + #34. Then: fix the @todo mentioned in #24 & #35.

This is blocked on me addressing the feedback, not blocked on review.

#43

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2050,6 +2050,10 @@ public function render() {
+    // If the output is a render array, add cache tags, regardless of whether
+    // caching is enabled or not; cache tags must always be set.
+    $element['#cache']['tags'] = $this->view->getCacheTags();

Wrong, because:

1. overwrites existing cache tags — just like Fabianx said in #54. This was already fixed in #55.
2. misses the entity type listing cache tags; only sets the View's own cache tags; it is essential that all relevant cache tags are always present, regardless of which cache plugin Views itself uses. Otherwise

#55

+++ b/core/modules/views/src/Plugin/views/query/Sql.php
@@ -1537,6 +1538,20 @@ function loadEntities(&$results) {
+  public function getCacheTags() {
+    $tags = [];
+    // Add cache tags for each row, if there is an entity associated with it.
+    if (!$this->hasAggregate) {
+      foreach ($this->view->result as $row)  {
+        if ($row->_entity) {
+          $tags = Cache::mergeTags($row->_entity->getCacheTags(), $tags);
+        }
+      }
+    }
+
+    return $tags;
+  }

This is necessary for field views, right?

#2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency should ensure every field has the correct cache tags. (Because it reuses the same field formatters as we use for "regular" entity rendering.)

This, in turn, ensures that the cache tag of the entity owning that field is also associated. That makes sense.

Finally, I want to call out that #1867518: Leverage entityDisplay to provide fast rendering for fields is closely related to all this too (it's already marked as a related issue of #2342045).

#56
b) assertPageCacheTags() always does 2 requests, the first must be a miss, the second must be a hit, and both must have the expected cache tags. Updating its docs.
c) Indeed, @dawehner fixed that.

Finally, having tested this patch, this part of the issue title was (IIRC) fixed in my patch, but now is broken again: bubble the output's cache tags to the cache items written to the Views output cache.

STR:

1. Create an article with a taxonomy term.
2. Enable tag-based caching for the frontpage view.
3. Load the frontpage.
4. Look at the render cache bin, find the CID that looks like frontpage:page_1:output:<hash>. Look at its cache tags.

Expected: the term's cache tag, the author's cache tag, the text format's cache tag are all present.
Actual: they're missing.

(Basically: frontpage:page_1:output:<hash>'s cache tags must be a superset of entity_view:node:1:teaser:*'s, but they're not!)

In this reroll:

1. Cleaned up AssertPageCacheTagsTrait. (Addresses #56b.)
2. Added \Drupal\views\Tests\ViewTestBase::assertViewsCacheTags()
3. Expanded test coverage for FrontPageTest, to demonstrate the problem with the output cache reported above. I'm also checking the results cache, because there too we want to know for sure that the right cache tags are present. And to verify it works correctly, I'm also rendering the frontpage view before any nodes are created, to check the correct cache tags are present in that case (they are!). BUT… in doing so, I'm running into bizarre problems:
   1. (DEBUG 1) Most frustratingly/puzzlingly: after the 15 test nodes are created and the node_list cache tag is invalidated 15 times, the Views result & output cache items (which has the node_list cache tag, as it should) continues to indicate that there are no nodes to list on the frontpage!? Since the cache tags were invalidated correctly, that leaves only one possible conclusion for me: static caching somewhere in Views. But if that's the case, then why doesn't \Drupal\views\Tests\Plugin\CacheTagTest::testTagCaching() suffer from that?
   2. (DEBUG 2) To test the results & output caches, I'm enabling tag-based caching on the frontpage view. Interestingly, reloading the view after having modified it seems to somehow forget that caching was enabled. Another static caching problem?
   3. (DEBUG 3) However, if I disable the assertions checking the views output & results caches before nodes are created, the static cache problem goes away, and the output cache assertions fail as expected. This is what I actually wanted to test, but in expanding the test coverage to also test the "before" situation, I'm running into all of the above…

(The two test failures are intentional, they demonstrate the problems/bugs in patch #58. But there would be more failing tests, if it weren't for the Views-static-caching problems I ran into while writing the tests. See bottom of #59.)

First, a thorough review of the current patch, including the test coverage. Conclusion: this looks great!

1. +++ b/core/modules/node/src/Tests/Views/FrontPageTest.php
   @@ -173,4 +186,105 @@ public function testAdminFrontPage() {
   +   * @todo Assertions ensuring the proper cache tags are present on the rendered
   +   *   views even when it's not using the 'tag' Views cache plugin.
   

   Doing this in my next comment.

2. +++ b/core/modules/node/src/Tests/Views/FrontPageTest.php
   @@ -173,4 +186,105 @@ public function testAdminFrontPage() {
   +          [
   +          'value' => $this->randomMachineName(32),
   +          'format' => filter_default_format(),
   +          ]
   

   Indentation. Fixed.

3. +++ b/core/modules/node/src/Tests/Views/FrontPageTest.php
   @@ -173,4 +186,105 @@ public function testAdminFrontPage() {
   +    $first_page_output_cache_tags = Cache::mergeTags($first_page_result_cache_tags, [
   +      'config:filter.format.plain_text',
   +      'node_view',
   +      'user_view',
   +      'user:0',
   +      'rendered',
   +    ]);
   ...
   +      Cache::mergeTags($first_page_output_cache_tags, ['rendered'])
   

   'rendered' is already in there. Fixed.

4. +++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
   @@ -191,7 +191,7 @@ public function cacheSet($type) {
   -        \Drupal::cache($this->outputBin)->set($this->generateOutputKey(), $this->storage, $this->cacheSetExpire($type), $this->getCacheTags());
   +        \Drupal::cache($this->outputBin)->set($this->generateOutputKey(), $this->storage, $this->cacheSetExpire($type), $this->storage['#cache']['tags']);
   

   This is caching a rendered render array. Hence it really is render caching. And thus should get the 'rendered' cache tag.

   If you're hesitant about that, then look at this argument: the Views output cache contains rendered render arrays. This means that the theme is already involved, and its templates have run. Theme settings use the 'rendered' cache tag. Therefore, whenever the theme settings are changed, all things affected by the theme must change, which includes all cached rendered render arrays, which includes those in the Views output cache.

5. +++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
   @@ -301,12 +298,18 @@ public function generateResultsKey() {
   -      foreach (array('exposed_info', 'page', 'sort', 'order', 'items_per_page', 'offset') as $key) {
   +      foreach (array('exposed_info', 'sort', 'order') as $key) {
            if ($this->view->getRequest()->query->has($key)) {
              $key_data[$key] = $this->view->getRequest()->query->get($key);
            }
          }
    
   +      $key_data['pager'] = [
   +        'page' => $this->view->getCurrentPage(),
   +        'items_per_page' => $this->view->getItemsPerPage(),
   +        'offset' => $this->view->getOffset(),
   +      ];
   

   So the results key was wrong before… interesting. This also means there was no test coverage for this aspect of the results cache. And now there is — awesome!

6. +++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
   @@ -343,18 +346,21 @@ public function generateOutputKey() {
   -  protected function getCacheTags() {
   +  public function getCacheTags() {
   

   This is now public because ViewExecutable::getCacheTags() calls it.

7. +++ b/core/modules/views/src/Tests/Plugin/CacheTest.php
   @@ -84,6 +84,48 @@ public function testTimeResultCaching() {
   +  public function testTimeResultCachingWithPager() {
   

   This says "WithPager", but it doesn't configure a pager anywhere… meaning that it's really the same test as above.

   … but then I realized you were setting page_1 as the display here, but the testTimeResultCaching() case uses the default display.

   So this actually makes sense. I made minor modifications to that other test case to clarify the difference.

8. +++ b/core/modules/views/src/Tests/RenderCacheIntegrationTest.php
   @@ -0,0 +1,116 @@
   +
   ...
   +    $tags_with_entity = array_merge($base_tags, $entities[0]->getCacheTags());
   ...
   +    $tags_exceeding_page = array_merge($base_tags, $entities[1]->getCacheTags(), $entities[2]->getCacheTags(), $entities[3]->getCacheTags(), $entities[4]->getCacheTags(), $entities[5]->getCacheTags());
   ...
   +    $this->assertViewsCacheTags($view, array_merge($base_tags, $entities[0]->getCacheTags()), array_merge($base_tags, $entities[0]->getCacheTags()));
   

   … etc.

   We always use Cache::mergeTags() to merge cache tags. Fixed.

9. +++ b/core/modules/views/src/Tests/RenderCacheIntegrationTest.php
   @@ -0,0 +1,116 @@
   +  public function testFieldBasedView() {
   ...
   +  public function testEntityRenderBasedView() {
   

   These were missing some docs and are pretty tricky to follow initially. Made some minor improvements that IMO make it easier to understand these tests.

Addressed #76.1.

(The interdiff is relative to #76.)

Stupid c/p error.

Alright, done.

I forgot to test FrontPageTest again — I broke it in the process due to the necessary changes I made to AssertViewsCacheTagsTrait::assertViewsCacheTags().

Fixed.

While I was rolling this patch, #2445761: Add a X-Drupal-Cache-Contexts header to aid in debugging and testing got committed, which caused this conflict.

#94: it was just a hunch. Compare #86 with #93 and you'll see that we have the exact same failures. So my hunch was wrong. :(
The thing is: this is green on my local D8. So… I really don't know what else to do. #91 can be reverted for sure, but I'm out of ideas. The only thing I can think of is to add a shitton of debug output and then analyze it again.

Hopefully you have an idea why Testbot has different results than a local D8?

I actually am running the tests using the UI. (I almost never use run-tests.sh.)

And yes, the "missing" cache tag is 'rendered'. It's not actually missing though: it is set despite us expecting it to NOT be set! It's happening for this assertion:

+++ b/core/modules/views/src/Tests/AssertViewsCacheTagsTrait.php
@@ -0,0 +1,81 @@
+    // Render array cache tags.
+    $this->pass('Checking render array cache tags.');
+    sort($expected_render_array_cache_tags);
+    $this->assertEqual($build['#cache']['tags'], $expected_render_array_cache_tags);

i.e. when we're inspecting the render array. A render array never receives the rendered cache tag unless it contains something that already was render cached. That's also why I tried #91.

Reproduced now. I've got a fix in the works, but can't finish it today. Expect it early tomorrow.

First, a small reroll that allows one to reproduce it even when using the UI test runner. The root cause for the difference in test results is that run-tests.sh always mocks a GET request, whereas the UI test runner doesn't.

Next up: a fix.

Now that we're able to reproduce the problem reliably, the real problem becomes obvious: the annoying presence of the 'rendered' cache tag in case of a render cache HIT.

If I render my render array, and a child of that just happens to be render cacheable, then the first time I render my render array, it'll have the expected set of cache tags, but the second time, it'll be the expected set plus the 'rendered' cache tag (due to the first time causing a render cache MISS plus a write so the second time it'll be a HIT).

This is a consequence that wasn't taken into account initially, and can easily be avoided: only set the 'rendered' cache tag on the render cache item, not on the render array that's about to be render cached!
This interdiff does exactly that. But it also ensures that we don't lose any test coverage; it in fact provides a better test coverage than HEAD: it verifies that render cache items indeed have the 'rendered' cache tag.

#107: uh, no, those are intentional. They're saying "hey, use the 'rendered' cache tag for this config (entity)!"

Diffing #105 and #111 locally shows sane changes only; the #111 interdiff is indeed wrong: the changes to PageCacheTagsIntegrationTest at the bottom are just noise; the rest shows the actual changes.