When searching via the main search_form, if a user simply clicks the "Search" button with nothing in the keywords textbox, the page refreshes with a "Please enter some keywords" message. However, if the user does the same from the search_block_form, they are simply sent to an empty main search_form with no message. Shouldn't both of these (identical) actions result in the same "Please enter some keywords" message being displayed?
The following patch is a very simple port of the search_form_submit() code to search_box_form_submit(). It doesn't use a "processed_keys" value which seems like an error, but in looking through the code I couldn't figure out exactly how that functioned. :-/
--- /nfs/home/D/dwong/search.module 2009-07-07 15:35:50.000000000 -0400
+++ search.module 2009-07-13 23:45:56.620410000 -0400
@@ -1081,6 +1081,12 @@ function search_box(&$form_state, $form_
* Process a block search form submission.
*/
function search_box_form_submit($form, &$form_state) {
+ $keys = $form_state['values']['search_block_form'];
+ if ($keys == '') {
+ form_set_error('keys', t('Please enter some keywords.'));
+ // Fall through to the drupal_goto() call.
+ }
+
$form_id = $form['form_id']['#value'];
$form_state['redirect'] = 'search/node/'. trim($form_state['values'][$form_id]);
Comment | File | Size | Author |
---|---|---|---|
#52 | search_from_block_with_no_keywords-518512-d6-52.patch | 969 bytes | Albert Volkman |
#51 | search_from_block_with_no_keywords-518512-51-d6.patch | 969 bytes | Albert Volkman |
#46 | 518512-46.patch | 2.29 KB | jhodgdon |
#43 | 518512-43.patch | 2.29 KB | jhodgdon |
#36 | 518512-fixurl.patch | 2.31 KB | jhodgdon |
Comments
Comment #1
jhodgdonThis is still a problem in Drupal 7, and should be fixed there first, then ported to Drupal 6.
Comment #2
jhodgdonAnd I think it's a bug not a feature request.
Comment #3
lelutin CreditAttribution: lelutin commentedI'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.
Comment #4
lelutin CreditAttribution: lelutin commentedoops. resending correctly this time
I can't confirm anything about that "processed_keys" thing that dwong127 mentioned, please someone advice.
Comment #6
lelutin CreditAttribution: lelutin commented#4: 0001-Empty-search-from-block-doesn-t-display-error.diff queued for re-testing.
Comment #7
jhodgdonThis 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.
Comment #8
douggreen CreditAttribution: douggreen commentedRTBC, but slightly streamlined patch
Comment #9
jhodgdonOK, RTBC assuming the test bot agrees.
Comment #10
webchickHm. Is there a reason we don't just set the field to required?
Comment #11
jhodgdonDuh. I'll look into that.
Comment #12
jhodgdonI'm looking into this one.
Comment #13
jhodgdonWell. 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...
Comment #14
jhodgdonI guess it was at RTBC before webchick asked that last question.
Comment #15
nadavoid CreditAttribution: nadavoid commentedHere's a re-rolled patch with a test added. The added test is:
Comment #16
dwightaspinwall CreditAttribution: dwightaspinwall commentedReviewed #15 and tested by community. Ready to be committed.
Comment #17
webchickLet'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! :)
Comment #18
nadavoid CreditAttribution: nadavoid commentedThanks 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.
Comment #19
nadavoid CreditAttribution: nadavoid commentedI 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!
Comment #20
nadavoid CreditAttribution: nadavoid commentedupdating status to "needs review"
Comment #21
hefox CreditAttribution: hefox commentedjust visually looked at it but "because the search keywords field has not title." not => no or not => does not have
Comment #22
nadavoid CreditAttribution: nadavoid commentedFixed a silly grammar mistake. Thanks for spotting that hefox.
Comment #24
jhodgdonThose test failures look real...
Comment #25
nadavoid CreditAttribution: nadavoid commentedYep, 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!
Comment #26
jhodgdonI 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?
Comment #27
nadavoid CreditAttribution: nadavoid commentedThanks 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!
Comment #28
nadavoid CreditAttribution: nadavoid commentedupdating status to "needs review"
Comment #29
jhodgdonYou left two commented-out code lines in like this:
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.
Comment #30
nadavoid CreditAttribution: nadavoid commentedDoh! 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.
Comment #31
marcingy CreditAttribution: marcingy commented#29: 518512-fixcomments.patch queued for re-testing.
Comment #33
jhodgdonThe reason that this test is failing is that the redirects have changed:
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.
Comment #35
jhodgdonWell, guess that didn't quite do it...
Comment #36
jhodgdonAh. The redirect url is url('search') not url('search/'). This should do it.
Comment #37
jhodgdon#36: 518512-fixurl.patch queued for re-testing.
Comment #38
jhodgdon#36: 518512-fixurl.patch queued for re-testing.
Comment #39
jhodgdonAdding 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?
Comment #40
jhodgdon#36: 518512-fixurl.patch queued for re-testing.
Comment #42
jhodgdonHmmm. "Redirected to correct URL" in the search block test is failing. Need to investigate...
Comment #43
jhodgdonA 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.
Comment #45
jhodgdonHuh, 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).
Comment #46
jhodgdonArgh. I see it: it's redirecting to search/node/ not search/node
This should do the trick.
Comment #47
jhodgdon#46: 518512-46.patch queued for re-testing.
Comment #48
jhodgdon#46: 518512-46.patch queued for re-testing.
Comment #49
pwolanin CreditAttribution: pwolanin commentedThis makes sense for the simple search block to behave the same way when there are no keys.
Comment #50
webchickWhile 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.
Comment #51
Albert Volkman CreditAttribution: Albert Volkman commentedD6 backport.
Comment #52
Albert Volkman CreditAttribution: Albert Volkman commentedPatch name fail. Reuploading.
Comment #53
jhodgdonThis patch seems simple and straightforward enough that it could be committed to 6.x.
Comment #54
pwolanin CreditAttribution: pwolanin commentedWhy is this in the submit function and not validate?
Comment #55
jhodgdonpwolanin: 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.
Comment #56
pwolanin CreditAttribution: pwolanin commentedIs this really the correct behavior? Some modules might want to execute a search with no keywords as long as other "conditions" are present
Comment #57
pwolanin CreditAttribution: pwolanin commentedrelated #1126688: Allow users to use advanced search without keywords entered
Comment #58
pwolanin CreditAttribution: pwolanin commentedComment #59
jhodgdonThe 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.
Comment #61
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, even though this is a simple three-line patch.