SearchQuery lacks preExecute(), making countQuery() run even when there are problems in prepare step

Thanks for filing this! Cleaning up issue summary and title.

So... I took a look at the code in the SearchQuery class.

Under normal circumstances, I do not see how the query is being run in this case. Here's what the code should/is doing:

execute() calls prepareAndNormalize(), if it hasn't already been called elsewhere.

prepareAndNormalize() calls parseSearchExpression() to parse out the string the user submitted into search words and and/or SQL conditions.

parseSearchExpression() calls parseWord() repeatedly, to actually parse out the words in the submission. In this method, only words longer than \Drupal::config('search.settings')->get('index.minimum_word_size') are added to $this->words and returned. I have verified that the parseWord() method is the only place that anything is added to $this->words.

After calling parseSearchExpression(), prepareAndNormalize() says:

    if (count($this->words) == 0) {
      $this->status |= SearchQuery::NO_POSITIVE_KEYWORDS;
      return FALSE;
    }

It is only after this if() statement that prepareAndNormalize would set $this->normalize to a non-zero value. I have verified that $this->normalize is never set to something other than zero outside this method.

After calling prepareAndNormalize(), execute() says:

    if (!$this->normalize) {
      // There were no keyword matches, so return an empty result set.
      return new StatementEmpty();
    }

So. It looks to me as if with the default setting where anything less than 3 characters is discarded, no query will be run if you enter a search with only 1 character words.

We do have a few tests that change that setting; in this case, the query should be run if it finds any words with length >= the minimum word setting.

So I do not think there is a bug here. I'll just set this to "postponed (maintainer needs more info)". If you can find a case where something is actually wrong, please point out where it is so I can investigate.

Thanks!

Ohhhhh... so you're saying that the PagerSelectExtender being added after SearchExtender means that its execute() takes precedence over SearchQuery. Right!

But your patch that switches the order, in #7, doesn't work -- the paging doesn't work. So it looks like the order has to be what it was. Also we cannot remove the grouping, that was necessary to get PostgreSQL to work on the other issue.

So. Given that, I think this can still be fixed. Looking at PagerSelectExtender::execute(), it will abort if preExecute() returns FALSE. That will call preExecute() on SearchQuery and any other extenders, or the base query.

So I think all we will need to do is to add a preExecute() method to SearchQuery, which it should have anyway to make it more standard. Here's a proposed patch. Thoughts?

Nice, seems that patch from #10 makes the groupBy() obsolete. So patch should include the removal of the groupBy() and I will do a another final test run before commit to make sure.

It looks like the patch in #15 is exactly the same mine in #9 except for the added comment.. but I am not sure about the comment really.

So. Let's think about this. What happens really on extenders is that when you do something like $query->execute() or $query->countQuery, the last added extender's method gets called first, and it generally adds stuff to the query and then passes the call on to the next extender, and so on down the chain until you get to the innermost select itself.

The Pager extender, during its execute(), invokes the countQuery() method to do a count and set up the query limits. When it does that, it needs to get a count on the actual, final, fully-modified query, not a partially-constructed query. So this is a fundamental property of the PagerExtender: it has to be last in the list of extenders. It is not something specific to a Search query.

So. I think maybe we should take that comment out of NodeSearch, and instead add a note to the PagerSelectExtender class that it must always be the last extender added to any query. Maybe that's a separate issue, but since it was noticed here and really only needs to be one or two lines of documentation, let's just do it now.

Also, in #14 it says we can take out the groupby that was added in the other issue. Should we do that now?

Also fixing the title.

Patch in #15 just passed PG bot, nice work!

Oh yeah I see we have removed the groupby. Ok then all we need to do is put docs in Pager extender. How about this? @bzrudi71 can you test on PostgreSQL? Adding tag for reference.

I am +1 on marking this RTBC, if both @andypost and @bzrudi71 agree.

Oh, OK, so we are good with PostgreSQL! The new patch only differs in documentation so it should be fine too.

Either @andypost or @bzrudi71 want to review the docs changes and mark this RTBC?

Great thanks! I'm adding beta eval to issue summary and updating title slightly again.

Adding note to the summary about why we are adding docs to Pager extender too.

Nice. Committed 09aad27 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.