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).
Comment | File | Size | Author |
---|---|---|---|
#29 | 799356-drupal-7-form_set_error-29-testonly.patch | 1000 bytes | vijaycs85 |
#29 | 799356-drupal-7-form_set_error-29.patch | 1.51 KB | vijaycs85 |
#28 | 799356-drupal-7-form_set_error-28.patch | 1000 bytes | vijaycs85 |
#26 | 799356-form_set_error-23-error.png | 44.15 KB | andymartha |
#26 | 799356-form_set_error-23-success.png | 44.4 KB | andymartha |
Comments
Comment #1
rfaysubscribe
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedActually, 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.
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedMerged this into #766146: Support multiple forms with same $form_id on the same page.
Comment #4
Dave ReidThe #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.
Comment #5
Dave ReidBTW, 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.
Comment #6
cesarpo CreditAttribution: cesarpo commentedcommit this so I don't have to hack core please :)
Comment #7
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #8
dabl CreditAttribution: dabl commented#2: drupal.set_error_class_only_for_validated_elements.patch queued for re-testing.
Comment #10
dabl CreditAttribution: dabl commentedBecause the patch is old enough to come from cvs it needs work!
Comment #11
m1n0 CreditAttribution: m1n0 commentedDrupal 8 has this issue already fixed, re-rolling patch for Drupal 7.
Comment #12
m1n0 CreditAttribution: m1n0 commentedComment #14
m1n0 CreditAttribution: m1n0 commentedRe-submitting patch (typo)
Comment #15
BrockBoland CreditAttribution: BrockBoland commentedConfirmed 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?
Comment #16
ZenDoodles CreditAttribution: ZenDoodles commented#14: error_class_for_validated_elements_d7_0.patch queued for re-testing.
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedThis 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.
Comment #19
jaffaralia CreditAttribution: jaffaralia commentedHere i correct the patch for #2
Thanks,
Jaffar
Comment #21
jaffaralia CreditAttribution: jaffaralia commentedI corrected the patch for #2.
Comment #22
xjmSo, 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.
Comment #23
vijaycs85@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:
Without patch log:
With patch page:
Comment #25
vijaycs85#23: 799356-form_set_error-23.patch queued for re-testing.
Comment #26
andymartha CreditAttribution: andymartha commentedAs 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.
Comment #27
catchCommitted/pushed to 8.x, moving to 7.x for backport.
Comment #28
vijaycs85Porting to drupal 7.
Comment #29
vijaycs85Ignore patch in #28. Use patch here ....
Comment #30
andypostWithout #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
Comment #31
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted 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?