search_query_insert breaks down when the value of key is 0

I 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.

I 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.

Can 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.

Can you please answer my question so we can continue here?

From the code, it seems to allow options without arguments

You'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.

At this point, this should be fixed in Drupal 7 and then backported to Drupal 6, shouldn't it?

In 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?

How 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.

Gracious. We still need a D7 patch for this. I'll work on it, should have a patch in a day or two.

Here'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.

#10: 419388-withdocandtests.patch queued for re-testing.

I don't know why that won't apply. It seems to apply for me. Here's a reroll just in case.

Just 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.

Doe we have to add (issue) behind an issue link?

 @see http://drupal.org/node/419388 (issue)

What's your question? I'm not understanding...

I was wondering why we put "(issue)" behind the link. Is that necessary?

Well, 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?

Well I think I like your explanation :).

#13: 419388-reroll.patch queued for re-testing.

#13: 419388-reroll.patch queued for re-testing.

#13: 419388-reroll.patch queued for re-testing.

#13: 419388-reroll.patch queued for re-testing.

I'd favor the defualt $value to be NULL, and use isset() as the test instead of !== ''

Probably a good idea. Let's try this one...

whoops, how did that happen?

Is 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.")

Signaling testbot.

Yes, 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.

Committed to CVS HEAD. Thanks.

Can the patch in #1 now be applied to Drupal 6?

Forgot the tag

Yes. The proper way to do that is rather than tagging, change the version/status.

Well, 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).

This 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...

I 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

Oh 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.

I don't have time to work on search.module any more, sorry.

Talked 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.