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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rfay’s picture

subscribe

effulgentsia’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
617 bytes

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.

effulgentsia’s picture

Status: Needs review » Closed (duplicate)
Dave Reid’s picture

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.

Dave Reid’s picture

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.

cesarpo’s picture

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

Tor Arne Thune’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
dabl’s picture

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.

dabl’s picture

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

m1n0’s picture

Version: 8.x-dev » 7.15
FileSize
471 bytes

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

m1n0’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

m1n0’s picture

Status: Needs work » Needs review
FileSize
531 bytes

Re-submitting patch (typo)

BrockBoland’s picture

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?

ZenDoodles’s picture

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.

effulgentsia’s picture

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.

jaffaralia’s picture

Status: Needs work » Needs review
FileSize
577 bytes

Here i correct the patch for #2

Thanks,
Jaffar

Status: Needs review » Needs work
jaffaralia’s picture

Status: Needs work » Needs review
FileSize
1.46 KB

I corrected the patch for #2.

xjm’s picture

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

vijaycs85’s picture

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

vijaycs85’s picture

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

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

andymartha’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
44.4 KB
44.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.

catch’s picture

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.

vijaycs85’s picture

Status: Patch (to be ported) » Needs review
FileSize
1000 bytes

Porting to drupal 7.

vijaycs85’s picture

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

andypost’s picture

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

David_Rothstein’s picture

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.