Remove drupal_reset_static for drupal_html_id() when form validation fails

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

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

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

Yeah like andypost says you should write a test :)

I recommend attach this test to AJAXMultiFormTestCase::testMultiForm

right now HEAD is broken so patch testing postponed 'til #61456-91: Aggregator titles display quotes and other characters with HTML entity equivalents badly (write 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

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)

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

@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 @@
+}
+ ¶

#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');
+    }

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

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

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

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

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

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.

@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

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

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.

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

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

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

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

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

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

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.

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

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

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: Allow AJAX to use GET requests

Another round to fix tests

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

Just a test

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

Filed separate issue about ajax_html_ids #1575060: ajax_html_ids are broken for forms with file element (encoding=multipart/form-data)

Simplified test, cleaned comments

Another fix of comments

Rerolled for form.test PSR-0

Tagging.

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

Excellent! Thanks.

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

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

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)

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

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.

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.

@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

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.

upload the patch again this time from linux.

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

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.

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!

@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

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)

Related issue: #2034631: Remove drupal_reset_static for drupal_html_id() during form processing

Added bare issue summary template