Language selection in advanced search filters broken

Good catch, sounds like a bug we should fix.

Attached patch seems to fix the problem. Problem lies in node.module, though, so maybe the "Component" should be changed.

But another thing: is it just me, or does the language box break searches in general? When it shows up, I can't use the advanced search form as for any search I enter, "language:" is appended at the end and the search yields no results. If I select one or both languages, it still yields no results (but maybe only because my node are all LANGUAGE_NONE). The only way for search via the regular search form with the languages item showing up, is searching and then deleting the "language%3A" from the URL …
Also, when I check any of the checkboxes, they are blank again after submitting, this should be corrected, too (but probably in another issue).

That's the right behavior... if your nodes are all marked LANGUAGE_NONE, then they won't show up in a language-based search, which restricts nodes to a particular language.

Having the advanced search boxes understand what search was just done is a separate issue that's been filed... let's see...
#278958: Advanced search form has usability issues

Looking at the patch, it looks like the right fix, but I think a test would be good? Things to test:
- With only English, verify no language options are shown.
- Then enable one non-English language and verify both are shown.
- Then disable English and verify that no language choices are shown.

Thoughts?

That's the right behavior... if your nodes are all marked LANGUAGE_NONE, then they won't show up in a language-based search, which restricts nodes to a particular language.

Then the bug is that I can't NOT restrict it to a particular language – as written, even if I just submit the search form with nothing but the keywords, "language:" is added (e.g., "foo bar language:").
This seems to restrict the search to no language at all – not German, not English and also not LANGUAGE_NONE. To find all English and German results, I manually have to check both checkboxes – and there is no way whatsoever to search for content with LANGUAGE_NONE, too – at least not from the advanced search form, only with the search block.

So, to further clarify: Suppose I have three nodes – "de", "en" and "und" – with their language property the same as their title and all containing "foobar" in their body.

• I type "foobar" into the search block and submit: The advanced search page is displayed, listing all three nodes as results.
• I hit "submit" again, on the page search form (nothing else done, not even expanded "Advanced search" fieldset): The advanced search page is displayed, with "foobar language:" as the keywords and no results.
• I expand the "Advanced search" fieldset and mark both languages, then submit again: The advanced search page is displayed, with "foobar language:en,de" as the keywords and "de" and "en" as the results.
• There is no way to do a search not using the search block that displays "und" as the result (besides deactivating the Locale module or deleting all non-English languages).

Regarding the test: Yes, looks like a good approach.

OK, that's a bug. I totally agree that if you don't choose a language, it shouldn't restrict you to a language, and it definitely shouldn't restrict you to no languages.

Should we file that as a separate issue to this one though?

And you are right, there should probably be an option for "Language neutral" (which is the UI text for "und"/LANGUAGE_NONE).

This patch still needs a test, and another option for "Language neutral".

This ought to be more than normal if it breaks search when any 2nd language is enabled.

I think that you cannot search for language neutral is a different bug. Seems like this issue is caused by two things though still. First, disabled languages show up in the language list, second if the language list is present, 'language:' is submitted, and therefore search happens for 'no language'.

The patch fixes the first, but the second still need attention. Once you install two more languages, the same bug will reappear. Unless you select a language, it will consider you look for "posts with no language", however, it should default to "I don't care about language'.

Here is an updated patch to tackle both problems that I think belong here:

1. Disabled languages show up in search (was handled the patch already)
2. Even if you select no language, the language filter is added (this is due to array_filter() applied after the condition decides the array has elements, but array_filter() empties it out - so I moved array_filter() around)

I think that there is a missing feature that you cannot search for language neutral posts is not to be handled here.

Where should we put the test? The test would depend on node, locale and search modules working together in a certain way, right? :)

Let's retitle since this is not just about enabled languages, but also about not selecting a language.

I would put the test in search.test. I think there are already some advanced search tests there, and some of them depend on other modules (such as comment), so it wouldn't be horrible to have a new test class that depends on locale.

The patch looks quite reasonable to me. A test or two would certainly be beneficial. :)

Sorry, cross-posted, setting title back

Ok, here are elaborate tests for both aspects of the bug: disabled languages and selecting no language. Just the tests to prove first that is indeed borked.

I filed a separate issue for the bug about not being able to search language-neutral content:
#987210: Language-specific searches should include language neutral content

Hmmm. Only one test failure in all of that? Something's probably not quite right with the tests?

Yup, we expect 1 failure for the two aspects of this bug respectively (in total 2 failures). The xpath check for the empty field does not really work though, since it cannot be used to check for emptyness. Instead I opted to check for the URL instead, which should stay the same if no language is selected. Now this should prove we have two fails.

Now we proved it has 2 fails, here is the patch to fix them. Please review.

Assuming that the test bot result in #23 is fine (and it should be, since it's been tested manually), I think this patch is ready to go in. The tests are correctly testing the issue, and the code to fix it is clear and looks exactly right to me.

Spoke too soon. Looks like the test failed.

I ran the test locally. The problem is that if you search with no keywords, you end up on the URL 'search/node/' not 'search/node'.

So I fixed the test. I also went back and verified that it fails (locally) without the node.module hunk of the patch (the URL in that case is the incorrect "search/node/language%3A" instead of "search/node"). And I tested that the patch works manually as well (i.e. went through the reproduce steps and it's all working fine).

New patch attached. Exactly the same as the previous patch, but for the trailing / on the one test case.

I'm +1 for RTBC on this one now, assuming the test bot agrees with my local test results.

Patch works for me, too, so RTBC as long as the test bot is happy now.
Thanks, Gábor, for fixing this!

Committed to CVS HEAD. Thanks.

Superb, thanks Jennifer for moving this through the finish line! Yay.