SearchQuery exact search conditions not added to CountQuery

This is actually a problem with the search module. Here's what's going on:

In search.api.php:hook_search_execute():

  $inner_query = clone $query;
  $count_query = db_select($inner_query->fields('i', array('sid')));

Since $count_query is being created as a regular SelectQuery object with an inner query, the original SearchQuery::execute is never run, which adds conditions for complex queries. From search.extender.php:execute():

    $this->join('search_dataset', 'd', 'i.sid = d.sid AND i.type = d.type');
    $this->condition($this->conditions);

The attached patch moves the above code from SearchQuery::execute to a new function so it can be called from hook_search_execute for the $count_query.

This needs a test.

We should set it back to needs work until the tests are added.

Good catch though and the patch looks good!

Idea for a test:
- Create a bunch of nodes (say, 17) containing the phrase "love pizza".
- Create a bunch of nodes (like another 17) containing "love cheesy pizza".
- Verify that the count of pages is 4 when you search for love pizza (searching just as keywords)
- Verify that the count of pages is 2 when you search for the phrase "love pizza".

I'm working on the test, thanks for the ideas jhodgdon!

Test included.

Hmmm...

Shouldn't the page count be 2 for the exact phrase and 4 for the keywords in the test? You are just testing whether a link to page=3 exists. Maybe also check to see that page=1 link is there in both cases, and page=2 is also missing in the last test case?

Also, when you're adding a new test, you should follow the doc guidelines and put some doxygen header comments on the test class and the test function. I realize that many of the other tests in core are not documented that way, but they should be. :)

e.g.

/**
 * Tests that searching for a phrase gets the correct page count.
 */
class SearchExactTestCase extends DrupalWebTestCase {

etc.

I've changed the test to check that each of the expected pager links are found for the phrase and keyword searches. Also added comments. I was thrown off by the lack of comments in the other tests :).

Great! The code looks good, including the test.

A few text suggestions. See http://drupal.org/node/1354 for our doc standards...

a)

   /**
+   * Add exact search conditions to query.
+   */
+  protected function addConditions() {

Function doc headers should start with 3rd person verb, like "Adds exact search conditions..." And yes, I realize that much of our existing code does not follow that standard. Sigh.

b)

+/**
+ * Tests that searching for a phrase gets the correct page count.
+ */
+class SearchExactTestCase extends DrupalWebTestCase {
+  public static function getInfo() {
+    return array(
+      'name' => 'Search engine exact queries',
+      'description' => 'Tests exact search queries.',
+      'group' => 'Search',
+    );
+  }

The description of the class in the doc doesn't match well with the description in the getInfo(). I like the doc header description better, since it mentions searching for a phrase (all search.module content search is "exact" in the sense that only exact keyword matches are returned).

c)

+    $this->assertNoLinkByHref('?page=4', 0, '4th page link does is not found for keyword search.');

does is not found -> is not found

d) Same in this line:

+    $this->assertNoLinkByHref('?page=2', 0, '3rd page link does is not found for exact phrase search.');

:) Thanks, jhodgdon! Seems like docs would be hard to keep consistant. I've updated the patch per your suggestions.

Well, it looks like the tests don't work correctly. There's also a typo here:

+    $this->assertLinkByHref('?page=3', 0, '4th page link is found for keyword search.');
+    $this->assertNoLinkByHref('?page=4', 0, '4th page link is not found for keyword search.');

That last one should be 5th page link?

The documentation and text looks good other than that. If you can just get the tests to pass...

Hmm, the tests run correctly on my local install (most recent version of HEAD). There was an error with assertNoLinkByHref (second parameter should be message), maybe that's it?

They are running for me on my test machine too, with your latest patch. Odd. I'm running HEAD from about 12 hours ago.

#17: searchquery-791270-17.patch queued for re-testing.

Ah...

So I think the test bot runs without clean URLs enabled. And I think if you don't have clean URLs, the href for the links doesn't have ?page=1 in it, but instead &page=1.

That is probably the problem. I think you can modify the test to say something like (in pseudo-code):
if (clean urls are enabled)
test for ?page=1
else
test for &page=1
end if

Worth a try...

Ahh, good find!

This should fix the tests.

We don't need to test for the &/? in the string, no point in complicating the tests. I simply removed '?' from all of them.

#24: 791270-searchquery.patch queued for re-testing.

Looks like the test client malfunctioned.

#24: 791270-searchquery.patch queued for re-testing.

Looks like the test bot got itself working...

This is a sensible fix to a critical issue, complete with a test and good doc. Great work duelj and lotyrin!

I noticed that something is wrong with SearchQuery too myself, and tried to fix it in #753084: Extenders and subqueries don't play nice. Instead of adding another method, my patch added a countQuery() implementation to SearchQuery, which is imho nicer than adding a new method that needs to be called.

I am not sure if this does fix the problem related here, but maybe we can merge the SearchQuery changes from the other issue into this? It doesn't belong into the other one anyway.

I think we should get the patch proposed here committed. It looks like there's some disagreement on that other issue about whether the patch there is a good idea. In the meantime, we have a serious issue in search that needs this fix.

Mmm. I'd like to get sign-off on this from a DBTNG maintainer on this approach. Berdir's comment sounds like the opposite of sign-off. It seems to be introducing something non-standard for SearchQuery only, which seems sub-optimal.

If we could bring Crell around on #753084: Extenders and subqueries don't play nice, that does indeed sound like a more holistic fix.

Uhm, sorry, I was not exactly clear above.

The mentioned issue does not fix this bug, they are not really related. But the changes introduced there would (correctly) break how the count query for node searches is created so I moved that part into SearchQuery since it can imho do that automatically.

Thinking about it, that change itself will not fix this bug since countQuery is called before execute() but it will allow to hide the call to the newly created function here, which means that we only need the function internally and can make it protected.

The discussion in the other issue was about the initially reported bug, not my changes to SearchQuery. But someone should probably confirm that my assumption about SearchQuery always knowing how to create the count query (not only for nodes!) is correct.

I really think that all this patch does is make sure that the same conditions are added to the count query as to the main query in search.

That other issue is doing something different -- changing the structure of how queries are done -- which is a totally separate issue, it seems?

Since the other issue is stalled, what I would like to see is to get this patch and its test committed now, so that whatever patch goes in for the other issue, which will change around how search is doing its querying, keeps this bug fixed.

Hm. Here's a patch to demonstrate what I mean. If you do not like it, then I'm happy to postpone it to a follow-up issue (again, the change does not belong into the other issue).

With the patch in #24, all developers that use SearchQuery *must* call addConditions() and also correctly define the count query (Which is just a copy & paste of what node.module is doing, but still... ). With my patch, the count query "just" works, you do not need to know the internals of SearchQuery to be able to use it.

Another (minor) point in #24: It defines the addConditions() method as protected but then calls it from the outside. This *really* confused me and is because we have a __call() method, which then calls the protected method, it would not be possible to call that method without the __call() "trick". So if #24 is preferred, the patch needs at least to be updated and addConditions() changed to a public method.

Berdir, you're method is much more elegant, since it removes the need to monkey around with creating a "correct" subquery for the count query. Plus it still allows users to implement a custom count query, since a call to setCountQuery could still be made.

#753084: Extenders and subqueries don't play nice is not totally unrelated to this issue, but IMO if that issue is resolved, we should still use the countQuery implementation from #35, since a lot of the query related code in SearchQuery::execute isn't needed for the countQuery.

The patch in #35 looks great and fixes the pager bug for phrase searching.

Ahhh, that looks much better! :D jhodgdon, are you willing to sign off on this approach?

It has my official stamp of approval too.

This looks solid -- it is an improvement over previous patches. Great job all -- committed to CVS HEAD. Thanks.