search_box_form_submit() improperly calls form_set_error()

Agreed with the major priority. Form errors are an important accessibility issue that I hope we can improve in Drupal 8.

I'm assigning backports, but I'm not so sure if really needs to.

I was trying to play along with this, but I'm not sure where to go. There are 2 calls: the first is to detect whether the search field is empty, and a second one aims to check if any search module is enabled. The second one can be moved to a validate, because at this time we won't have any redirect, so we don't need to execute anything else.

The problem lies with the first, as noted in #1, which aims to fire an error while redirecting at the same time. This patch moves the second only.

For the purpose of demonstrating the test mentioned in the description, this patch moves both occurences to a validate function.

This is a possible solution which introduces a new functionality on form_state processing:

$form_state['error_redirect'] - boolean which indicates wether form must redirect even on validation fail. Just to make it clear, this doesn't mean submit handlers are executed, only drupal_redirect_form().

I've concluded that the only clean solution to this (redirect even on form errors) is by introducing this feature on form.inc

Adding the tag, it is a very small change, so I guess it's still back-portable.

The failing test is basically comparing the url with a trailing slash against the current url (without one). I don't know how this got introduced, but this simple change on the test helps fixing it.

Forgot to rebase before generating patch.

Chx believes it's better to drop this behavior and do not redirect if search query is empty. That will make things easier, we can very well eliminate the submit handler without adding any API change.

This feature was introduced in #1789768: search_box_form_submit() improperly calls form_set_error()

This is a patch to simply disregard this whole logic discussed in the past on the basis that someone believe it is stupid to redirect when there are form errors.

By the way, the patch in #11 only failed because I forgot to remove the $form['#submit'] handler when removing the function.

I just want to verify what needs to be done to test this patch.
1) can one still use the search box (I'm assuming both from /search/node & the default search block.
2) Check behavior with null values.

What else do we need to do?

Why is the code associated this being removed:
// Confirm that the user is redirected to the search page, when form is submitted empty.

I'm guessing it's because we're sending them to a more direct function so it isn't needed.

@mgifford There are 2 approaches:

• The patch in #11 keeps the same behavior when an empty query is submitted, which is, redirecting to search/node. To do that with proper use of form_set_error() it introduces an API feature
• The patch in #14 which simply removes the functionality of redirecting. That's why it removes the test, because the test won't be needed in that case.

Personally, I don't like the last approach. It was suggested by chx so to keep a certain "purity" of never redirecting on form errors. This was his single argument to back-up this approach. As you can read, this feature was intentionally introduced in the past in this hackish way. I think it has a reason for that and I'm unconfortable simply removing a feature that was previously discussed without a true motivation for it.

This can be fixed quite easily.

Attached patch leverages the new #742344: Allow forms to set custom validation error messages on required fields

I don't think we can or want to backport this, so removing the tags.

sun, this is not what this issue is about.

bleen18, my patch in #14 does it in a more simple way, why disregard it with a more complex way? Also, did you read #17?

1. Apparently, #18 revealed a bogus expectation in LanguageConfigurationTest, which wrongly assumes that the language_negotiation_configure_url_form() would not redirect after submitting the form when there are no validation errors.

   The test passes in HEAD currently, because $form_state['redirect'] is defined in the form constructor and gets lots upon form processing (or rather, reset to NULL, which signifies no redirect to drupal_redirect_form()).

2. I carefully read and reviewed the issue #292565: Regression: Can't set /user to 403 page (was: login destination is broken) that is being referenced in the search_box_form() inline comments, so as to make sure that the changes here are in line with original expectations for the search block form.

   Fact is, this weird hack of throwing validation errors in a submit handler only exists because Form API was not able to support the situation we are facing with search block form. Today, the situation looks very different, since Form API has been vastly improved, and we're able to implement the originally intended behavior of this form in a proper way without any hacks.

   Fixing that inherently changes one expectation being asserted in the test:

   A form that does not validate will not redirect.

   The new code is not only much simpler, but also much more "standard" and in line with regular Form API processing.

   And of course, as long as we're using the HTML5 required attribute, it's the browser that will actually prevent the user from submitting the search block form in the first place. (see also #1797438: HTML5 validation is preventing form submit and not fully accessible) In turn, that results in the same behavior and experience for html5-enabled and older browsers: The search block form is validated on the page it appears.

3. Lastly, we should replace this entire form value futzing with a simple q= query parameter, using a simple #action, and HTTP GET as method. However, that is definitely left for a separate issue, since it requires further changes to Search module.

The issue was about the expected redirect (even with validation errors), not the fact that "required" was not being used, although this was one of the issues. The whole idea of not use "#required" was not to make a custom message, but to be able to redirect to 'search/node' even if the field is empty.

The former inline comment disagrees with you - it precisely talks about #required causing a confusing error message:

 function search_box_form_submit($form, &$form_state) {
...
-  // Check to see if the form was submitted empty.
-  // If it is empty, display an error message.
-  // (This method is used instead of setting #required to TRUE for this field
-  // because that results in a confusing error message.  It would say a plain
-  // "field is required" because the search keywords field has no title.
-  // The error message would also complain about a missing #title field.)
-  if ($form_state['values']['search_block_form'] == '') {
-    form_set_error('keys', t('Please enter some keywords.'));
-  }

That's completely obsolete due to #required_error, so that resolves the first bogus form_set_error() in a clean way.

The second form_set_error() is insanely bogus in the first place: If there are no search module implementations available, then it's completely irrational to output the search block form in the first place. This is cleanly resolved by setting a top-level #access on the entire form instead.

Thus, the new code gets as simple as it can currently get. Only aforementioned query parameter change to Search module would allow us to make it simpler than this, as it would eliminate the entire form processing for the search block form.

-    form_set_error('keys', t('Please enter some keywords.'));

How about we just kill this entirely? It's not an error condition really (in fact, if you're using Solr as a backend, searching with no keywords can be a completely legitimate thing to do and will return results).

For core search, it won't return results, but I think this could be handled by putting a message to that effect in the "no results" section underneath the form. Failing to enter a search term still shouldn't be treated as an error.

That would be a much simpler way to solve this issue, I think.

I've moved the $form_state['redirect'] fixes into #1251616: $form_state['redirect'] does not work in form constructors

re: #27 / @David_Rothstein:
I'd say that's something for apache_solr module to figure out. For the core search, it does not make sense to submit the search form without any keywords.

Additionally, as long as #1797438: HTML5 validation is preventing form submit and not fully accessible remains to be the case, the #required flag on the search input field will actually prevent the user from submitting the form on the client-side already. Therefore, the #required_error will only be visible to users on older browsers in the first place.

I still think that #24 is the proper fix to apply, which resolves all issues.

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

#24: drupal8.search-box-form-error.24.patch queued for re-testing.

Tagging for reroll.

working on the reroll

Rerolled

I haven't been following this issue until now, but I just took a look at this patch, and it seems to have a bunch of unrelated stuff in it?

@jhodgdon I think (if I followed the issue correctly) that the main chunk of the patch fixes the validation and submission issue described in the issue summary and the commenting related to it, as well as some tests.

Normally adding test functionality would belong in a separate patch, but since the changes are breaking existing tests (see comment #24) I think making test changes falls into the scope of this issue. (Feel free to correct me if I am wrong.)

At any rate, applying the patch failed so I am going to have a look at re-rolling this...

Changes like this are unrelated to this issue:

-    $this->assertEqual($this->getUrl(), url('admin/config/regional/language', array('absolute' => TRUE)), 'Correct page redirection.');
+    $this->assertUrl('admin/config/regional/language');

This change also lost information: it used to have a message saying "Correct page redirection" and now it doesn't.

There are about 10 similar changes in this patch, and I don't understand why they are there.

Re-rolled, and patch should now apply...

OK, just launched a sandbox with simplytest.me and the patch applied fine.

I am not great with tests, so I am not the best person to look at the reasoning behind this. If someone tells me what needs to change I'm happy to rewrite the patch though.

#39: drupal8.search-module.1789768-38.patch queued for re-testing.

Can you just remove the unrelated changes from this patch? The changes cited in #38 are completely irrelevant to this issue, as far as I can tell. If they are relevant, please explain why. I don't think any test changes should be needed for this issue.

Also, this change looks like it is way out of scope for an issue about a search form:

+++ b/core/includes/form.inc
@@ -601,7 +601,6 @@ function form_state_keys_no_cache() {
     'must_validate',
     'rebuild',
     'rebuild_info',
-    'redirect',

This change also lost information: it used to have a message saying "Correct page redirection" and now it doesn't.
There are about 10 similar changes in this patch, and I don't understand why they are there.

See #24.

this change looks like it is way out of scope for an issue about a search form

See #28.

Neither of those comments explain this to me and I do not have time to read all the comments on this issue. Please update the issue summary explaining the changes that are being made.

Oh I see there is already a "needs issue summary update" tag. It needs to be done so that reviewers who are coming in now can review the patch without reading through all the comments.

I do not have time to read all the comments on this issue.

Seriously? Subtracting the testbot and other noise, there are less than 30 comments here. Is it too much to ask to read through a few comments in order to get familiar with an issue?

I'm sorry, but I spend a ****lot**** of time every day reviewing patches. If a patch contains things that appear to be out of scope compared to the existing issue title and summary, it is, in fact, not too much to ask to have the issue summary updated. In fact, our Core Gates require that issues have summaries.

This issue summary doesn't say anything about why the tests were updated to use different test assertions, why form.inc was updated, or why the test assertion messages were removed when the assertion types were changed. The comments you referred me to did not clarify it for me either. So can someone please update the issue summary? Otherwise, as a reviewer, I think it is perfectly reasonable to say that the patch includes pieces that are out of scope with the stated purpose of this issue, as stated in its title and current summary.

On the other hand, since #30 the issue is marked to have the summary properly rewritten. In that case, if you can't spend time getting familiar to the issue, maybe it's better to just leave it until someone rewrites the summary.

Updated issue summary.

Updated issue summary.

Thank you to @pieterjd for updating the issue summary!

#39: drupal8.search-module.1789768-38.patch queued for re-testing.

Needs re-roll apparently...

Thanks for the summary update! Let's either get the additional changes noted in the summary documented there (which will be necessary to get the patch committed), or remove them from the next patch.

Updated issue summary.

Apparently this issue is also causing
#2017095: After searching for a too-short word, original search keyword is retained in subsequent searches
which has been marked as a duplicate, and is a real problem, not just a "we shouldn't be doing this" type of problem. I have updated the issue summary accordingly. We need to get this fixed. I would almost like to mark this critical...

Actually, given that the issue mentioned in #54 has been closed as a duplicate, we also need to add a test for that problem to this issue and verify that the fix to this issue fixes that problem as well. If not, we'll need to reopen that other issue. I'll add this task to the issue summary.

add note about the effect of this bug

So... I'm really really really confused about this issue.

The latest patch still has a form_set_error() call in submitForm(). So it doesn't even resolve the issue as stated here. It also has a bunch of stuff from form.inc and language tests in it, which makes no sense to me.

What is really going on here? Do we just need to start over? Can someone who has been working on this *****please***** update the issue summary?

add task to task list for duplicate issue needing test

add related issue

We need to redo how errors and warnings are handled in searching on #1366020: Overhaul SearchQuery; make search redirects use GET query params for keywords, which will hopefully take care of this problem, so I'm postponing this until that one is done.

This was completely taken care of on #1366020: Overhaul SearchQuery; make search redirects use GET query params for keywords.

Hi

I saw this issue still on drupal7

Won't fix for drupal7 ?

Note that for Drupal 7, #2530652: Cannot use search block again if error occured in previous search should address some (but perhaps not all) of this. Not sure if that means this issue should be reopened for Drupal 7.