form_set_error() flags ALL form elements with the same name on a page

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?

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

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.

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.

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.

subscribing

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

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.

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

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

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

Also interested in a 5.x patch/snippet.

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

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

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.

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.

An updated version of patch for Drupal 5.6

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.

Anyone know if this works on 5.7?

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

+10 :-)
subscribing.

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

subscribe

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

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

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.

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.

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.

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.

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?

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.

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.

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

Whitespace removed.

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

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.

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