Advanced search form has usability issues

I just ran into this again today.

My suggestion would be to fix the node_search_form_alter() function in node.module() so that it addresses these issues, except I'm not sure #3 is so terrible if the other items are addressed.

I think this is actually a bug. If you use the advanced search form, the search form you see with the search results should look like what you submitted.

It shouldn't actually be all that hard to fix either.

Hmmm...

Looking closer at the "advanced" search form, there are some problems with being able to populate the form values. The main problem is that in "advanced" search, you can enter or, phrase, or negative keywords, which are then added to the main search query like:

other keywords already present OR orword1 OR orword2 -negative1 -negative2 "phrase goes here"

The problem is that if those are put into the "advanced" search form, then you also need to take them out of the main search keywords box. I'm not sure if that is a great idea. Maybe it's OK if the advanced box is expanded so you can at least see what your search query was...

Thoughts?

I'm going to look into this today, with the goal of either making a patch or shoving it off to D8.

Note: I found a WTF in here. node.module processes taxonomy term filtering from advanced search, but it doesn't currently display a taxonomy term filter in the advanced search UI. I am adding some comments to the code so that this is at least a known factor in the state of the code. A contrib module could presumably add selected vocabularies to the advanced search form, and node.module would process them.

Here is a patch, which appears to be working well on my test box.

Since I don't have -up in my patching setup (Eclipse, sigh), I will just note that the first hunk is a code comment added to node_search_execute() (see #4 above), the last hunk is a code comment added to node_search_validate() (#4 again).

The rest is all added to the node_search_form_alter() function, which is where the advanced search form is built. This patch:
- Figures out which options/entries were made in the advanced search form to build the search keywords query string.
- Populates these as the current/default values for the advanced search form.
- Removes them from the keywords query string, so they don't appear twice
- Expands the advanced search form if anything was put in there, so people can see what search they did.

Note: we should also fix #987210: Language-specific searches should include language neutral content, which has implications for advanced searching.

whoops, forgot the patch.

This looks like a significant change, though I see the benefit.

Minor nit - aside from the failing test, we can use the faster/simpler str_replace() instead of preg_replace() since we have the full match.

It is definitely a significant change to the UI, and I think it's a significant UI improvement. The current state of the UI is a big WTF to a non-sophisticated user. They try an advanced search, and they get back their search encoded in a non-friendly way in the search keywords box, so they can't easily modify their search and try again. With this fix, they get back what they put in, so they can see what they are doing.

Good point about the str_replace, and there seems also to be a test failure that is real (a test that was apparently looking at the search query string shown on the screen).

re #8 - I guess we can use str_replace for the cases where preg_match_all() == 1 is tested. For the other cases, I don't think a loop over $matches[0] and using str_replace there would be more efficient than preg_replace? At least I think it wouldn't save much, and I would think the compiled preg_replace would be faster than a PHP loop. Not sure.

Given that this means one place would need to stay as preg_replace and two would change to str_replace, I am inclined to leave them all as preg_replace to make the code more consistent and easier to scan. If I used str_replace, I think I'd need to put in an explanation like // We verified there is only one match, and we have the whole matched string, so str_replace is more efficient. Thoughts?

Anyway, here's a new patch, with the test fixed.

I've also attached a screen shot of what this looks like, after selecting a bunch of stuff in the advanced search form and submitting. The advanced search form comes up expanded and filled in, just as you had it filled in when you submitted. Which is what a user would expect.

The second screen shot is what it looks like if you do the same advanced search without the patch (I've used Firebug to widen the search keywords form so you can see the full effect). The advanced search form comes up collapsed, and if you expand it, you'll see that nothing is filled in. Which is very confusing UI.

#6: 278958.patch queued for re-testing.

Need to fix in 8 first, or are we assuming we are going to eliminate the advanced search form in 8?

I don't see any work happening yet on getting a new search module for d8, so I think we should mark this d8 for now at least.

Just throwing it out there, but maybe the advanced search form should be eliminated from core all together and handled in Contrib? I honestly have never seen anyone use the advanced search form, and often times modules such as Views are leveraged when advanced search like functionality is needed.

I'm not working on search any more, sorry (unfollowing too).

:-( Thanks for all of your invaluable contributions up to this point, though!

#10: 278958-10.patch queued for re-testing.

Retesting against latest HEAD since it has been over a year.

I think we should postpone this on #2083717: Convert Search Results to Views (or close it as a duplicate or even make the other issue a META and this one one of its sub-issues).

Need to look at this in 8 and see what's happening and how to fix it, if it is actually broken in 8?

It does still need work in D8, especially in not having both the regular search and advanced search visible at the same time.

Yes, it's still an issue in D8 and still should be fixed.

I am looking into this.

Here's a first pass at least. It does the following:
- If the user submitted the Advanced form, it opens up the form.
- It takes the information from the keywords string and puts it into the Advanced form.

I am not sure it is absolutely perfect, but I think it works. Let's see if it breaks any of the existing tests, and I'd still need to make a test for it.

No interdiff. Things have changed 'slightly' in 4 years and I started over.

Actually setting back to needs work, since I forgot I need to write some tests for this. Still, if someone wants to (a) test it and (b) review the code/approach, that might be useful.

The trouble I had with the patch in #25 was that I kept entering keywords in the "Containing any of the keywords" just gets them plunked into "Enter your keywords" after hitting submit.

It doesn't make sense for me that it doesn't stay where you are expecting it to be.

I actually preferred the more cryptic combination of searches that was in the "Enter your keywords" as it shows how you the more complex searches you can do in a single line.

Great that the Advanced search remains open and that the other fields come pre-populated with what you entered though. Good step forward.

Thanks for testing! So, the way it's supposed to work is the "any" keywords become the "OR" terms, so ... anyway, things shouldn't be moving back and forth. I'll have to check on this, maybe there's something wrong.

Regarding liking the cryptic format it does without this patch, well I think you're alone on that. :)

That said, if someone prefers that on a site, they can definitely just turn off the Advanced form though, and you are free to use the unique Drupal-specific cryptic format... which I think may also be documented in the Search Help now too.

Anyway, the way this patch is supposed to work, if you entered something in Advanced it opens the form and tries to put the stuff back in Advanced, but if you used the basic field it doesn't try to extract it to Advanced. And it should be consistent with what you did. I need to write some tests to make sure that is true, and I'll do some more manual testing as well.

RE #28, I tested this... What happens is that if you only put 1 word in the "Any" box, you aren't supplying alternatives, so it ends up in the main search box. If you want to have alternatives, you need to put at least two words into the "Any" box.

So for instance if in the main search box you have "cat", and then in Any you put "dog", you're asking for "Page contains both cat and dog" so the query becomes "cat dog" in the main search box.

But if you have "cat" in main and "dog fish" in Any, your query is "Page contains cat, and also either dog or fish". In this case, you get "at" in main and "dog fish" in the Any box, and the page title is "Search for cat dog OR fish".

Also by the way you do get to see what the "cryptic" query would be without the parsing out -- it is in the page title. ;)

Anyway, so I think the form is working correctly. If you still have problems, please provide details on the steps to reproduce so I can tell what you're seeing, but in my tests it is working right where it can.

So now I'm going to write some tests and upload a new patch. Finally. ;)

OK here's a new patch with a test. The interdiff is the test-only patch (I didn't make any changes to the previous patch's code, only added the test). Test passes locally.

Also updating the issue summary with template and beta eval.

Patches passed/failed as expected. A review would be lovely.

The Containing any of the words is still confusing. It does get transferred to Enter your keywords but I can't get the value I type to stick in Containing any of the words after submitting the form.

If you want to have alternatives, you need to put at least two words into the "Any" box.

1 entry is logically a valid entry for any. Containing none of the words works fine for just one word.

Ultimately though, if you enter a word in Containing any of the words you expect it to be there after you hit submit.

That's my 2c.

The problem is that there is no difference between "containing any of the words" with just 1 word, and typing that 1 word into the main search box.

Basically the "any" is an OR box. The main box is AND. But if you have only one word, there is no difference between AND and OR. So that is one UI flub: if you only type one word in the "any" box, it gets transferred to the main box. I really cannot do anything about that without a MAJOR rearchitecture and it is semantically correct for the search, even though I agree it could be confusing. It is way less confusing than it used to be when everything vanished, however!

I also disagree that 1 word is semantically OK for "any", or that it is semantically distinguished from typing that one word in the main search box. If you only type one word, it is not "any of these words".

Looks like a nice addition to fill the form, and definite usability improvement.

Looks like it's not yet done, considering #36/#37/#38?

Well, the two maintainers of the Search module think it's done. It's a vast improvement in usability from the existing Search module, and probably all that we'll have time to do before 8.0.x.

I agree - given that we are parsing the keywords back out of the single query param, it's not possible to distinguish that a single word was entered into one of the other boxes. i think having the search work is probably better than an annoying form error for this case.

Oh, sorry, @jhodgdon — I misread your comment!

1. I agree that this change looks like a nice usability improvement and has a test - nice.

2. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
   @@ -490,38 +490,53 @@ public function indexStatus() {
   +    $used_advanced = (isset($parameters['advanced']) && $parameters['advanced'] == 'T');
   

   Unnecessary wrapping in ()

3. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
   @@ -490,38 +490,53 @@ public function indexStatus() {
   +    $defaults = ($used_advanced ? $this->parseAdvancedDefaults(isset($parameters['f']) ? $parameters['f'] : array(), $keys) : array('keys' => $keys));
   
   @@ -621,11 +646,70 @@ public function buildSearchUrlQuery(FormStateInterface $form_state) {
   +    if (preg_match('/ "([^"]+)" /', $keys, $matches)) {
   

   This ternary should be expanded as it is hard to read.

4. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
   @@ -621,11 +646,70 @@ public function buildSearchUrlQuery(FormStateInterface $form_state) {
   +    // Record that the person used the advanced search form, if they did.
   +    if ($advanced) {
   +      $query['advanced'] = 'T';
   +    }
   

   How about about changing this to $query['form'] = ADVANCED_FORM and making ADVANCED_FORM a class constant - which is equal to advanced. T is pretty cryptic - I guess it stands for TRUE.

how about this?

The change looks good to me, and the tests still pass. I think all of @alexpott's concerns were addressed. Thanks!

uh, bot sporadic fail? re-uploading patch.

Committed 0623c09 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

diff --git a/core/modules/search/src/Tests/SearchAdvancedSearchFormTest.php b/core/modules/search/src/Tests/SearchAdvancedSearchFormTest.php
index 3650a93..0905589 100644
--- a/core/modules/search/src/Tests/SearchAdvancedSearchFormTest.php
+++ b/core/modules/search/src/Tests/SearchAdvancedSearchFormTest.php
@@ -82,9 +82,9 @@ function testFormRefill() {
     );
     $this->drupalPostForm('search/node', $edit, t('Advanced search'));
 
-    // Test that the encoded query appears in the page title. Only test the
-    // part not including the quote, because assertText() cannot seem to find
-    // the quote marks successfully.
+    // Test that the encoded query appears in the page title. Only test the part
+    // not including the quote, because assertText() cannot seem to find the
+    // quote marks successfully.
     $this->assertText('Search for cat dog OR gerbil -fish -snake');
 
     // Verify that all of the form fields are filled out.
@@ -99,9 +99,9 @@ function testFormRefill() {
       }
     }
 
-    // Now test by submitting the or/not part of the query in the main
-    // search box, and verify that the advanced form is not filled out.
-    // (It shouldn't be filled out unless you submit values in those fields.)
+    // Now test by submitting the or/not part of the query in the main search
+    // box, and verify that the advanced form is not filled out. (It shouldn't
+    // be filled out unless you submit values in those fields.)
     $edit2 = array('keys' => 'cat dog OR gerbil -fish -snake');
     $this->drupalPostForm('search/node', $edit2, t('Advanced search'));
     $this->assertText('Search for cat dog OR gerbil -fish -snake');

re-flowed comments to the 80 character width on commit.