When using "Containing the phrase" from "advanced search", on the result Drupal using Count query of default search (Containing any of the words) => Pager was wrong when using "Containing the phrase".
Here is the test case:
Download and install Drupal from CVS: http://apps.sanglt.com/drupal7/
Install devel and generate 5000 node
Index 5000 node.
Enable "Use search" and "Use advanced search" permission.
Here is the result with keyword: tego augue
Normal search: http://apps.sanglt.com/drupal7/search/node/tego%20augue => We have 234 page result => Correct
Using "Containing the phrase": http://apps.sanglt.com/drupal7/search/node/%22tego%20augue%22 => We only have 4 page (http://apps.sanglt.com/drupal7/search/node/%22tego%20augue%22?page=3) but the Pager display 234 page => Wrong
You can login to the test page with user and password is: admin-admin
Comment | File | Size | Author |
---|---|---|---|
#35 | 791270-searchquery2.patch | 5.78 KB | Berdir |
#24 | 791270-searchquery.patch | 4.47 KB | lotyrin |
#17 | searchquery-791270-17.patch | 5.15 KB | duellj |
#14 | searchquery-791270-14.patch | 5.16 KB | duellj |
#12 | searchquery-791270-12.patch | 5.13 KB | duellj |
Comments
Comment #1
duellj CreditAttribution: duellj commentedThis is actually a problem with the search module. Here's what's going on:
In search.api.php:hook_search_execute():
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():
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.
Comment #2
Shellingfox CreditAttribution: Shellingfox commentedThanks. It's work for me!
Comment #3
duellj CreditAttribution: duellj commentedThis needs a test.
Comment #4
Shellingfox CreditAttribution: Shellingfox commentedHere is the before i'm apply the patch: http://apps.sanglt.com/drupal7/search/node/%22tego%20augue%22?page=3
And here is after i'm apply the patch: http://apps.sanglt.com/drupal7patched/search/node/%22tego%20augue%22?page=3
Comment #5
jhodgdonWe should set it back to needs work until the tests are added.
Good catch though and the patch looks good!
Comment #6
jhodgdonIdea 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".
Comment #7
duellj CreditAttribution: duellj commentedI'm working on the test, thanks for the ideas jhodgdon!
Comment #8
duellj CreditAttribution: duellj commentedTest included.
Comment #10
jhodgdonHmmm...
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?
Comment #11
jhodgdonAlso, 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.
etc.
Comment #12
duellj CreditAttribution: duellj commentedI'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 :).
Comment #13
jhodgdonGreat! The code looks good, including the test.
A few text suggestions. See http://drupal.org/node/1354 for our doc standards...
a)
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)
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)
does is not found -> is not found
d) Same in this line:
Comment #14
duellj CreditAttribution: duellj commented:) Thanks, jhodgdon! Seems like docs would be hard to keep consistant. I've updated the patch per your suggestions.
Comment #16
jhodgdonWell, it looks like the tests don't work correctly. There's also a typo here:
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...
Comment #17
duellj CreditAttribution: duellj commentedHmm, 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?
Comment #19
jhodgdonThey are running for me on my test machine too, with your latest patch. Odd. I'm running HEAD from about 12 hours ago.
Comment #20
jhodgdon#17: searchquery-791270-17.patch queued for re-testing.
Comment #22
jhodgdonAh...
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...
Comment #23
duellj CreditAttribution: duellj commentedAhh, good find!
Comment #24
lotyrin CreditAttribution: lotyrin commentedThis 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.
Comment #26
lotyrin CreditAttribution: lotyrin commented#24: 791270-searchquery.patch queued for re-testing.
Looks like the test client malfunctioned.
Comment #28
jhodgdon#24: 791270-searchquery.patch queued for re-testing.
Comment #29
jhodgdonLooks 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!
Comment #30
BerdirI 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.
Comment #31
jhodgdonI 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.
Comment #32
webchickMmm. 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.
Comment #33
BerdirUhm, 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.
Comment #34
jhodgdonI 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.
Comment #35
BerdirHm. 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.
Comment #36
duellj CreditAttribution: duellj commentedBerdir, 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.
Comment #37
duellj CreditAttribution: duellj commentedThe patch in #35 looks great and fixes the pager bug for phrase searching.
Comment #38
webchickAhhh, that looks much better! :D jhodgdon, are you willing to sign off on this approach?
Comment #39
jhodgdonIt has my official stamp of approval too.
Comment #40
Dries CreditAttribution: Dries commentedThis looks solid -- it is an improvement over previous patches. Great job all -- committed to CVS HEAD. Thanks.