Use new cache tag ENTITY_TYPE_list:BUNDLE in Views to improve cache hit rate

Perhaps you can simply disable caching in your views configuration?

I wouldn't really call this a bug, it's mostly more like an intended feature of the Drupal cache system.
Some comments about this: https://www.drupal.org/project/views_custom_cache_tag tries to provide a solution for this.
There is also a core issue, see https://www.drupal.org/project/drupal/issues/2145751 . Maybe I would suggest to mark this issue as "closed (worked as intended)"

Yes, this overlaps with several other issues. #2145751: Introduce ENTITY_TYPE_list:BUNDLE cache tag and add it to single bundle listing will allow to use views_custom_cache_tag without custom code but it will still require additional modules and manual configuration.

#2498857: Support min-age in render caching is another approach, that adds min-age to render caching.

As @dawehner said, the current behavior is for many cases not optimal but it is correct in ensuring that the correct information is shown all the time. I think strategies to change that, as in forcing longer caches or skipping invalidations, should be done in contrib. There could be "time only" cache plugin for example. Note that it still needs to respect the cache tags specific entities that are shown, or you might be unable to unpublish or delete content, whic is important.

> If a component doesn't use cache, it may not use cache tag either.

Yes it does, because there's still the page cache, surrounding render caches and possibly other things, none just means the view isn't cached on its own.

> But the core can't just keep this big performance leak without providing choice / solution.

That's where I disagree, this is not a leak, this is how it is designed to work. Unlike drupal 7, where content changes invalidated every content cache, D8 still has the nodes and other blocks render cached, just the parts that have to are invalidated. It's not optimal, but there are ways to improve that.

> Or maybe create another cache plugin "None - Without cache tag".

I don't think none without cache tag is a good idea, but time-based without the list cache tag could be an option yes, but I don't think that needs to be in drupal core.

node_list cache tag is definitely a bottleneck for performance issues if we have more content types and content editors changing content more frequently and we are displaying views block with nodes in many pages. Actually we use views with lot of filters to create the page/block display, But the more generic node_list tag invalidates caches on all pages where this views block displayed even though non-relate content updates.

One solution as of now is using this module https://www.drupal.org/project/views_custom_cache_tag and it allow you to enter view specific cache tags something like entity:bundle:display-mode / mysite:node:category: and then write custom code to invalidate this particular cache in entity presave hooks.

Right now there is no out of the box solution available for this issue, Some related issues are already raised and discussion going on for example https://www.drupal.org/project/drupal/issues/2145751 talking about adding bundle name also to the node_list tag like node_list:page.

I think it would be more logic to have the node_list tag for "None" and "Tag based" (improved by 2145751).
And to have no tag for "Time based".

What do you think ?

Yes, the time based caching that exists at the moment is pretty useless, I do agree with that. Still, changing the behavior of an existing plugin is problematic as it will change for sites.

One option would be to add another cache plugin and have descriptions/explanations for the behavior difference.

There are also issues to support min-age caching, so it would return it from the cache for at least X seconds. But that's challenging and just having a time-based version that's not invalidated by new entities might be best solution.

The problem is that there will be confusing cases, imagine a view with time-based caching and a pager. It shows node 1-5 on the first page and 6-10 on the second. Now, node 3 gets unpublished or deleted. You'd expect the first page to now show 1,2,4,5,6 and the second page 7-11, because that moves up by one. With the list cache tag, that happens, but without that, only the first page that actually has node 3 gets invalidated. So a user will see item 6 on both pages. Not a huge deal, but still, it will definitely result in confusion and bug reports :) So it will be important to explain the advantages/drawbacks of both options (the only advantage of the current option is that it will definitely rebuild after X seconds, in case changes are not something that can be invalidated (e.g. "upcoming events starting in the future"))

Postponed on #2145751: Introduce ENTITY_TYPE_list:BUNDLE cache tag and add it to single bundle listing, that makes sense.

Let's not forget @berdir's #17:
> Still, changing the behavior of an existing plugin is problematic as it will change for sites.
> One option would be to add another cache plugin and have descriptions/explanations for the behavior difference.

Or we add a setting on the bundle filter plugin.

In any case,
* the code to add the bundle cache tags might better live in \Drupal\views\Plugin\views\filter\Bundle::getCacheTags (to not violate Encapsulation)
* we'd still need to remove the list cache tag in CachePluginBase:getCacheTags depending on the filter tags
* we'll have to account for the case of ORed filters. If you OR two bundle filters, the resulting tags are different as when you AND them. When you OR a bundle filter with a is-published filter, the result is list tag.

FWIW, my comment was about changing the behavior for the time cache plugin, this is a bit different and might be OK, but we'll have to figure that out once the blocker is committed (which btw now should be ready and needs reviews to be able to set it to RTBC).

#2145751: Introduce ENTITY_TYPE_list:BUNDLE cache tag and add it to single bundle listing is RTBC now, so unpostponing this.

Marking needs work because there already is a patch, in #19!

At least bundle filter could add tags

valid patch

So the bundle filter is the safest way to add the cache tags, but is there a way for views to then not add the entity_list cache tag when those are added? Otherwise we don't get the actual cache invalidation benefits.

I think kind of it could work, basically here could be more then one bundle filter so they add override for "old" list tags

Cache tag names fixed, also changed comments a bit

filters are null by default( could use follow-up to set default value to empty array

+++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
@@ -234,14 +235,35 @@ public function generateResultsKey() {
+    $filters = $this->view->filter ?: [];
+    foreach ($filters as $filter) {
+      if ($filter instanceof Bundle && $filter->value) {
+        // Bundle filters could provide optimized cache tags when limit query to
+        // specific list.
+        $entity_type_id = $filter->getEntityType();
+        $filter_tags[$entity_type_id] = array_map(function ($bundle) use ($entity_type_id) {

we'll also need to check the operator, as it could be an exclude condition.

I'm wondering if there are any scenarios where this could cause a problem other than that, not sure yet.

> we'll also need to check the operator, as it could be an exclude condition.
> I'm wondering if there are any scenarios where this could cause a problem other than that, not sure yet.

Good point. What i stated in #24 implies that any correct implementation must look on all negations, conditions, and condition groups.
And pass at least these tests (assuming we only have page and article bundles):

• bundle-in(page) => node_list:page
• NOT-bundle-in(page) => node_list:article
• bundle-in(page) AND is-published => node_list:page
• bundle-in(page) OR is-published => node_list
• bundle-in(page) AND bundle-in(page,article) => node_list:page
• bundle-in(page) OR bundle-in(page,article) => node_list:page,node_list:article
• bundle-in(page) OR NOT-bundle-in(page) => node_list
• (bundle-in(page) AND is-published) OR (NOT-bundle-in(page) AND is-published) => node_list

One other edge case could be views with relationships to the same entity type, where a bundle filter exists on either the queried entity or the related entity and does not exist on the other. This might mean that the view should still have the ENTITY_TYPE_list tag since changes to entities of different bundles could yield different results for the view, but I think if I understand the patch correctly, this wouldn't happen.

It looks like since $this->view->getQuery()->getEntityTableInfo() contains results for each relationship (keys generated with $entity_tables[$relationship_id . '__' . $relationship->tableAlias]), if we set the keys of $filter_tags to match we could check the filter against each relationship instead of against each entity type.

Yes, it can get complicated. For BC and edge-cases, we could add trying-to-use-bundle-cache-tag as a new setting, default to enabled for new views while existing ones would default to false?

Hi Berdir,

I was thinking the same, but perhaps it might be worth initially committing the change with it defaulting to disabled in all circumstances and change to defaulting to enabled for new views in a follow up once it's been soak-tested in the real world for a while.

Hi there.
I agree that this bundle list cache option should not be enabled by default at this point, so I created new views cache plugin that fallback to default cache tag plugin if there is no bundle filter in views. It works with IN and NOT IN operators as well as with grouped filters, please check it.
I also tried to find out how to correctly create a test for this new plugin, but I have not found anything how to easily build a view with filter on the fly.

@berdir #40:

(#38)
> [This] implies that any correct implementation must look on all negations, conditions, and condition groups.

I doubt that anyone trying to code that understood that this effectively means duplicating the query builder. Or extend the api to have caching passed up from filters. Which both is complex, and the first an imho too big maintenance burden for the effect.

Instead i'd advocate let sitebuilder add cache tags, and crafting a good help text. (effectively moving your fine views_custom_cache_tag module into core)

@geek-merlin I see your point, but what I`ve done it is completely safe as it is just fallbacks to default cache tag plugin and the new cache plugin will cover like 90% of views I guess (I have not seen some complex views with many grouped filters yet). So my point is that kind of solution should be available for end users either in Drupal core or in contrib module.

@berdir what do you think about moving patch #42 from this issue to your views_custom_cache_tag module?

I'd be open to adding it to my project if we can't get it into core easily, probably as another cache plugin or a setting in the existing one?

There is an issue with the path for the new file in the patch from #42, there shouldn't be "web" in it. E.g.:
"b/core/modules/views/src/Plugin/views/cache/EntityListBundleTag.php" instead of "b/web/core/modules/views/src/Plugin/views/cache/EntityListBundleTag.php"

There were times when I received the following warning:

Warning: Invalid argument supplied for foreach() in Drupal\views\Plugin\views\cache\EntityListBundleTag->isSupported() (line 87 of core/modules/views/src/Plugin/views/cache/EntityListBundleTag.php).

So I added some checks on $this->view->filter.

Added #49, Warning message issue has been resolved on 9.1

Im really liking this idea,

It works wonders on most displays, but not for the ViewsBlock display,
It looks like its because $this->view->buildRenderable() is called ahead of View::preRenderViewElement(), the default cache tags like node_list are being added to $output['#view']->element['#cache'].

In fact, this means anything that uses the \Drupal\views\ViewExecutable::buildRenderable()method, like the block display or attachment display, will not be able to use the more specific cache tags.

I think there is two ways to go about this,

option 1

We can improve the caching process so that we only set the cache metadata after the View is rendered and the related cache tags are "calculated". I think we could avoid applying Cache Metadata in \Drupal\views\Plugin\views\display\DisplayPluginBase::buildRenderable().

option 2

Perhaps, we can unset the cache data in the ViewsBlock.php.
I don't like this one since that requires any plugins that use buildRenderable() to make a similar change to utilize the better cache.

Any thoughts on the 2 options?

Attached is a patch that does option 2 for now.

Removed unused use statement.

This patch based on #34, and did some improvement.

Remove useless ensureMyTable().

Moving this to needs review as there is a patch that needs to be reviewed.

Reading the latest patch on this issue I don't see how this is going to replace the node_list cache tag that the view already has. It's going to add the additional bundle tags in but the generic node_list is not being removed. I've just created an issue to discuss how the published filter should affect the cache tags - #3214362: Add and use a node_list:published cache tag. I think these issues should work together to discuss how filters / where's can alter the list cache tags from entity queries / views / json lists.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

patch 56 had some fuzz, new reroll has no fuzz.

The tests written for the views_custom_cache_tag module could inspire / be borrowed to help more easily create automated tests for a new patch, this would accelerate the creation of automated test coverage and hopefully move this issue forward more quickly.

Given the performance gain opportunity here and the wide proliferation of the views module and views in general , I think this is a worthy core effort that merits the effort.

Any volunteers? https://drupal.org/project/views_custom_cache_tag

I like the small scope of the above patch, if we can keep it simple , add the necessary test and no more, we can move this forward in a timely manner.