Problem/Motivation

Bug in Drupal 7.x.

For example, page contains at least two different forms. If submit one of them and validation fails I will recieve this forms with same element IDs.
If I use AJAX I may got an unexpected behavior, because jQuery thinks that I have no duplicates in DOM. It takes first element with needed ID and process it using AJAX.
But needed ajax element might be second in the DOM, and AJAX was processed wrong element.

To test this you should enable ajax register and contact modules. Put login block into the second sidebar, then logout and go to /contact page. When you get there, just submit contact form. No data should be filled (you should get a validation error).
Then open any web developer tool (I tested in firebug) and search for #edit-name or #edit-submit element. You'll find it in contact form and in the loggin form. Then try to submit contact form again and you will see that this button was processed with AJAX.

Proposed resolution

Problem

Proposed resolution

Also I've got a solution: When element was not validated in drupal_process_form function drupal_static_reset('drupal_html_id'); shouldn't be called, because it breaks correct ID's count.

Patch that solves that problem is attached.

Remaining tasks

User interface changes

API changes

Files: 
CommentFileSizeAuthor
#65 1414510-core-htmlid-54.patch3.17 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 39,588 pass(es).
[ View ]
#62 form_validate.patch2.04 KBperelman.yuval
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form_validate_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#61 form_validate.patch2.04 KBperelman.yuval
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form_validate_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#59 form_validate.patch2.11 KBperelman.yuval
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in form_validate.patch.
[ View ]
#58 before.jpg43.59 KBandypost
#58 after-submit.jpg46.56 KBandypost
#58 after-submit-fixed.jpg55.34 KBandypost
#54 1414510-core-htmlid-54.patch3.17 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 39,387 pass(es).
[ View ]
#52 drupal-1414510-52.patch3.37 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 40,568 pass(es).
[ View ]
#52 interdiff.txt1.04 KBtim.plunkett
#48 drupal-1414510-48.patch3.31 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 37,037 pass(es).
[ View ]
#47 1414510-html_id-with-test-47.patch3.25 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 36,590 pass(es).
[ View ]
#46 1414510-html_id-just-test-46.patch2.51 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 36,590 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#46 1414510-html_id-with-test-46.patch3.24 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 36,590 pass(es).
[ View ]
#44 1414510-ajax-just-test-43.patch2.67 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 36,589 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#44 1414510-ajax-with-test-43.patch3.41 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 36,592 pass(es).
[ View ]
#42 1414510-ajax-test-41.patch1.71 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 36,601 pass(es), 15 fail(s), and 0 exception(s).
[ View ]
#41 1414510-ajax-with-test-40.patch5.18 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 36,590 pass(es).
[ View ]
#39 1414510-ajax-with-test-39.patch3.77 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 36,583 pass(es), 21 fail(s), and 0 exception(s).
[ View ]
#37 1414510-ajax-with-test-37.patch2.97 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 36,764 pass(es), 22 fail(s), and 6,048 exception(s).
[ View ]
#29 contact-form-empty.png10.31 KBandypost
#26 1414510-26-test.patch1.16 KBSpleshka
FAILED: [[SimpleTest]]: [MySQL] 33,359 pass(es), 4 fail(s), and 0 exception(es).
[ View ]
#26 1414510-26-test-and-patch.patch1.77 KBSpleshka
PASSED: [[SimpleTest]]: [MySQL] 33,361 pass(es).
[ View ]
#24 1414510-ajax-test.patch750 bytesandypost
FAILED: [[SimpleTest]]: [MySQL] 33,358 pass(es), 19 fail(s), and 0 exception(es).
[ View ]
#24 1414510-ajax-with-test.patch1.48 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 33,337 pass(es), 4 fail(s), and 0 exception(es).
[ View ]
#19 1414510-19-remove-duplicate-ids-when-form-validation-fails.patch1.88 KBSpleshka
PASSED: [[SimpleTest]]: [MySQL] 36,987 pass(es).
[ View ]
#18 1414510-18-remove-duplicate-ids-when-form-validation-fails.patch1.92 KBSpleshka
PASSED: [[SimpleTest]]: [MySQL] 33,355 pass(es).
[ View ]
#17 1414510-17-remove-duplicate-ids-when-form-validation-fails.patch1.93 KBSpleshka
PASSED: [[SimpleTest]]: [MySQL] 33,364 pass(es).
[ View ]
#12 1414510-12-remove-duplicate-ids-when-form-validation-fails.patch1.89 KBSpleshka
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1414510-12-remove-duplicate-ids-when-form-validation-fails.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#10 1414510-10-remove-duplicate-ids-when-form-validation-fails.patch.patch1.93 KBSpleshka
PASSED: [[SimpleTest]]: [MySQL] 33,352 pass(es).
[ View ]
#7 1414510-7-remove-duplicate-ids-when-form-validation-fails_test.patch1.32 KBSpleshka
FAILED: [[SimpleTest]]: [MySQL] 33,354 pass(es), 4 fail(s), and 0 exception(es).
[ View ]
#3 remove-reset-drupal-html-id-when-form-validation-fails-D8.patch685 bytesSpleshka
PASSED: [[SimpleTest]]: [MySQL] 36,593 pass(es).
[ View ]
remove-reset-drupal-html-id-on-form-validate.patch665 bytesSpleshka
PASSED: [[SimpleTest]]: [MySQL] 36,992 pass(es).
[ View ]

Comments

aspilicious’s picture

Status:Needs review» Reviewed & tested by the community

The next test also tests on form_get_errors() so I believe this is a correct fix.

aspilicious’s picture

Version:7.x-dev» 8.x-dev
Status:Reviewed & tested by the community» Needs work

Srry needs work again, we need to fix this first in D8. Can you provide a d8 patch too?

Spleshka’s picture

Version:8.x-dev» 7.x-dev
Status:Needs work» Reviewed & tested by the community
StatusFileSize
new685 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,593 pass(es).
[ View ]

Sure, patch attached.
Should I write any tests or this fix is obvious?

Spleshka’s picture

Version:7.x-dev» 8.x-dev
Status:Reviewed & tested by the community» Needs work

Sorry for version and status change. It was happened by accident.

andypost’s picture

Issue tags:+Needs tests

I think better to add unit test to form.test to ensure that drupal_html_id static is not cleared when form has validation errors

aspilicious’s picture

Yeah like andypost says you should write a test :)

Spleshka’s picture

Status:Needs work» Needs review
StatusFileSize
new1.32 KB
FAILED: [[SimpleTest]]: [MySQL] 33,354 pass(es), 4 fail(s), and 0 exception(es).
[ View ]

Test is attached.

andypost’s picture

Status:Needs review» Needs work
Spleshka’s picture

Status:Needs work» Needs review
StatusFileSize
new1.93 KB
PASSED: [[SimpleTest]]: [MySQL] 33,352 pass(es).
[ View ]

Patch with fix and test.

andypost’s picture

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

Let's get this one commited, and please re-roll same path for D7

But I still prefer to see this test as part of testMultiForm as I pointed in #8

Spleshka’s picture

StatusFileSize
new1.89 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1414510-12-remove-duplicate-ids-when-form-validation-fails.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Patch for D7.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1414510-12-remove-duplicate-ids-when-form-validation-fails.patch, failed testing.

andypost’s picture

Status:Needs work» Reviewed & tested by the community

Posting patch for D7 you should change Version accordingly
#10 still ready to be commited

EDIT @Spleshka - let's get #10 commited and then roll a patch for D7, also you could use -d7.patch suffix for D7 patched within D8 issues (they're not be tested)

catch’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/modules/simpletest/tests/form.testundefined
@@ -5,6 +5,40 @@
+
+    // Give anonymous user permission to use contact form

This and other comments don't meet Drupal's coding standards - they should be a full sentence with a full stop. See http://drupal.org/coding-standards for more. There's also some whitespace issues in the patch - extra line breaks etc.

Also please upload the test as a patch by itself so the test bot can show it fails OK.

andypost’s picture

@catch #7 has only test that shows breakage

#10 is actual patch for D8 should be re-rolled without extra line breaks in testFormValidationFails()

@Spleshka #12 whitespace issue in D7 patch

+++ b/modules/simpletest/tests/form.test
@@ -5,6 +5,39 @@
+}
+ ¶
Spleshka’s picture

Status:Needs work» Needs review
StatusFileSize
new1.93 KB
PASSED: [[SimpleTest]]: [MySQL] 33,364 pass(es).
[ View ]

Thanks for your notes. Here is patch for D8.

Spleshka’s picture

StatusFileSize
new1.92 KB
PASSED: [[SimpleTest]]: [MySQL] 33,355 pass(es).
[ View ]

Patch for D8 without extra line breaks in testFormValidationFails().

Spleshka’s picture

Version:8.x-dev» 7.x-dev
StatusFileSize
new1.88 KB
PASSED: [[SimpleTest]]: [MySQL] 36,987 pass(es).
[ View ]

Same patch for D7.

andypost’s picture

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

#18 is good to go but ...

    // drupal_html_id() maintains a cache of element IDs it has seen,
    // so it can prevent duplicates. We want to be sure we reset that
    // cache when a form is processed, so scenarios that result in
    // the form being built behind the scenes and again for the
    // browser don't increment all the element IDs needlessly.
    drupal_static_reset('drupal_html_id');

I think this comment should be changed to say about why static is not cleared when form has errors.

+++ b/includes/form.inc
@@ -839,7 +839,9 @@ function drupal_process_form($form_id, &$form, &$form_state) {
-    drupal_static_reset('drupal_html_id');
+    if (!form_get_errors()) {
+      drupal_static_reset('drupal_html_id');
+    }
andypost’s picture

@aspilicious please post your opinion about test and check English grammar

aspilicious’s picture

+++ b/modules/simpletest/tests/form.testundefined
@@ -5,6 +5,35 @@
+ * Test duplicates elemens IDs when form validation fails.

The comment is not ok. You maybe can write:

"Tests that element ID's don't get duplicated when form validation fails."

if it is shorter than 80 chars. I'm not sure if it has to be IDs or ID's.

+++ b/modules/simpletest/tests/form.testundefined
@@ -5,6 +5,35 @@
+    // Give anonymous user permission to use contact form.
+    user_role_grant_permissions(DRUPAL_ANONYMOUS_RID, array('access site-wide contact form'));
+    // Go to page contains contact form.
+    $this->drupalGet('contact');
+    // Submit contact form.
+    $this->drupalPost(NULL, array(), t('Send message'));

These comments are not needed, code is clear enough.

Can you upload a patch with only tests and a patch with the test and the fix (only D8)

andypost’s picture

Test itself been posted at #7 results http://qa.drupal.org/pifr/test/218633

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new1.48 KB
FAILED: [[SimpleTest]]: [MySQL] 33,337 pass(es), 4 fail(s), and 0 exception(es).
[ View ]
new750 bytes
FAILED: [[SimpleTest]]: [MySQL] 33,358 pass(es), 19 fail(s), and 0 exception(es).
[ View ]

There's already test for 2 node forms, just extended it a bit

1414510-ajax-test.patch - just a test
1414510-ajax-with-test.patch - patch with test

locally both fails, but if I manually enable drush en form_test -y and make submit second node form with empty title - IDs are different

So let's see what bot will say

Status:Needs review» Needs work

The last submitted patch, 1414510-ajax-with-test.patch, failed testing.

Spleshka’s picture

Status:Needs work» Needs review
StatusFileSize
new1.77 KB
PASSED: [[SimpleTest]]: [MySQL] 33,361 pass(es).
[ View ]
new1.16 KB
FAILED: [[SimpleTest]]: [MySQL] 33,359 pass(es), 4 fail(s), and 0 exception(es).
[ View ]

@andy, your patch didn't pass the test, so I attached my own.

andypost’s picture

@Spleshka sheck my test and comment, your test is edge case but with ajax it somehow broken

sun’s picture

The patch in #24 looks better to me. However, we should revise the existing comment in form.inc and do a better job of clarifying why this condition/exception exists. (I don't fully understand it yet.)

That said, it's also a bit strange that the simple test in #26 failed, too.

andypost’s picture

StatusFileSize
new10.31 KB

@sun test fails without patch because after validation of contact form form elements gets the same IDs
login form also gets same IDs - edit-name, edit-actions and edit-submit
contact-form-empty.png

sun’s picture

ah, thanks, that explains it. yeah, #26 relies on way too much coincidence then. ;)

tim.plunkett’s picture

Status:Needs review» Needs work

So the patch with test in #24 still fails 4 times, because instead of there being page-node-form and page-node-form--2, there are two page-node-form--2.

Probably not a surprise, just my results from running the test locally.

andypost’s picture

@tim.plunkett can you explain i different behaviour of local & bot's fails in #24 - why locally

there are two page-node-form--2.

tim.plunkett’s picture

https://skitch.com/plnktt/8pkx9/ajax-test-with-two-form-instances-drupal

Both node form's get page-node-form--2 after validation fails.

andypost’s picture

I know about this but can't understand why #24 breaks this

Probably #node-form needs special processing or some hook_forms hacks

tim.plunkett’s picture

I don't think it breaks it, all of the fails in #24 with-fix are also in #24 test-only: http://qa.drupal.org/pifr/test/220473 http://qa.drupal.org/pifr/test/220478.

andypost’s picture

Probably today we found another cause of this trouble. In case of form with file element

<?php
 
//form.inc
if (isset($form_state['has_file_element'])) {
     
$element['#attributes']['enctype'] = 'multipart/form-data';
    }
?>

jquery.form.js wrongly serializes extraData and ajax_html_ids are passed as string to drupal backend

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new2.97 KB
FAILED: [[SimpleTest]]: [MySQL] 36,764 pass(es), 22 fail(s), and 6,048 exception(s).
[ View ]

New patch that should fix problem, core passes ajax_html_ids as array but in case of multipart data serialization fails.

EDIT I'm using space character as separator becaouse HTML5 allows any chars to be used as ID. Probably html5 innitiative should take care about this.

Patch still needs better comment as @sun suggests in #28

Exception is thrown when jquery.form.js gets updated to latest version (uses strict JS) #1574560: Update jQuery Form Plugin to the latest in the git repository

Also this should help to move #956186: [meta] Allow to use AJAX with GET requests

Status:Needs review» Needs work

The last submitted patch, 1414510-ajax-with-test-37.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new3.77 KB
FAILED: [[SimpleTest]]: [MySQL] 36,583 pass(es), 21 fail(s), and 0 exception(s).
[ View ]

Another round to fix tests

Status:Needs review» Needs work

The last submitted patch, 1414510-ajax-with-test-39.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new5.18 KB
PASSED: [[SimpleTest]]: [MySQL] 36,590 pass(es).
[ View ]

I found a way to reproduce failure - just add any field to content type with unlimited cardinality.
Patch just moves field creation after a testing IDs.

So it means that field API breaks html_ids but this is out of scope this patch

andypost’s picture

StatusFileSize
new1.71 KB
FAILED: [[SimpleTest]]: [MySQL] 36,601 pass(es), 15 fail(s), and 0 exception(s).
[ View ]

Just a test

Status:Needs review» Needs work

The last submitted patch, 1414510-ajax-test-41.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new3.41 KB
PASSED: [[SimpleTest]]: [MySQL] 36,592 pass(es).
[ View ]
new2.67 KB
FAILED: [[SimpleTest]]: [MySQL] 36,589 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Finally here's a new test.
Patch with just the test to make sure that functionality broken

andypost’s picture

andypost’s picture

StatusFileSize
new3.24 KB
PASSED: [[SimpleTest]]: [MySQL] 36,590 pass(es).
[ View ]
new2.51 KB
FAILED: [[SimpleTest]]: [MySQL] 36,590 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Simplified test, cleaned comments

andypost’s picture

StatusFileSize
new3.25 KB
PASSED: [[SimpleTest]]: [MySQL] 36,590 pass(es).
[ View ]

Another fix of comments

tim.plunkett’s picture

Issue tags:+needs backport to D7
StatusFileSize
new3.31 KB
PASSED: [[SimpleTest]]: [MySQL] 37,037 pass(es).
[ View ]

Rerolled for form.test PSR-0

Cottser’s picture

Issue tags:+Needs manual testing

Tagging.

geekyMoa’s picture

Status:Needs review» Reviewed & tested by the community

Patch in #48 passed manual test.

Tested by..

  • Enabling contact module and making sure user login block is placed in sidebar
  • Going to /contact
  • Entering invalid email to trigger validation error
  • Using Firebug to change type of input field to text, to circumvent browser validation.
  • Verifying that form element ID:s were duplicated before patch, and unique after applied patch
tim.plunkett’s picture

Issue tags:-Needs manual testing

Excellent! Thanks.

tim.plunkett’s picture

StatusFileSize
new1.04 KB
new3.37 KB
PASSED: [[SimpleTest]]: [MySQL] 40,568 pass(es).
[ View ]

It needed a minor reroll for the static class property $modules instead of setUp, leaving RTBC.

catch’s picture

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

Thanks! Committed/pushed to 8.x. Moving to 7.x for backport.

andypost’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new3.17 KB
PASSED: [[SimpleTest]]: [MySQL] 39,387 pass(es).
[ View ]

Patch for D7

EDIT Also ajax_html_ids totally broken and needs to be fixed for forms with file field #1575060: ajax_html_ids are broken for forms with file element (encoding=multipart/form-data)

arvinsingla’s picture

Status:Needs review» Reviewed & tested by the community

Came across a situation where this issue occurred. Tested the D7 port patch and works brilliantly.

webchick’s picture

Assigned:Unassigned» David_Rothstein

I think this makes sense (and comes with lovely tests, even!) but I'd love to get David's eyeballs on this before committing, since it does represent a change in behaviour.

David_Rothstein’s picture

Sorry for the delay.

I've been staring at this for a while and have yet to figure out exactly why the patch is safe. As far as I can tell, it relies on the fact that a form with errors will be rebuilt and re-displayed later on, or something like that. I can't figure it out from the code comment either, so I'm going to have to put this aside and look again later. This is touching some pretty fundamental stuff and I don't want to commit it to Drupal 7 until I really understand how it works, sorry.

As an aside, this patch does not fix the both-forms-get-the-validation-error bug shown in the screenshot in #29 (nor is it actually trying to). I was pretty confused by that, because when you test this with the contact form you see the same behavior either way... But it seems like that's just a separate bug entirely.

andypost’s picture

StatusFileSize
new55.34 KB
new46.56 KB
new43.59 KB

@David_Rothstein bug with high-ligting errors totally out of scope this issue.
Also this change is not so fundamental and I think better to include it into upcoming release.

You can test it manualy by:
1. Patch only tests hunk
2. enable form_test module
3. visit

form-test/double-form

4. see the html-id
5. try to submit any form

See the attached screenshots

perelman.yuval’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.11 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in form_validate.patch.
[ View ]

Hi all, i make a patch to fix the both-forms-get-the-validation-error, i know this is not the issue here but i cant find the relevant one, so i upload it here.
I dont sure if it is the right solution, so plase review and give me feedback.

Status:Needs review» Needs work

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

perelman.yuval’s picture

StatusFileSize
new2.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form_validate_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

upload the patch again this time from linux.

perelman.yuval’s picture

Status:Needs work» Needs review
StatusFileSize
new2.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form_validate_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
andypost’s picture

Status:Needs review» Reviewed & tested by the community

Still waiting #54 to be commited, #59-62 is irrelevant

@perelman.yuval your changes are out of scope this issue - please file new one in D8 queue and drop a link here

Status:Reviewed & tested by the community» Needs work

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

David_Rothstein’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new3.17 KB
PASSED: [[SimpleTest]]: [MySQL] 39,588 pass(es).
[ View ]

I think I convinced myself that this patch is safe, but incomplete. So, we can commit it to Drupal 7 and file a followup issue for the rest.

I'll get it committed soon. Reuploading #54 now for the testbot.

David_Rothstein’s picture

I filed a followup issue with a possible patch at #1831560: Remove Html::resetSeenIds() call during form processing.

As I mentioned above, the screenshot shown in #29 looks like an independent bug also, so it would be great if someone filed a followup issue for that one too (if it doesn't exist already).

Thanks!

andypost’s picture

@David_Rothstein I glad to see this commited and have follow-up for errors highlighting - probably @perelman.yuval work could be used as starting point. As I figured out this issue about to fix html_id generation for forms in case of validation error.
The error highlight is caused by using element name and not ID - so it's different task and probably is not applicable for D7 backport

David_Rothstein’s picture

Status:Reviewed & tested by the community» Fixed
Issue tags:+7.17 release notes

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

Just to clarify, by the way (since I think #66 and #67 were crossposts), there are two possible followups here:

  1. #1831560: Remove Html::resetSeenIds() call during form processing (which is about duplicate HTML IDs appearing in another situation that is similar to this issue, but not exactly identical)
  2. An issue for incorrect error highlighting due to overlapping element names (not yet filed)
David_Rothstein’s picture

Assigned:David_Rothstein» Unassigned

Status:Fixed» Closed (fixed)

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

fago’s picture

fago’s picture

Issue summary:View changes

Added bare issue summary template