Search from block with no keywords doesn't display the "Please enter some keywords" message

This is still a problem in Drupal 7, and should be fixed there first, then ported to Drupal 6.

And I think it's a bug not a feature request.

I've rerolled dwong127's modification upon HEAD and it seems to be working for me. Sending the patch so that it can be tested by testbot.

oops. resending correctly this time

I can't confirm anything about that "processed_keys" thing that dwong127 mentioned, please someone advice.

#4: 0001-Empty-search-from-block-doesn-t-display-error.diff queued for re-testing.

This patch needed a small modification. The comment says it falls through to drupal_goto(), but there is no drupal_goto() call (it is using form redirect).

It also probably needs a test, assuming the test bot thinks it's OK.

RTBC, but slightly streamlined patch

OK, RTBC assuming the test bot agrees.

Hm. Is there a reason we don't just set the field to required?

Duh. I'll look into that.

I'm looking into this one.

Well. I have an answer as to why. I set the field to #required => TRUE for both the form in the search block and the form on the search page.

From the search block, I thought the error message was confusing. For one thing, it was in the main body of the page, not very near the search box (see screen shot). For another thing, since the search keywords field has no title, the message says "field is required" instead of a more helpful "please enter some keywords". And there was also a validation message complaining about a missing #title from the default form validation (see screen shot).

So I guess it could be moved into a custom validation function instead of using #required, but I don't really see why this would improve the code much over the current code?

Tentatively suggesting we commit the last patch instead...

I guess it was at RTBC before webchick asked that last question.

Here's a re-rolled patch with a test added. The added test is:

    // Test an empty search via the block form, from the front page.
    $terms = array('search_block_form' => '');
    $this->drupalPost('node', $terms, t('Search'));
    $this->assertText('Please enter some keywords');

Reviewed #15 and tested by community. Ready to be committed.

Let's also maybe get a comment to the effect of jhodgdon's in #13. I forsee us not being the only people to wonder about this.

Also, can we add one more assertion to that test to check and see if you're redirected properly to the search page after submitting the block form? Otherwise, looks good! :)

Thanks for the feedback webchick. I will make those changes and submit a new patch. I will need a day or so since I'm travelling home from drupalcon at the moment.

I added some comments reflecting #13, and added two new assertions to confirm correct redirection. One assertion is for when the search block form has something in it, and one for when it's submitted empty. Please let me know if this patch needs any other attention. This is my first core patch, so I want to be sure this is good and proper, and that I get started off on the right foot. Thanks!

updating status to "needs review"

just visually looked at it but "because the search keywords field has not title." not => no or not => does not have

Fixed a silly grammar mistake. Thanks for spotting that hefox.

Those test failures look real...

Yep, it's actually an error in the two tests I added. They assume that Clean URLs are enabled. This evening I'll look for other examples of confirming correct URL redirection, and get my tests corrected. If you know of any I should look at for reference, please point them out to me. Thanks!

Assigning to me because I want to finish it up. Thank you for your patience!

I searched through the code base for getUrl(), and it looks like quite a few tests are using it. For instance, in module/aggregator/aggregator.test, they compare getUrl() to url('whatever'), rather than using getAbsoluteURL('whatever'), so you might try that in your test?

Thanks for the suggestion. Using the core url() function is exactly what I needed to do, I believe. I'll let the testing bot help determine that. Let's see if I get a green light on this one. Thanks again for the help!

updating status to "needs review"

You left two commented-out code lines in like this:

+    // $this->assertEqual($this->getUrl(), $this->getAbsoluteUrl('search/node/' . $terms['search_block_form']));

Those need to be removed.

I've confirmed that the patch works (fixes the problem), and the test bot is also happy. The tests look reasonable to me.

So I just removed those two commented code lines from your version, rerolled, and it should be ready to go now.

Doh! The commented-out code. I'll be sure to make an extra pass visually next time I submit a patch to make sure that type of thing is removed.

#29: 518512-fixcomments.patch queued for re-testing.

The reason that this test is failing is that the redirects have changed:

+    // Test an empty search via the block form, from the front page.
+    $terms = array('search_block_form' => '');
+    $this->drupalPost('node', $terms, t('Search'));
+    $this->assertText('Please enter some keywords');
+
+    // Confirm that the user is redirected to the search page, when form is submitted empty.
+    $this->assertEqual(
+      $this->getUrl(),
+      url('search/node/', array('absolute' => TRUE)),
+      t('Redirected to correct url.')
+    );

A no-term search ends up at URL 'search' rather than 'search/node' now, so this last assertEqual is failing (it needs to be changed so it checks url('search', ...) instead. See #245103: Search page tabs not highlighting for more information on this change.

Well, guess that didn't quite do it...

Ah. The redirect url is url('search') not url('search/'). This should do it.

#36: 518512-fixurl.patch queued for re-testing.

#36: 518512-fixurl.patch queued for re-testing.

Adding tag. This is really a very small change - three lines of code, several of comments, and some additional tests. Could we possibly get it in?

#36: 518512-fixurl.patch queued for re-testing.

Hmmm. "Redirected to correct URL" in the search block test is failing. Need to investigate...

A committed patch from another issue changed the blank page URL to search/node rather than just search, which is why that test was failing.

Also the above patch would not apply.

So here's a redo.

Huh, that's strange. That patch passed that test on my test box. Will need to investigate (possibly by disabling clean URLs to make sure that's not the problem again).

Argh. I see it: it's redirecting to search/node/ not search/node

This should do the trick.

#46: 518512-46.patch queued for re-testing.

#46: 518512-46.patch queued for re-testing.

This makes sense for the simple search block to behave the same way when there are no keys.

While this patch does touch strings, it's re-using an existing one so shouldn't have any bearing on string freeze.

Committed to HEAD. Thanks!

Marking for backport to D6.

D6 backport.

Patch name fail. Reuploading.

This patch seems simple and straightforward enough that it could be committed to 6.x.

Why is this in the submit function and not validate?

pwolanin: The reason is because the validate with form_set_error would put the message in a weird spot, just as it would if the #required was used, as described in comments above.

Is this really the correct behavior? Some modules might want to execute a search with no keywords as long as other "conditions" are present

related #1126688: Allow users to use advanced search without keywords entered

The only thing this patch does is make it so that a search from the Search block works the same as a search from the Search page.

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, even though this is a simple three-line patch.