Updated: Comment #47
Problem/Motivation
Currently the search_box_form_submit calls the form_set_error function. Only validate functions should be calling form_set_error.
As a side effect of doing this, it apparently causes a search for a too-short word to put the form in a bad state, so that you can't search again afterwards. See #2017095: After searching for a too-short word, original search keyword is retained in subsequent searches. For this reason, it's a very major bug that we need to fix.
Proposed resolution
Proposed approaches from #17:
- 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.
- In #27 it is suggested that it isn't really an error condition if you look at a the same issue in Solr.
Remaining tasks
- Patch in #39 failed to run tests.
- Patch needs reroll.
- At the moment, there are changes in the tests, without any clear reasons #47. This needs further explanation by the original patcher, or those changes need to be removed from the patch.
- Since #2017095: After searching for a too-short word, original search keyword is retained in subsequent searches has been marked as a duplicate, we need a test for this bug which is allegedly caused by the same problem as here. We need a test-only patch that illustrates the problem, and then a patch with the fix here and the new test that shows the test passing.
Related Issues
#1932708: Empty search error gets cached by Akamai is a duplicate
Original report by @bleen18
This issue came up while working on #1493324: Inline form errors for accessibility and UX
Currently the search_box_form_submit calls the form_set_error function which is bad. Only validate functions should be calling form_set_error. It appears the reason it was done this way was to satisfy the requirement that the user is redirected if the search form is empty (there is a simpletest for this already)...
This is holding up an issue marked as "major" so I'm not certain if this should be marked as "major" ... please adjust accordingly.
Comment | File | Size | Author |
---|---|---|---|
#39 | drupal8.search-module.1789768-38.patch | 6.68 KB | yanniboi |
#35 | drupal8.search-module.1789768-35.patch | 6.67 KB | rgoodine |
#24 | drupal8.search-box-form-error.24.patch | 6.66 KB | sun |
#18 | drupal8.search-box-form-error.18.patch | 3.22 KB | sun |
#14 | 1789768-remove_form_set_error_on_submit.patch | 1.67 KB | franz |
Comments
Comment #1
mgiffordAgreed with the major priority. Form errors are an important accessibility issue that I hope we can improve in Drupal 8.
Comment #2
franzI'm assigning backports, but I'm not so sure if really needs to.
Comment #3
franzI 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.
Comment #4
franzFor the purpose of demonstrating the test mentioned in the description, this patch moves both occurences to a validate function.
Comment #6
franzThis 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
Comment #7
franzAdding the tag, it is a very small change, so I guess it's still back-portable.
Comment #9
franzThe 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.
Comment #11
franzForgot to rebase before generating patch.
Comment #12
franzChx 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()
Comment #14
franzThis 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.
Comment #15
franzBy the way, the patch in #11 only failed because I forgot to remove the $form['#submit'] handler when removing the function.
Comment #16
mgiffordI 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.
Comment #17
franz@mgifford There are 2 approaches:
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.
Comment #18
sunThis 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.
Comment #20
franzsun, this is not what this issue is about.
Comment #21
bleen CreditAttribution: bleen commentedfranz: can you give some details about your objection to sun's patch in #18
It seems to me that it he is offering a legitimate alternative to setting form errors in the form_validate validate function. (failing tests aside...)
Comment #22
franzbleen18, my patch in #14 does it in a more simple way, why disregard it with a more complex way? Also, did you read #17?
Comment #23
bleen CreditAttribution: bleen commented@franz: I did read #17 (and I personally favor the patch in #14). The patch sun submitted in #17 accomplishes the goal of this issue (albeit in a very different manner than your proposal) and I just want you to give a specific reason as to why you feel we should dismiss it. Your comment "this is not what this issue is about" needs clarification is all...
Comment #24
sunApparently, #18 revealed a bogus expectation in
LanguageConfigurationTest
, which wrongly assumes that thelanguage_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 toNULL
, which signifies no redirect todrupal_redirect_form()
).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.
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.Comment #25
franzThe 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.
Comment #26
sunThe former inline comment disagrees with you - it precisely talks about #required causing a confusing error message:
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.
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedHow 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.
Comment #28
sunI've moved the $form_state['redirect'] fixes into #1251616: $form_state['redirect'] does not work in form constructors
Comment #29
sunre: #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.
Comment #30
dcam CreditAttribution: dcam commentedhttp://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.
Comment #31
Berdir#24: drupal8.search-box-form-error.24.patch queued for re-testing.
Comment #33
star-szrTagging for reroll.
Comment #34
rgoodine CreditAttribution: rgoodine commentedworking on the reroll
Comment #35
rgoodine CreditAttribution: rgoodine commentedRerolled
Comment #36
jhodgdonI 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?
Comment #37
yanniboi CreditAttribution: yanniboi commented@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...
Comment #38
jhodgdonChanges like this are unrelated to this issue:
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.
Comment #39
yanniboi CreditAttribution: yanniboi commentedRe-rolled, and patch should now apply...
Comment #40
yanniboi CreditAttribution: yanniboi commentedOK, 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.
Comment #42
yanniboi CreditAttribution: yanniboi commented#39: drupal8.search-module.1789768-38.patch queued for re-testing.
Comment #43
jhodgdonCan 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:
Comment #44
sunSee #24.
See #28.
Comment #45
jhodgdonNeither 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.
Comment #46
sunSeriously? 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?
Comment #47
jhodgdonI'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.
Comment #48
franzOn 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.
Comment #48.0
pieterjd CreditAttribution: pieterjd commentedUpdated issue summary.
Comment #48.1
pieterjd CreditAttribution: pieterjd commentedUpdated issue summary.
Comment #49
yanniboi CreditAttribution: yanniboi commentedThank you to @pieterjd for updating the issue summary!
Comment #50
yanniboi CreditAttribution: yanniboi commented#39: drupal8.search-module.1789768-38.patch queued for re-testing.
Comment #52
yanniboi CreditAttribution: yanniboi commentedNeeds re-roll apparently...
Comment #53
jhodgdonThanks 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.
Comment #53.0
jhodgdonUpdated issue summary.
Comment #54
jhodgdonApparently 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...
Comment #55
jhodgdonActually, 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.
Comment #55.0
jhodgdonadd note about the effect of this bug
Comment #56
jhodgdonSo... 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?
Comment #56.0
jhodgdonadd task to task list for duplicate issue needing test
Comment #56.1
jhodgdonadd related issue
Comment #57
jhodgdonWe 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.
Comment #58
jhodgdonThis was completely taken care of on #1366020: Overhaul SearchQuery; make search redirects use GET query params for keywords.
Comment #59
Jieyyal CreditAttribution: Jieyyal commentedHi
I saw this issue still on drupal7
Won't fix for drupal7 ?
Comment #60
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedNote 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.