Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Hello,
While debugging a problem for faceted search we came across a bug in the search code (i think)
function search_query_insert($keys, $option, $value = '') {
if (search_query_extract($keys, $option)) {
if $keys is "field_do_u_smoke:0" the function search_query_extract returns 0 which is a valid value for a key (we're using a cck number field checkbox/radio and the allowed values are 0|No and 1|Yes). When using the module cck_facets this function is invoked and the facets for key = 0 breaks down.
We've also seen a bug in cck_facets and here's our comment on it - http://drupal.org/node/400810#comment-1420620
If this is a valid bug i'd be glad to help fix it.
thx.
yashesh
Comment | File | Size | Author |
---|---|---|---|
#36 | 419388-trim.D6.patch | 3.12 KB | jcnventura |
#29 | 419388-trim.patch | 6.47 KB | Garrett Albright |
#27 | 419388-fixwhoops.patch | 6.02 KB | jhodgdon |
#25 | 419388-usenull.patch | 6.35 KB | jhodgdon |
#13 | 419388-reroll.patch | 5.84 KB | jhodgdon |
Comments
Comment #1
David Lesieur CreditAttribution: David Lesieur commentedI see no reason for not allowing a value of 0 in $keys, so I think this is a valid bug. Here's a patch that solves this.
Comment #2
jcnventura CreditAttribution: jcnventura commentedI have just reviewed this and I confirm that this solves the problem.
The patch is simple enough that by code inspection, one can see that it fixes the case of option:0 which would return 0 from search_query_extract() and fail the test even though there's a perfectly valid key there (0).
This code was dropped in D7, so it can't be backported from there.
This is also needed for #400810: Empty result set when search key value is 0.
Comment #3
Gábor HojtsyCan the return value of search_query_extract() be empty and still be valid? From the code, it seems to allow options without arguments, therefore the return value being empty is valid?!? Seems like !empty() would be just as good, otherwise, right? Looks like I need some more enlightening.
Comment #4
Gábor HojtsyCan you please answer my question so we can continue here?
Comment #5
David Lesieur CreditAttribution: David Lesieur commentedYou're right, it looks like the regular expression allows nothing after the colon (':'). Thus a string like 'mykey:' matches, but not 'mykey'. That does not look like a desirable behavior... I think I'd rather put a '+' instead of a '*' in that regex.
Comment #6
jhodgdonAt this point, this should be fixed in Drupal 7 and then backported to Drupal 6, shouldn't it?
Comment #7
jhodgdonIn Drupal 7, the equivalent functions are search_expression_extract() and search_expression_insert() by the way. And they look exactly the same as the functions in D6, except the names.
Gábor: to answer your question, it seems to me that if search_expression/query_extract()'s regex finds the query key, then search_query/expression_insert() should remove it from the query, whether it was:
key:
key:0
key:something_else
It also seems to me that search_query/expression_insert() should then allow inserting
key:
key:0
key:something_else
But the problem with allowing insert of key: is that currently you pass $value='' to indicate "clear this out and don't put anything else in there".
The best course of action would be to document that you can pass in a value of '' to clear, and ' ' to set it to an empty value, I think?
Comment #8
jhodgdonHow about having search_query/expression_extract() return FALSE if the key wasn't found, and the value (which could be '' or 0) if it is found? That small change in behavior might clear things up.
Comment #9
jhodgdonGracious. We still need a D7 patch for this. I'll work on it, should have a patch in a day or two.
Comment #10
jhodgdonHere's a patch.
- New behavior allows you to insert values of 0, ' ', '0', etc. as legal values, although on extracting the option, ' ' is retrieved as an empty string.
- Documents that on insert, you should pass '' if you want to clear the option without adding a new one, and generally makes the documentation of both functions explicit. (Note that this patch incorporates #744002: search_query_insert and related functions doc is unclear, because obviously better documentation was needed, so if it is accepted, the other one should be marked as duplicate.)
- Adds tests to verify that various cases work.
I think that these functions are now as consistent and logical as we can get... And if this is accepted, it should also be ported back to Drupal 6.
Comment #11
jhodgdon#10: 419388-withdocandtests.patch queued for re-testing.
Comment #13
jhodgdonI don't know why that won't apply. It seems to apply for me. Here's a reroll just in case.
Comment #14
jhodgdonJust as a note: If this patch is committed, we should mark this for porting to D6, and close #744002: search_query_insert and related functions doc is unclear as a duplicate, since the doc here incorporates what's needed on that issue.
Comment #15
aspilicious CreditAttribution: aspilicious commentedDoe we have to add (issue) behind an issue link?
Comment #16
jhodgdonWhat's your question? I'm not understanding...
Comment #17
aspilicious CreditAttribution: aspilicious commentedI was wondering why we put "(issue)" behind the link. Is that necessary?
Comment #18
jhodgdonWell, I think it's useful in a test to link to the issue it is testing, and I think it's clearer if the link points out it's to an issue, rather than to a Handbook page, etc.. What do you think?
Comment #19
aspilicious CreditAttribution: aspilicious commentedWell I think I like your explanation :).
Comment #20
jhodgdon#13: 419388-reroll.patch queued for re-testing.
Comment #21
jhodgdon#13: 419388-reroll.patch queued for re-testing.
Comment #22
jhodgdon#13: 419388-reroll.patch queued for re-testing.
Comment #23
jhodgdon#13: 419388-reroll.patch queued for re-testing.
Comment #24
pwolanin CreditAttribution: pwolanin commentedI'd favor the defualt $value to be NULL, and use isset() as the test instead of !== ''
Comment #25
jhodgdonProbably a good idea. Let's try this one...
Comment #27
jhodgdonwhoops, how did that happen?
Comment #29
Garrett Albright CreditAttribution: Garrett Albright commentedIs this what we're trying to do? Note the addition of trim() in search_expression_insert() and the resultant changes to the tests. (This is assuming that you're not counting a string comprised of a single space character as an "empty string.")
Comment #30
Garrett Albright CreditAttribution: Garrett Albright commentedSignaling testbot.
Comment #31
jhodgdonYes, your tests should work where mine failed -- that is what I meant. Thanks!
Assuming the test bot agrees, I think we've addressed pwolanin's concerns, and I am assuming at this point that both I am Garrett Albright think this patch is OK, so let's get it in.
Comment #32
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #33
jcnventura CreditAttribution: jcnventura commentedCan the patch in #1 now be applied to Drupal 6?
Comment #34
jcnventura CreditAttribution: jcnventura commentedForgot the tag
Comment #35
jhodgdonYes. The proper way to do that is rather than tagging, change the version/status.
Comment #36
jcnventura CreditAttribution: jcnventura commentedWell, instead of having to re-apply it again when Drupal 6.20 comes again, maybe it's easier if I just port it myself..
This patch is basically the same as #29, keeping the D6 function names in order not to break the API, but renaming the $keys argument to $expression (mostly because the added documentation would be confusing without this rename).
Comment #37
jhodgdonThis patch looks correct to me. Someone needs to test it out thoroughly though, before we can set it to "Reviewed and Tested" (RTBC). In D6 we don't have comprehensive tests like in D7...
The reason is that this is a code as well as doc change. The search_query_extract function should behave exactly the same, but the insert function behavior is changing, in that (a) old options are always removed, and (b) new options are then inserted for any non-null value.
Previously old options were removed only if search_query_extract evaluated to FALSE, and new ones were added only if the value evaluated to TRUE.
So this is a somewhat important change, and could break contributed modules, as well as potentially the advanced content search in core. And it shouldn't be taken lightly...
Comment #38
jcnventura CreditAttribution: jcnventura commentedI can always remake the patch keeping the call to search_query_extract, and merging it #1 so that it doesn't choke on "$option:0".
That patch, which I tested 9 months ago, always did that replacement as well unless the return was null.. That could only happen in case the expression was "$option:". I can't even trigger that condition in my problem case, as that would mean searching for a field with empty content.
I think that the code is better without that call to search_query_extract.
João
Comment #39
jhodgdonOh I agree the code is better -- that is why we fixed it that way in Drupal 7. But we have to be more careful about API changes in Drupal 6, that's all I'm saying.
Comment #40
jhodgdonI don't have time to work on search.module any more, sorry.
Comment #42
jhodgdonTalked with Gabor (the Drupal 6 branch maintainer) and D6 issues are really not being committed unless they're really essential -- we really don't have a test system for Drupal 6 and it's too dangerous. So... putting this back to D7 / fixed.