Set search api caching as default for new search api views

Just asked @StryKaizer to also test time based caching. Attached patch also fixes cs.

Let's see if I can figure out the upgrade path.

I tested time-based and that doesn't seem to be working either. So adding in that as well. I also added the upgrade path.

Quite major as it is causing a lot of confusion from what I heard. Only "clones" of the default view is working till "it is all broken". Let's get this in asap.

We need some tests to make sure that the "old" views get updated accordingly. If we decide to not do this, we should make it clear in the release notes. Borisson told me there were 4 issues in Facets that were caused by this bug. drunken_monkey, opinions?

+++ b/search_api.module
@@ -507,3 +509,30 @@ function _search_api_search_module_warning() {
+      $display['default']['display_options']['cache']['type'] = 'search_api';
+      $entity->set('display', $display);

btw is there schema for this value

Thanks for creating this issue. I wasn't aware that a cache plugin is set by default, but this really does sound like a problem we need to fix. Facets won't be the only components affected by this, and this might also mess with the search results itself.

However, I don't think the approach in the patch is the best way to fix the problem.
First off, there were a number of smaller problems with the patch, fixed in the attached revision.
Secondly, though, it seems to me that search_api_views_plugins_cache_alter() would be a much better place to restrict the available cache plugins. It would be quite a bit of data to set, granted, but the code should be simple: just set 'bases' to all non-Search API base tables for all cache plugins that don't have the key set already. Doing a form alter seems a much shakier solution for this.
Also, regarding hook_view_presave(): wouldn't this result in overwriting the cache backend even when importing a finished view – which might have caching set to "None" (as, I think, is even what we want to recommend – "Search API specific" is better than the default ones, but will still often report stale results and have a very low rate on cache hits for a typical site)? If we want to set a default, I'm for "None", and in any case we should take care not to overwrite existing settings.
(Regarding that, even in my attached patch we might want to change the update function to only change the cache plugin if it is one of the two we're removing.)

As for tests, does anyone know how to write update path tests? Are there any examples we can look at?
If it's easy, we should definitely add a test (good practice, and best to start early with that), but otherwise I think we can also skip it for this issue.

btw is there schema for this value

That's Views default configuration. If that doesn't have a schema, it's Views' problem, not ours.

Just had another bugreport for this: #2817269: facets blocks appear only when the cache is cleared

I don't know how to write update tests, not sure if that's easy to do. It's also a very simple change. I wouldn't mind just committing as-is. I did work on this patch so feel free to not commit this just on my rtbc alone.

I agree regarding tests, but what about all the other arguments I make in #10?
Also, I think hook_ENTITY_TYPE_create() might be more appropriate than hook_ENTITY_TYPE_presave().

create does sound like a better place to do this. I also agree that having none as a default is a good idea.

I don't think create is a good idea, as that's not when a first save is triggered. I did change the default cacheing mechanism though.

I don't think create is a good idea, as that's not when a first save is triggered. I did change the default cacheing mechanism though.

But if we do it in the create hook, people will get the right default, right? I.e., if someone creates a new view, it will be right there when they get to the edit UI for the view?

Also, the update hook still needs to be fixed, to only change the plugin if it isn't 'none' (or already 'search_api').

+++ b/search_api.install
@@ -125,3 +126,21 @@ function search_api_requirements($phase) {
+/**
+ * Implements hook_update_N().
+ *
+ * Updates the views that are based on search api to the search_api cache type.
+ */
+function search_api_update_8001() {

I wouldn't do this, this might break existing sites.

There are two valid reasons where people will not be using the default cache plugin: the first is because they use no caching (which is the default), and secondly because they are using their own custom plugin that is tailored to their use case.

This also contradicts what this issue is about - to default to the Search API cache on NEW views.

+function search_api_entity_presave(EntityInterface $entity) {
+  if ($entity InstanceOf View && !isset($entity->original)) {
+    // Set default search_api caching for search api views.
+    $dependencies = $entity->get('dependencies');
+    if (in_array('search_api', $dependencies['module'])) {
+      $display = $entity->get('display');
+      $display['default']['display_options']['cache']['type'] = 'none';
+      $entity->set('display', $display);
+    }
+  }
+}

Is this reliable? This code will be executed whenever a View is saved, not only when one is created through the interface. The !isset($entity->original) seems to be to detect if the entity is new, but that can be better achieved with $entity->isNew().

The problem is that this will force the disabling of the cache on any new view that is being saved to the storage, so this will also happen when importing a new view in production using CMI, or whenever any view is created programmatically or through REST etc.

I think the idea is that if a site builder sets up a new view through the UI, the caching plugin should default to 'none', but not when saving a new view entity programmatically.

How does Views decide that the 'tag' plugin is the default? Can't that be altered somehow?

Thanks for weighing in here, pfrenssen!

For the update hook, maybe a compromise would be to only update this if the setting is one of the two cache plugins provided by Views itself (tag or time)? That should let us cover most accidental misconfigurations, while also not changing any (or only very few) configurations that are deliberate.

But maybe leaving this out completely is indeed the better option. It's just that a lot of existing sites already seem to have problems, so we'd need a different solution for those. (Maybe a hook_requirements() warning?)

You're right about the pre-save code – like this, it's not acceptable, since it would prevent any kind of cache plugin changes. Might be acceptable if it only changes the two Core Views cache plugins, though. But I'd still prefer it as post-create code.

Another thing I now noticed, though: Why do we only change this for the default display? Can't this be overridden in other displays?

Thanks for weighing in here, pfrenssen!
For the update hook, maybe a compromise would be to only update this if the setting is one of the two cache plugins provided by Views itself (tag or time)? That should let us cover most accidental misconfigurations, while also not changing any (or only very few) configurations that are deliberate.
But maybe leaving this out completely is indeed the better option. It's just that a lot of existing sites already seem to have problems, so we'd need a different solution for those. (Maybe a hook_requirements() warning?)

I think leaving it out and just making a mention in the next release notes should be sufficient.

You're right about the pre-save code – like this, it's not acceptable, since it would prevent any kind of cache plugin changes. Might be acceptable if it only changes the two Core Views cache plugins, though. But I'd still prefer it as post-create code.

Sure, I guess that might help, I don't really have time to look at it this week but if no one has picked this up when I find time for this, I'll change that.

Another thing I now noticed, though: Why do we only change this for the default display? Can't this be overridden in other displays?

No idea. We should probably loop over all displays.

Removed the update hook en only react on hook_insert. Tested locally and it looks like it works like a charm.

I think this is OK. I would add some documentation here though to explain why we are forcing people's cache plugins to 'none' when they save using the standard 'tag' or 'time' plugins. People might wonder what is going on here, why we're messing with their entities on save :) If possible also maybe log a message in the watchdog to alert them.

Maybe we can do a drupal_set_message if we changed the cache plugin?
How about drupal_set_message(t("We changed the cacheing strategy of this view 'none' to make sure that you don't run in to unexpected problems, such as facets not working"));?

I think leaving it out and just making a mention in the next release notes should be sufficient.

Unfortunately, no-one really reads release notes, so this might still get us a few issues with people complaining about this. So I'm not sure we shouldn't just do it with an update function. After all, if we just change "time" and "tag", it's sure to be the right thing to do in almost all cases.

Also, I don't like this checking for dependencies. Doesn't checking for the query plugin make more sense? Patch attached, please test/review!

Unfortunately, no-one really reads release notes, so this might still get us a few issues with people complaining about this. So I'm not sure we shouldn't just do it with an update function. After all, if we just change "time" and "tag", it's sure to be the right thing to do in almost all cases.

Hm, I understand. I'll have a look at the patch after dinner, do you have an opinion on the message? (should we add it / is it a good one?)

Setting this to rtbc. I think this new default is good and the implementation makes sense, the update hook will only works for the core cacheing methods that were previously also unsupported.

The message would make sense, yes. My suggested wording (see attached patch):

The selected caching mechanism does not work with views on Search API indexes. Please either use one of the Search API-specific caching options or "None". Caching was turned off for this view.

I don't really understand your argument against (or is it in favor?) an update function. Changing the cache setting away from unsupported plugins is exactly what we want, isn't it? So I'd just do that, and notify the user in the update function's return value of the changes.

And now again with a proper update function. Already tested it, too.

That looks great!

Good to hear, thanks for your continued input on this!
Committed.
Thanks again, everyone!