If you have multiple forms on a page, it's easy for an element in one form to have the same #parents as an element in another form. For example, suppose a page has 2 search forms, and each form has an element named 'search'. In this case, if one of these is submitted with a validation error, when the page is rendered, both will have the 'error' class (and therefore, be red, or however the theme styles the error class). This is clearly bad UI. Solving this isn't trivial, because what's needed is for either _form_set_class() or form_set_error() to have access to $form_state (so that in addition to matching on #parents, we also only consider an element to be eligible for the 'error' class if its form's $form_state['process_input'] is TRUE).

Files: 
CommentFileSizeAuthor
#29 799356-drupal-7-form_set_error-29-testonly.patch1000 bytesvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 40,111 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#29 799356-drupal-7-form_set_error-29.patch1.51 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 40,132 pass(es).
[ View ]
#28 799356-drupal-7-form_set_error-28.patch1000 bytesvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 40,118 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#26 799356-form_set_error-23-error.png44.15 KBandymartha
#26 799356-form_set_error-23-success.png44.4 KBandymartha
#23 799356-form_set_error-23-test-only.patch1.02 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 52,650 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#23 799356-form_set_error-23.patch1.6 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 52,616 pass(es).
[ View ]
#23 799356-without-patch.png22.86 KBvijaycs85
#23 799356-without-patch-log.png42.35 KBvijaycs85
#23 799356-with-patch.png23.26 KBvijaycs85
#21 drupal_set_error_class_validated_elements_d8-799356-2968920.patch1.46 KBjaffaralia
PASSED: [[SimpleTest]]: [MySQL] 42,796 pass(es).
[ View ]
#19 drupal_set_error_class_validated_elements_d8-799356-2968920.patch577 bytesjaffaralia
FAILED: [[SimpleTest]]: [MySQL] 42,301 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#14 error_class_for_validated_elements_d7_0.patch531 bytesm1n0
FAILED: [[SimpleTest]]: [MySQL] Fetch test patch: failed to retrieve [ error_class_for_validated_elements_d7_0.patch] from [drupal.org].
[ View ]
#11 error_class_for_validated_elements_d7.patch471 bytesm1n0
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch error_class_for_validated_elements_d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 drupal.set_error_class_only_for_validated_elements.patch617 byteseffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.set_error_class_only_for_validated_elements_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

subscribe

Status:Active» Needs review
Issue tags:+Needs tests
StatusFileSize
new617 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.set_error_class_only_for_validated_elements_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Solving this isn't trivial, because blah blah blah

Actually, it is. This could use a test so it stays fixed, but that test will be easier to write after #766146: Support multiple forms with same $form_id on the same page lands, since that issue sets things up nicely for testing multiple forms on a page.

Status:Needs review» Closed (duplicate)

Status:Closed (duplicate)» Needs review

The #766146: Support multiple forms with same $form_id on the same page has been pushed back to D8, so I would like to consider us fixing this only for D7.

BTW, we don't need the tests on 766146 - we only need contact module enabled and to try and submit the contact form without the name field - you'll get a required indicator on the login form block's name field as well.

commit this so I don't have to hack core please :)

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7

Status:Needs review» Needs work
Issue tags:+Needs tests, +needs backport to D7

The last submitted patch, drupal.set_error_class_only_for_validated_elements.patch, failed testing.

Because the patch is old enough to come from cvs it needs work!

Version:8.x-dev» 7.15
StatusFileSize
new471 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch error_class_for_validated_elements_d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Drupal 8 has this issue already fixed, re-rolling patch for Drupal 7.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, error_class_for_validated_elements_d7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new531 bytes
FAILED: [[SimpleTest]]: [MySQL] Fetch test patch: failed to retrieve [ error_class_for_validated_elements_d7_0.patch] from [drupal.org].
[ View ]

Re-submitting patch (typo)

Version:7.15» 7.x-dev
Status:Needs review» Needs work

Confirmed that this fixes the bug in D7, by using the Contact form method that Dave Reid described in #5.

I think that we'll need a test case for this one—right?

Status:Needs work» Needs review
Issue tags:-Needs tests, -needs backport to D7

Status:Needs review» Needs work
Issue tags:+Needs tests, +needs backport to D7

The last submitted patch, error_class_for_validated_elements_d7_0.patch, failed testing.

Version:7.x-dev» 8.x-dev

This is not yet fixed in D8. One way to replicate:
- Give "Administer vocabularies and terms" permission to anonymous
- Disable JavaScript in your browser
- View "admin/structure/taxonomy/add" as anonymous
- The User login block in the left sidebar has a "Username" element with name="name" and the main taxonomy form has a "Name" element with name="name". Click either form's submit button without filling in anything, and both elements get a red border.

Status:Needs work» Needs review
StatusFileSize
new577 bytes
FAILED: [[SimpleTest]]: [MySQL] 42,301 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Here i correct the patch for #2

Thanks,
Jaffar

Status:Needs review» Needs work

The last submitted patch, drupal_set_error_class_validated_elements_d8-799356-2968920.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.46 KB
PASSED: [[SimpleTest]]: [MySQL] 42,796 pass(es).
[ View ]

I corrected the patch for #2.

+++ b/core/modules/system/lib/Drupal/system/Tests/Form/FormTest.phpundefined
@@ -247,7 +247,7 @@ function testRequiredTextfieldNoTitle() {
-    $this->assertFieldByXPath('//input[contains(@class, "error")]', FALSE, 'Error input form element class found.');
+    $this->assertNoFieldByXpath('//input[contains(@class, "error")]', FALSE, 'Error input form element class found.');

So, the fact that this assertion is failing with the current patch and has to be changed makes me think the fix might be introducing a regression. Jaffar and I spoke on IRC and he is going to create a separate test method that tests the case described in #18, without the fix. Then we can further explore whether the fix is correct or not.

StatusFileSize
new23.26 KB
new42.35 KB
new22.86 KB
new1.6 KB
PASSED: [[SimpleTest]]: [MySQL] 52,616 pass(es).
[ View ]
new1.02 KB
FAILED: [[SimpleTest]]: [MySQL] 52,650 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

@xjm, you are right, #22 is a regression. Added additional test case to test multiple form same element name issue. Attaching screenshot of test results and test-only patch to fail :)

Without patch:
799356-without-patch.png

Without patch log:
799356-without-patch-log.png

With patch page:
799356-with-patch.png

Status:Needs review» Needs work
Issue tags:-Needs tests, -needs backport to D7

The last submitted patch, 799356-form_set_error-23.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Needs tests, +needs backport to D7

#23: 799356-form_set_error-23.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new44.4 KB
new44.15 KB

As described by effulgentsia in #18, the 8.x-dev branch of Drupal on March 6, 2013 did show the double error messages to me (with or without Javascript) in Safari and IE. The Chrome, Firefox and Opera versions I use all use the smart html5 forms to catch the form from submitting with blanks so I am unable to test those. See attached error message image.
The patch 799356-form_set_error-23.patch was able to correctly place the red error css classes onto only the single form which created the error. See attached success message image.
Not sure what else might need to be done.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed/pushed to 8.x, moving to 7.x for backport.

Status:Patch (to be ported)» Needs review
StatusFileSize
new1000 bytes
FAILED: [[SimpleTest]]: [MySQL] 40,118 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Porting to drupal 7.

StatusFileSize
new1.51 KB
PASSED: [[SimpleTest]]: [MySQL] 40,132 pass(es).
[ View ]
new1000 bytes
FAILED: [[SimpleTest]]: [MySQL] 40,111 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Ignore patch in #28. Use patch here ....

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests

Without #1575060: ajax_html_ids are broken for forms with file element (encoding=multipart/form-data) for D7 it would be anyway broken if both forms have file field.
So RTBC but should be commited after pointed above

Status:Reviewed & tested by the community» Fixed

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/154389d

However, I'm very curious why the code was added there, rather than inside form_get_error() itself... Should that be a followup?

Status:Fixed» Closed (fixed)
Issue tags:-needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.