Form errors are set at the field level in form_set_error() and the array is checked for errors for each individual fields when rendering a form. The problem arises when two forms, which exist on the same page, have the same field names. The forms API marks the fields in both forms as errors.
An example of where multiple forms appear with the same fields is the editview module, where node forms exist repeatedly down the page.
The patch I've included moves the static array out of the form_set_error function, into a private function which allows you to set the arrays contents. I've also created a function which allows you to reset the form array, so it can be called between rendering two forms. This is not an ideal solution, as form errors should really be tied to the form_id so that if the same field exists in a number of forms on the same page, it doesn't mark all of them as an error on a page rebuild.
Comment | File | Size | Author |
---|---|---|---|
#38 | drupal8-form.inc-119208-38.patch | 2.4 KB | ttkaminski |
#36 | drupal8-form.inc-119208-36.patch | 2.41 KB | ttkaminski |
#35 | drupal7-form.inc-119208-35.patch | 3.83 KB | ttkaminski |
#31 | drupal6-form.inc-119208-31.patch | 3.27 KB | ttkaminski |
#30 | drupal6-form.inc-119208.patch | 1.77 KB | ttkaminski |
Comments
Comment #1
eaton CreditAttribution: eaton commentedThis looks very promising. I've run it through its paces on number of different forms and seen no ill effects; it also makes certain very impressive hacks like the "editview" module possible.
Long term we want to solve it in a 'proper' way; for now, simply fixing the bug may do. Thoughts? Anyone?
Comment #2
Mojah CreditAttribution: Mojah commented+1 on this
If for no other reason than the making the editview.module work.
Also confirming that I've tested this patch and have not discovered any errors so far.
Comment #3
eaton CreditAttribution: eaton commentedThis can probably be integrated withthe new $view_state work that's been done in the FormAPI 3 patch for Drupal 6. If form errors are carried around in the $form_state variable rather than a general array, they can be restricted to just the form that's being rendered.
Comment #4
Dries CreditAttribution: Dries commentedI don't think the proposed patch solves this problem the right way. I agree that we want to fix this, but we'll want to fix it properly. The solution might be to follow Eaton's advice, or to pass a $form_id into form_set_error(), form_has_errors() and friends.
Comment #5
eaton CreditAttribution: eaton commentedThe latter approach is something chx and I considered for the FAPI3 patch but decided against because of the already-large list of features that were going in. I'd love to see that happen; we already made a similar change to form_set_value(). I don't have time to tackle it myself but I'd be happy to give some guidance to anyone willing to take on the patch.
Comment #6
ray007 CreditAttribution: ray007 commentedsubscribing
Comment #7
goose2000 CreditAttribution: goose2000 commentedHi, I'm trying out the Editview, we need this module or something quite the same. How do I apply this patch? Copy & paste on forms.inc ?
A little scary pasting into the core...
Comment #8
nterbogt CreditAttribution: nterbogt commentedThere is a page here which details how to apply the patch
http://drupal.org/patch/apply
In linux it's as simple as going to the base installation directory of drupal (the one that has the sites directory in it) and running 'patch -p0 < form_2.patch'
I haven't done this on Windows, but I'm sure that the page above can tell you how to do it in that case.
If you're feeling cautious about patching core, which you have every right to be, I suggest reading patches before applying them.
This patch only effects the file '/includes/form.inc', so you could make a backup of this file and roll back if you're unhappy with the outcome.
Happy patching!
Comment #9
nterbogt CreditAttribution: nterbogt commentedChanging the status back to being a bug against 6.x.
I'd still love to see a resolution on this... but I don't think I have the background knowledge of the forms API to be able to do it myself.
I do like the idea of storing the errors in the $form_state variable, but again, not entirely sure what I'd be doing if I were to try and implement it.
I'd be happy to take a look into it if someone can provide me with some direction.
Comment #10
eaton CreditAttribution: eaton commentedIn a nutshell, we currently call form_set_error($name-of-form-element, $error-message). That stores things in a global variable that is shared across all forms. (whoops!) At its simplest, form_set_error and form_get_errors should be keyed by form id THEN form element. Ideally, though, we would instead build up elements in $form_state['errors']. We'd only display them to the screen when the form is rendered, and things like drupal_execute would be able to check the errors collection after execution to find any problems.
Comment #11
pisco23 CreditAttribution: pisco23 commentedThis may not be the place for this newbie question but.... I don't have shell access on my shared host, and I don't have any linux box at home (shame on me), so how can I use the patch? Is it possible to manually merge the two files? If not, does "patching" involve any modifications to any files other than form.inc? If its just a modification of this file, could someone provide me with a 'patched' form.inc (or am I being too naive?)
Thanks for any help with this
Comment #12
Zach Harkey CreditAttribution: Zach Harkey commented+1
This bug is really blocking a lot of cool administrative innovation here.
It's a bug, not a feature. While it may not be the perfect long-term fix, is there any argument that it is better than the current behavior?
Comment #13
StuartDH CreditAttribution: StuartDH commentedTried this on 5.2 but it doesn't patch. Is there an equicvalent for 5.2 as I can't get edit view module to work and it looks as though it's the same problem as with 5.1
Comment #14
jason.fisher CreditAttribution: jason.fisher commentedAlso interested in a 5.x patch/snippet.
Comment #15
mkalkbrennerTo require a
form_id
in addition to a form element is a good approach. But it doesn't solve all problems. The second thing is that you need a possibility to reset the error state of a form. For example if you run a script that calls drupal_execute multiple times on the same form_id (like we create nodes from an data file).This patch solves the second issue. But due to the fact that a reset of the error array must be called from others it should not be private (function
_form_set_error_array
should not start with an underscore).Comment #16
nterbogt CreditAttribution: nterbogt commentedYou can do this with a form_reset_errors(); anyway though.
Just a thought.
Comment #17
mkalkbrennerYou're right. I didn't recognize this function. Sorry.
But if you decide to extend the patch and handle form_ids, form_reset_errors() should have a signature like form_reset_errors($form_id).
Comment #18
Darren OhBug fixes should not be put off to the next version when there is a patch available for the current version. Since there have been many changes since Drupal 5, a separate issue should be created for Drupal 6, or the status of this issue should be made “to be ported” after the patch has been applied to Drupal 5.
Applying this patch to Drupal 5 will not affect future versions. From what I can see, it’s ready to be committed. The re-roll below offsets the patch by four lines but does not change any code.
Comment #19
drummAs implemented, this is an API change and should not go into Drupal 5.x. As far as I can tell, this does not actually fix the problem, just provides a new API to reset the form error array.
It is usually best to fix bug in the current development version first. More people will look at 6.x bugs since that is the development focus at the moment. This is an exception since 5.x and 6.x Form API are a bit different.
Comment #20
sinasalek CreditAttribution: sinasalek commentedAn updated version of patch for Drupal 5.6
Comment #21
mkalkbrennerI discovered an additional issue.
drupal_validate_form()
behaves likeform_set_error()
and caches the form_ids of all forms that have been validated. So every form will only be validated once!I extended the latest patch in this thread to solve the problem the same way like for
form_set_error()
. Now it's hopefully possible to really process the same form multiple times in one script.I also added a function named
form_reset_all()
to reset errors and validation cache at once.Comment #22
canadrian CreditAttribution: canadrian commentedAnyone know if this works on 5.7?
Comment #23
mkalkbrennerWe currently run drupal 5.7 with form.inc5-6.patch applied.
Comment #24
geek-merlin+10 :-)
subscribing.
Comment #25
asak CreditAttribution: asak commentedIs this still necassery for Editview ? will this work on Drupal 5.10 ?
Comment #26
maria_zk CreditAttribution: maria_zk commentedsubscribe
Comment #27
1kenthomas CreditAttribution: 1kenthomas commented@25: Patch appears to work for 5.10, cannot tell if it is necessary for edit-view (don't see a difference).
Comment #28
Darren OhThis is only necessary for Editview when JavaScript is disabled in the browser.
Comment #29
dpearcefl CreditAttribution: dpearcefl commentedConsidering the lack of activity on this issue and that Drupal v5 is no longer supported by fixes or patches, I am going to close this ticket.
Comment #30
ttkaminski CreditAttribution: ttkaminski commentedHere's a patch for drupal 6 that uses a different approach to solve the problem than the patches submitted above. It uses the POST variable to get the form_id. It then uses the form_id to scope the form element name so that when rendering the form element, it will be able to identify exactly which field the error applies to. The patch should be completely backward compatible, and requires no changes to any of the forms to work. This patch can be easily updated to work with Drupal 7 and 8. Please review and provide feedback.
Comment #31
ttkaminski CreditAttribution: ttkaminski commentedI found a case where the patch in #30 causes problems when certain modules depend on the return value of form_set_error(). I reworked it so that the return value remains the same to allow for better backward compatibility. Added a parameter called $scoped, that allows one to request the errors array with scoped keys. This additional parameter shouldn't cause problems because it has an appropriate default value. I think for Drupal 8, these form_set_error() and form_get_error() functions should not depend on these two modes of operation, but use the scoped mode only.
Comment #32
apadernoAs the issue is still present on Drupal 7, the patch needs to be given for Drupal 8, and then back ported to Drupal 7.
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedI just came across this nasty bug :(
Comment #34
kscheirerFirst, patches must adhere to Drupal coding standards. Specifically, there should be spaces between function arguments, no trailing whitespace on any line, always use braces even for 1-line ifs, comments should be on the line above with a period at the end just to name a few. You can also use the coder module to review your patch and it'll highlight most of those same problems.
Second, since this is a patch for D8 first - let's have $scoped be TRUE by default - that should simplify the function calls somewhat. If/when it gets backported to D7, that's when the maintainer may request that scoped start off as FALSE for consistency.
Third, we need a test. It'll need a page with 2 forms on it, both with form_elements with the same name. Then submit it with the error, and ensure that only the form element with the error is marked.
simplify to return $scoped ? $form_scoped : $form;
I'm not sure I understand this for loop, can you explain what's happening?
Comment #35
ttkaminski CreditAttribution: ttkaminski commentedI've been using the attached patch for drupal 7 that is based on my patch in #31 but with slight enhancements. This version keeps an array of errors indexed by form id. This approach allowed me to develop a special ajax module that displays errors to the correct form.
For drupal 8, I think it's safe to replace the old
form_get_error()
andform_set_error()
withform_get_error_scoped()
andform_set_error_scoped()
respectively. Any contrib modules that rely on the return value ofform_set_error()
would need to be updated as it would return an form id indexed array.Comment #36
ttkaminski CreditAttribution: ttkaminski commentedHere's a patch for drupal 8. Much simpler because we don't have to worry about backward compatibility. I haven't tested this patch, and I also didn't include any test code.
Comment #37
kscheirerremove trailing whitespace
remove trailing whitespace
Comment #38
ttkaminski CreditAttribution: ttkaminski commentedWhitespace removed.
Comment #39
kscheirerThanks ttkaminski! Now let's see what testbot thinks...
Comment #41
ttkaminski CreditAttribution: ttkaminski commentedTestbot didn't like it. A bunch of tests failed because the return value of
form_get_errors()
returns different information. I'm thinking of changing the function to pass in the form id:function form_get_errors($form_id) {
The tests would still need to be updated to pass in the $form_id, however. The second option is to use the method in #35, which makes it backward compatible.
Comment #42
tim.plunkettI think I duplicated this issue, but also fixed it in #2131851: Form errors must be specific to a form and not a global.