Views Integration - Consider compatibility with internal page cache

Oh god, is there never an end to this nightmare? Caching has practically become the new Views at this point …

I'm not sure what to do here, I have to admit. I'm long past understanding the caching layer. Your proposal does seem reasonable – if we can't bring the internal page cache to not cache our search results at all, then we should at least bring it to clear them at some point. And the search_api_list:[index] seems the way to do that.

However, I'd really appreciate some more input here from people who work with the cache layer before deciding anything here, lest I have to revert the whole thing a few weeks later.

I'm not sure if that's the best way to fix this issue. But it sounds like a good idea, can we write a test for this?

We have experienced search results cache issues on our project too. Pinged Wim Leers on Twitter - https://twitter.com/zerolab/status/905071088002093057. You definitely want input from either Wim Leers or FabianX on this.

I did some debugging on this matter today.

The "None" cache plugin returns the list cache tag by default. The information is derived from ::getEntityTableInfo (@see \Drupal\views\Plugin\views\cache\CachePluginBase::getCacheTags). These cache tags are crucial for cache invalidation and bubble all cache tags onto the page cache level.

Search API Views have no ::getEntityTableInfo. The return values from this function is empty. Therefor no cache tags and page cache never invalidates.

There are a few possible solutions

1. add a "None (Search API)" similar to the other two (tag & time)
2. Add cache tags on the query level (\Drupal\search_api\Plugin\views\query\SearchApiQuery::getCacheTags()), this is method called in \Drupal\views\Plugin\views\cache\CachePluginBase::getCacheTags
3. Populate ::getEntityTableInfo with the required info, even if the base table is not a real table.

Implementation of #5-2. Not sure if this is the right thing to do. I thought we could get rid of SearchApiTagCache and SearchApiTimeCache because the Query class takes care the cache tags, but the logic in SearchApiCachePluginTrait is quite complex. Maybe #5-1 is better after all.

@rjacobs The "None" Cache Plugins adds the list tag to the view.

Here is the None cache Plugin for Search API.

1. +++ b/src/Plugin/views/cache/SearchApiNoneCache.php
   @@ -0,0 +1,60 @@
   + * Defines a tag-based cache plugin for use with Search API views.
   

   /s/tag-based/none/

2. +++ b/src/Plugin/views/cache/SearchApiNoneCache.php
   @@ -0,0 +1,60 @@
   + * This cache plugin basically sets an unlimited cache life time for the view,
   + * but the view will be refreshed when any of its cache tags are invalidated.
   ...
   + * Use this for search results views that are fully controlled by a single
   + * Drupal instance. A common use case is a website that uses the default
   + * database search backend and does not index any external data sources.
   

   I don't think these docs are correct.

3. +++ b/src/Plugin/views/cache/SearchApiNoneCache.php
   @@ -0,0 +1,60 @@
   + *   help = @Translation("Cache results until the associated cache tags are invalidated. Useful for small sites that use the database search backend. <strong>Caution:</strong> Can lead to stale results and might harm performance for complex search pages.")
   

   This doesn't seem correct either, it should be: Never cache any of the results.

This implementation looks solid though, I think doing this is a good idea. It's probably also the most simple implementation we can do.

"No caching of Views data. This setting also helps enforce invalidation within caching layers external to views in a way that is compatible with search api. This is an appropriate choice for search api views that want to bypass views caching by still maintain basic compatibility with any page-level caches that may be enabled."

It's certainly tricky to explain what's going on here in a concise way.

I don't think there's a need to be so specific and verbose here. "No caching of Views data." will be all the user is interested in.
We should just make sure to disable the normal "None" cache plugin for Search API views, as we already do for the others. (Though I think that hasn't really been working completely, at least lately?)

I think, though, that I actually like the approach of #6 better: just set the cache tag on the query. If that works the same, it seems like a simpler approach than even overriding the "None" cache plugin. It looks like that might even allow us to remove our SearchApiTagCache::getCacheTags() override? Or am I mistaken? (And, might we need to instead override SearchApiTimeCache::getCacheTags() to remove that tag again? Seems like Core's time-based cache also has the normal entity list cache tags attached, so actually it seems that plugin would need to be fixed in any case, too.)

+++ b/src/Plugin/views/cache/SearchApiNoneCache.php
@@ -0,0 +1,60 @@
+    $tag = 'search_api_list:' . $this->getQuery()->getIndex()->id();

#2824640: Views cached results are not taking full cacheability metadata into account introduced cache tags in the Search API Query, making this change redundant.

#2824640: Views cached results are not taking full cacheability metadata into account introduced cache tags in the Search API Query, making this change redundant.

Just this line, or the whole issue?

My apologies for the comment in #13. I scanned the related issue without checking for the implementation of cache tags. In fact, there is explicit test coverage that shows the index list tag is missing for the 'None' plugin in ViewsDisplayCachingTest.

I agree with #12 that adding the list tag on the query is the preferred approach here, since it saves us the headache of adding an upgrade path for existing Search API views that use the existing None plugin.

Attached patch implements the following changes compared to #6:

1. Adds test coverage to check the search_api_list cache tag is added when using the 'None' cache plugin
2. Removed the getCacheTags implementation on the Search API Tag cache plugin, as its tags are now available through the SearchApiQuery

ViewsDisplayCachingTest still passes without changes to the Search API Time cache plugin, so it appears it is not necessary to remove the list tag again as suggested in #12

Attached patch implements the following changes:

1. Updated assertions for existing tests to include the list cache tag.
2. Turns out the Search API Time-based cache plugin needs to removed the list cache tag after all.

Tested on our project and the correct search_api_list cache tag now gets added to all pages that show Search API generated Views.

Thanks, great work! Also thanks @Lendude for testing.
Just two tiny corrections – if the test bot is happy with those then I’ll commit.

Committed.
Thanks a lot again, everyone!