Aborted queries cause problems in Views

Thanks a lot for reporting this! It seems indeed that aborted queries cause massive problems with Views caching. (See also #2871030: Cached Search API results in Views are NULL, with a similar problem.)
Posting such workarounds is also always a good idea, since (even if they aren't proper solutions) they clearly show that we can indeed influence the problem at hand. (Otherwise, with just that error message, it would seem more likely that it's just a Views bug.)

I now see that, in the query plugins build() method, we currently don't initialize the pager properly if the query was aborted. Which seems to make sense from our perspective, but since Views doesn't understand our concept of "aborted queries", it can't handle this in some cases (i.e., for cached results). Moving the pager initialization to the top of that method, to run even for aborted queries, should fix this problem.

I've also included a test (modification) that should verify this problem and the solution. The test might also require #2871030-2: Cached Search API results in Views are NULL to pass, though, so we might need to fuse the two issues and patches.

The test only patch doesn't fail, was it supposed to?

I did find one nit to pick.

+++ b/src/Plugin/views/query/SearchApiQuery.php
@@ -323,6 +323,13 @@ public function getEntityTypes($return_bool = FALSE) {
+    // Initialize the pager and let it modify the query to add limits.
+    $view->initPager();
+    $view->pager->query();

Can we add in documentation why this is at the top? So adding This is near the top of the method because views expects to be able to call the pager, even if we don't return any results.

The test only patch doesn't fail, was it supposed to?

As you've probably suspected: yes, it was. Damn.
I had feared the test wouldn't pass with just this fix, but instead we get one that even passes without it … Hm, guess I should try and get functional tests running again locally.
Or, could maybe someone else try to debug? Or at least verify that reproducing the bug works this way, just by …

1. Setting the "Search: Fulltext search" filter to exposed and required.
2. Setting caching to something other than "None".
3. Accessing/submitting the empty form multiple times (or even just once, in some cases, for some reason).

Suggested comment is added in the attached patch. I felt this was taken care of already by the comment immediately afterwards, but I guess this really makes it clearer, which can't hurt.

Note (mostly to self): Once we get a working test, we should also verify it correctly tests the fix in #2871030: Cached Search API results in Views are NULL.

OK, I think I've done it! Was quite some work, but eventually it turned out that, for some reason, both page cache modules were enabled and, of course, massively interfered. I now just clear those caches before each of the page requests to really test the Views cache plugin – and now it works, and even correctly catches #2871030: Cached Search API results in Views are NULL.
Patch attached, please test/review!

(Also, I now can run functional tests locally again – a mixed blessing, as I now don't have any excuse not to debug those. Blergh.)

Looks good, one nit to pick though.

+++ b/tests/src/Functional/ViewsTest.php
@@ -390,6 +391,12 @@ protected function regressionTest2869121() {
+      \Drupal::getContainer()->get('cache.render')->deleteAll();
+      if (\Drupal::moduleHandler()->moduleExists('dynamic_page_cache')) {

I don't see why you'd need to do the if here, we control those modules ourselves.

I don't see why you'd need to do the if here, we control those modules ourselves.

That's because I'm not actually sure where the module is enabled/installed. It's not in our code, as far as I can see. And if I don't know where it comes from, my thinking was that I can't guarantee it will stay there. (My bet is that this was enabled at some point in Core, by default, for all browser tests, so the page cache functionality would be automatically tested. But I couldn't even verify that.)

On the other hand, I guess it's very likely that it will stay there, now that Core is stable, and if it's removed, we can still add the if, or remove the line. So, if you think we should remove it, we can also go with that – your choice.

Was quite some work, but eventually it turned out that, for some reason, both page cache modules were enabled and, of course, massively interfered.

This triggered some intensive facepalming.

+++ b/tests/src/Functional/ViewsTest.php
@@ -383,10 +388,20 @@ protected function regressionTest2869121() {
+      // Flush the page-level caches to make sure the Views cache plugin is
+      // used.
+      \Drupal::getContainer()->get('cache.render')->deleteAll();
+      if (\Drupal::moduleHandler()->moduleExists('dynamic_page_cache')) {
+        \Drupal::getContainer()->get('cache.dynamic_page_cache')->deleteAll();
+      }
+      $this->submitForm([], 'Search');
+      $this->assertSession()->addressEquals('search-api-test');
+      $this->assertSession()->responseNotContains('Error message');
+      $this->assertSession()->pageTextNotContains('search results');

This tests is pretty pointless, because it's explicitly emptying the render cache and Dynamic Page Cache (although NOT the Page Cache)… which basically means that you don't know whether this module behaves correctly with render cache and Dynamic Page Cache enabled (which is the case for pretty much every D8 site).

Rather than doing this, you should be A) asserting the X-Drupal-Cache and X-Drupal-Dynamic-Cache response headers, B) fixing Search API's code so that it also works correctly when those modules are installed.

I don't see why you'd need to do the if here, we control those modules ourselves.

Indeed.

That's because I'm not actually sure where the module is enabled/installed. It's not in our code, as far as I can see. And if I don't know where it comes from, my thinking was that I can't guarantee it will stay there.

Ehm… class ViewsTest extends SearchApiBrowserTestBase, abstract class SearchApiBrowserTestBase extends BrowserTestBase; the latter has:

  /**
   * The profile to install as a basis for testing.
   *
   * @var string
   */
  protected $profile = 'testing';

And this is testing.info.yml:

name: Testing
type: profile
description: 'Minimal profile for running tests. Includes absolutely required modules only.'
version: VERSION
core: 8.x
hidden: true
dependencies:
  # Enable page_cache and dynamic_page_cache in testing, to ensure that as many
  # tests as possible run with them enabled.
  - page_cache
  - dynamic_page_cache
# @todo: Remove this in https://www.drupal.org/node/2352949
themes:
  - classy

So there's your reason, there's no magic involved here at all.

Awesome, thanks for the information Wim! Really helpful.

@drunken monkey: because we explicitly want to test the views cache plugin I think we should probably keep the cache clears but we should remove the if around the dynamic page cache clear.

We should probably also check for the X-Drupal-Cache and X-Drupal-Dynamic-Cache headers after doing the loop and checking that those are outputted as expected.

This tests is pretty pointless, because it's explicitly emptying the render cache and Dynamic Page Cache (although NOT the Page Cache)… which basically means that you don't know whether this module behaves correctly with render cache and Dynamic Page Cache enabled (which is the case for pretty much every D8 site).

The point was that it's working with the two page cache modules, which is pointless for a regression test. For that, we had to make the code fail without the fix, so needed to disable those modules. Makes sense, right? As you see, we already have the modules enabled for all other tests.

So there's your reason, there's no magic involved here at all.

I never said it was magic, just that I couldn't be bothered to go through all the code to find out where it's enabled. And if you read my paragraph above, my guess was actually spot-on. So the question, as I said before, is just whether we want to rely on Drupal's testing framework keeping those modules enabled by default. If you're both in favor, sure, let's do that. I guess it really is unlikely they'll go back on that.

Please try to be a bit less condescending here. I did my best in the other issue, too. (At least after your reprimands.)

But yeah, thanks for the information!

We should probably also check for the X-Drupal-Cache and X-Drupal-Dynamic-Cache headers after doing the loop and checking that those are outputted as expected.

OK, sure. Though it doesn't seem like that's the thing we want to assert there, it does make sure we're actually testing the correct thing.
So, like this, I guess?

Yeah, this looks good.

Good to hear, thanks for reviewing!
Committed.
Thanks again, everyone!