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.

Files: 
CommentFileSizeAuthor
#38 drupal8-form.inc-119208-38.patch2.4 KBttkaminski
FAILED: [[SimpleTest]]: [MySQL] 49,684 pass(es), 43 fail(s), and 8 exception(s).
[ View ]
#36 drupal8-form.inc-119208-36.patch2.41 KBttkaminski
FAILED: [[SimpleTest]]: [MySQL] 49,738 pass(es), 43 fail(s), and 8 exception(s).
[ View ]
#35 drupal7-form.inc-119208-35.patch3.83 KBttkaminski
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal7-form.inc-119208-35.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#31 drupal6-form.inc-119208-31.patch3.27 KBttkaminski
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#30 drupal6-form.inc-119208.patch1.77 KBttkaminski
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#21 form.inc5-6.patch2.72 KBmkalkbrenner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form.inc5-6_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 form.inc5-6.patch1.65 KBsinasalek
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form.inc5-6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#18 form.inc-119208.patch2.07 KBDarren Oh
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form.inc-119208.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
form_2.patch1.89 KBnterbogt
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form_2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

eaton’s picture

Status:Active» Needs review

This 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?

Mojah’s picture

+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.

eaton’s picture

Title:Repeating errors» form_set_errors() flags ALL instances of a form on a page
Version:5.1» 6.x-dev

This 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.

Dries’s picture

I 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.

eaton’s picture

The 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.

ray007’s picture

subscribing

goose2000’s picture

Version:6.x-dev» 5.1
Category:bug» support

Hi, 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...

nterbogt’s picture

There 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!

nterbogt’s picture

Version:5.1» 6.x-dev
Category:support» bug

Changing 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.

eaton’s picture

In 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.

pisco23’s picture

This 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

Zach Harkey’s picture

+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?

StuartDH’s picture

Tried 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

jason.fisher’s picture

Also interested in a 5.x patch/snippet.

mkalkbrenner’s picture

To 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).

nterbogt’s picture

You can do this with a form_reset_errors(); anyway though.
Just a thought.

mkalkbrenner’s picture

You'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).

Darren Oh’s picture

Version:6.x-dev» 5.x-dev
Status:Needs review» Reviewed & tested by the community
StatusFileSize
new2.07 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form.inc-119208.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Bug 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.

drumm’s picture

Status:Reviewed & tested by the community» Needs work

As 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.

sinasalek’s picture

StatusFileSize
new1.65 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form.inc5-6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

An updated version of patch for Drupal 5.6

mkalkbrenner’s picture

StatusFileSize
new2.72 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form.inc5-6_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I discovered an additional issue. drupal_validate_form() behaves like form_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.

canadrian’s picture

Anyone know if this works on 5.7?

mkalkbrenner’s picture

We currently run drupal 5.7 with form.inc5-6.patch applied.

axel.rutz’s picture

+10 :-)
subscribing.

asak’s picture

Is this still necassery for Editview ? will this work on Drupal 5.10 ?

maria_zk’s picture

subscribe

1kenthomas’s picture

@25: Patch appears to work for 5.10, cannot tell if it is necessary for edit-view (don't see a difference).

Darren Oh’s picture

This is only necessary for Editview when JavaScript is disabled in the browser.

dpearceMN’s picture

Status:Needs work» Closed (won't fix)

Considering 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.

ttkaminski’s picture

Title:form_set_errors() flags ALL instances of a form on a page» form_set_errors() flags ALL form elements with the same name on a page
Version:5.x-dev» 6.26
Status:Closed (won't fix)» Needs review
StatusFileSize
new1.77 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

Here'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.

ttkaminski’s picture

Title:form_set_errors() flags ALL form elements with the same name on a page» form_set_error() flags ALL form elements with the same name on a page
StatusFileSize
new3.27 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

I 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.

kiamlaluno’s picture

Version:6.26» 8.x-dev

As the issue is still present on Drupal 7, the patch needs to be given for Drupal 8, and then back ported to Drupal 7.

ivanjaros’s picture

I just came across this nasty bug :(

kscheirer’s picture

Status:Needs review» Needs work

First, 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.

+++ b/includes/form.incundefined
@@ -798,29 +798,41 @@ function form_execute_handlers($type, &$form, &$form_state) {
+  if($scoped) return $form_scoped;
+  else return $form;

simplify to return $scoped ? $form_scoped : $form;

+++ b/includes/form.incundefined
@@ -828,16 +840,29 @@ function form_get_errors() {
+  for($i=0;$i<2;$i++) {

I'm not sure I understand this for loop, can you explain what's happening?

ttkaminski’s picture

StatusFileSize
new3.83 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal7-form.inc-119208-35.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I'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() and form_set_error() with form_get_error_scoped() and form_set_error_scoped() respectively. Any contrib modules that rely on the return value of form_set_error() would need to be updated as it would return an form id indexed array.

ttkaminski’s picture

StatusFileSize
new2.41 KB
FAILED: [[SimpleTest]]: [MySQL] 49,738 pass(es), 43 fail(s), and 8 exception(s).
[ View ]

Here'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.

kscheirer’s picture

+++ b/core/includes/form.incundefined
@@ -1666,13 +1671,19 @@ function form_get_errors() {
+  $form_id = $element['#form_id']; ¶

remove trailing whitespace

+++ b/core/includes/form.incundefined
@@ -1666,13 +1671,19 @@ function form_get_errors() {
+  } ¶

remove trailing whitespace

ttkaminski’s picture

StatusFileSize
new2.4 KB
FAILED: [[SimpleTest]]: [MySQL] 49,684 pass(es), 43 fail(s), and 8 exception(s).
[ View ]

Whitespace removed.

kscheirer’s picture

Status:Needs work» Needs review

Thanks ttkaminski! Now let's see what testbot thinks...

Status:Needs review» Needs work

The last submitted patch, drupal8-form.inc-119208-38.patch, failed testing.

ttkaminski’s picture

Testbot 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.

tim.plunkett’s picture

Issue summary:View changes
Status:Needs work» Closed (duplicate)
Related issues:+#2131851: Form errors must be specific to a form and not a global

I think I duplicated this issue, but also fixed it in #2131851: Form errors must be specific to a form and not a global.