FAPI fails to check auto-generated IDs for uniqueness - XHTML validation fails.

Same problem on my site. I changed user.module user_login_block() from:

petermoulding.com/web_architect

Changed search.module search_box from:

Also add id to name to solve a conflict with the contact form.

petermoulding.com/web_architect

I have the same issue.

Comes up in three places on my install, I am using the Google Site Search Module, built in Search Module, and of course a login form. All three use the same identifier "edit-submit."

Fairly simple to address in the code to work around it, but would be nice to fix this so that out of the box Drupal sites validate.

(And BTW, I am new to Drupal, great software in all, props to all involved.)

I just spent a bunch of time searching through all of the Drupal 5.1 install files trying to locate where "edit-submit" was being called from. Should've done a search here first :P

This should definitely be fixed with the next Drupal release.

Add me to the list of users who would like this issue fixed :-)

I would also like to see this fixed without having to change code included with drupals core modules. I get errors for more than just the "edit-submit" ID. When latest poll block is enabled and a poll has been promoted to the front page I get ID "poll-view-voting" , "edit-nid" , "edit-vote" and "edit-poll-view-voting" already defined. I'm currently using 5.1 with a login bar in header, login block, googlesearch and core search blocks all using the same identifier. Needless to say my pages won’t be validating any time soon…

People who do not wish to hack core modules may try this approach until the issue is resolved:
If you are using a phptemplate theme, add the following code (without the php tags) to the bottom of your template.php file...

I should make it clear that the suggestion offered above will only fix the validation error caused when the conflict is between a search form and one other form. If you have multiple forms on one page then you'll have to look for the theme functions for those forms, or alternatively maybe work on the theme_item function (?) and add a random number to each "edit-submit", but sorry I haven't got time to look into that.

Or even the theme_submit function...?
http://api.drupal.org

Go on then, while I'm waiting for slow number crunching pcs with ancient cpus and not enough ram!
Forget the snippet above, put this (without the php tags) into your template.php file:

If that does happen then it probably won't on the next page load! You could always try changing 100,999 to 100,999999 if you're that desperate! ;-)

Would somebody like to adapt this into a patch and submit it? I know nothing about patching!

I reckon this is "patch (code needs review)" status if the above code is accepted and somebody changes it into a patch for form.inc?

Subscribing. Thanks

That shouldn't be a patch, only a temp hack. It doesn't make any sense to randomise an element ID since that renders it useless - it might as well be removed.

It is just a hack, and there's no actual patch attached. Setting back to active. http://drupal.org/diffandpatch

I'm subscribing to this too...

beautiful code, standards, and validation are all important to drupal. shouldn't this get fixed in drupal 6?

Plonking IDs onto every single element really misses the point of CSS. Drupal is brilliant when it comes to the actual PHP code, but there is too much garbage in the markup output.

If every form on a page has an ID, there is no need anymore to have individual IDs for form elements if they all have a class anyway. That is more than enough to target each item individually. And as far as DOM trickery goes...the awesome jQuery would need even less to do its magic.

So why not take id="edit-submit" off completely and let class="form-submit" do it all? How many differently styled submit buttons does one need on a page?

I believe this is minor, yet severe enough to be fixed in HEAD and then backported down to 5.x branch.

But at the end of the day, it really depends on people's interpretation on how big an issue is if a site doesn't "validate".

So why not take id="edit-submit" off completely and let class="form-submit" do it all? How many differently styled submit buttons does one need on a page?

+1

I suspect there are probably modules that will break if the ID is changed. I think this should be considered more a feature than a bug, and therefore should be left for 6 only (in official releases). If anyone cares about it deeply enough for their d5 site, they can hack it.

I wish there was a way to subscribe without having to post...

I wish there was a way to subscribe without having to post...

+1

To follow up, here is what I would consider a more elegant, reliable "hack" to get past the validation errors with minimal effort. It's the same as Sverre's approach, but use this code instead (still in template.php):

subscribe

Hi - this solution works great! I hope these changes find their way to Drupal 6. However, I still have validation errors with "id = edit-name" and "id = edit-pass". Could anyone show how to modify this snippet to work with these tags?

Thanks very much!

Based on the ideas of the other fixes floating around here, I wrote a module that handles all issues with duplicate ids (I also had issues with edit-nid and others). Just install the module, will be make all id attributes unique after formbuilder had its go. you can remove the fix from your template.php file too. works great for me, but no guarantuees.

As no tar.gz can be attached, here is the code per file. They all need to go into a directory drupal5formfixer in the modules directory.

drupal5formfixer.info:

drupal5formfixer.install:

drupal5formfixer.module:

Oops, looks like attaching the tar.gz in my post before worked after all with an extension .patch. it IS a tar.gz though. Sorry about that duplication, it was not intended.

I've tried the module send by macrule but it didn't change anything. Still the same evaluation errors and no changes in names for id's... Do I have to configure something in the module?

The phptemplate_submit() function only seems to be called for users that are logged in. It's not running for a non-logged in user.

subscribing

i wonder if it might be better to start a new issue for this that's focused on fixing it in drupal 6... there's a lot of tangential stuff going on in this thread.

The problem is that form.inc automatically generates IDs for form elements in such a way that on pages with multiple forms where the forms have elements that are defined the same, the IDs get duplicated. This patch adds to the form element ID the form's ID as well, to hopefully eliminate the duplication.

This patch is for Drupal 6.x. I can create one for 5.x as well, since I fixed it there first.

I want this to get looked at before the code freeze, so "Bump!"

I also think it makes more sense to put the $form_id near the beginning of the #id because of the order of #parents puts the most specific parts at the end.

I suspect there are probably modules that will break if the ID is changed.

Does the ID do anything server-side? I thought it was just the name and value that are transmitted back... I can't imagine anyone using the ID to style the button specifically, either, since any such styles could be far easier applied to the button directly in the form-building array.

There are a few issues in the XHTML it's impossible to work with without taking away some functionality, but duplicate IDs are both unnecessary and bad. In fact, unlike minor issues like having divs inside p tags, or text outside a p tag (which the validator will complain about, but the browser will ignore), this is an XML violation and will in fact break the page entirely when served as application/xhtml+xml. That was in fact what I was doing when I noticed this bug.

I expect themes use it in their CSS files. There are probably some modules that use it too. They should be using .form-submit, so this is a good opportunity for those developers and themers to clean up...

There are possibly also modules that edit the DOM or style via javascript, and depend on the ID of it to find it.

But *can* you do getElementsById() on an ID that is duplicate? I thought the very fact that it's not being used correctly would prevent Javascript and CSS from working with it correctly... unless it's a browser quirks mode thing, of course.

Maybe not, but it's quite possible that modules have not taken into consideration this particular edge case, and never had the bug reported. Remember this bug only affects sits with multiple submit forms on each page, which is not every site. Usually submit forms are only found in the content/body of a page, but some blocks also have them (like simplenews, EC, and others) -- which is where this becomes an issue.

Patch #33 is for D6.

This patch is exactly the same, for D5.

Patch #33 (or #32) is a reasonable approach. It solves the problem, and I cannot see what it would break, especially for D6 which is the dev version.

#32 looks OK to me for Drupal 6, but it would be great to get some actual testing with all the folks asking for this change, plus a comment into the code on why we add the form ID.

You mean something like this:

IMHO a shorter comment would be enough (your first sentence):

or:

I have tested the Patch #38 on D5 and it works for users who are logged in. Nevertheless for non-logged in users it does not have any effect (seems the same problem like #29).

I'm not very familiar with the forms api but it seems to me that the function form_builder is not called for non-logged in users because I tried changing sth there and it showed no effect when reloading my page (not logged in) but it changed when I logged in.

Anybody who knows a solution to that for non-logged in users?

@Morgenstern: it's a caching problem, no?

@morgenstern. clear the cache

I guess we lost radio contact with Morgenstern.

Here is the same patch, with a comment added.
Gábor mentioned patch #32 but didn't say anything about #33, so it's unclear if he has a preference between the two combinations.

This patch is based on #33.

This will work fine -- the 'id' attribute isn't actually used by FAPI in parsing anything, so we don't have to worry about breaking (say) submission handling or what not.

I spoke too soon. I just checked and the patch interferes rather seriously with quite a bit of Javascript code -- many of the jQuery snippets rely on id detection, etc. Check the teaser splitter for an obvious example.

Would the next course of action be to go through the core's JavaScript and CSS and change every instance of an ID selector? It's the most obvious solution, but I wonder if there isn't something more elegant.

Since teaser.js is new in version 6, and since I don't yet have a version 6 that's publicly available to validate, I'll be a little slow on updating this. This week is going to be fairly full for me, so it may be next week before I can get back in here with new testing and patching as necessary.

I forgot to mention that since teaser.js is not in Drupal 5, beginner's patch should be fine as is, unless someone finds something else, and except for the first three odd lines:

Tracing it back to where the teaser splitter get's its settings is difficult. Wonder where else it would break.

I think I found the source to the splitter, but anyone think it's worth it? I could try to post a patch.

sorry for answering so late.
@43-45: yes you were right. It was a cache problem. Patch #38 works fine for my D 5.2 and my site now validates to XHTML 1.0 Strict! YEAH!

Thanks a lot for distributing the patch!

Since we have confirmation that it was a cache problem, maybe someone can RTBC this issue?

Ive tried patch #38 for drupal 5.2 and it works for validation fine BUT its disabling all the options while submitting or editng a content. All the links like Meta tags, menu setting, file attachments and so on are dead after applying this patch.

Whats the best solution to validate a page without other problems??

Patch #38 seems to solve the underlying problem, but any javascript (including my own) which references any form element will suddenly and completely break.

As a module developer, I kinda need to know what ID certain specific form elements will carry. Since there's no telling how form element IDs will be constructed after this bug is "officially" fixed, I'm using "#id" to manually set the form element id for any element which I need to pick out using jQuery. As long as I use appropriately-unique ids (including my module name, form id, etc.) I'm assuming this isn't a problem.

Until there's some official word on this problem, I'm going to assume that this is the best way to deal with it in a forward-compatible manner.

It would be a very rare edge case to have two forms with the same ID in the content -- but two forms in blocks is conceivable; Suppose a theme uses search-theme-form twice, once at the bottom and once at the top. Or pulls search-block-form into another block and implements two search forms this way. So what if we only append to block forms? We could add '-block' or the block ID '-block-search' or even 'block-search-0'?

Can we do this without breaking javascript modules? Shouldn't modules be using class instead of ID if they want to work with ALL instances of a form on the page, (instead of just the form in the content area)? We should add some classes too then. e.g.
<input type="submit" class="form-submit edit-search-block-form-submit" value="Search" id="edit-search-block-form-submit-0" name="op"/>
instead of
<input type="submit" class="form-submit" value="Search" id="edit-search-block-form-submit" name="op"/>
and modules and themes have a few ways of specifying precisely which forms they edit.

What happens to forms in panels? Are we worrying about too-extreme edge cases?

admin/content/search has two submit buttons in one form if you have permissions to do an advanced search. This is arguably an issue with search.module and/or FAPI, and perhaps should be dealt with there.

Suppose a theme uses search-theme-form twice, once at the bottom and once at the top. We could add '-block' or the block ID '-block-search' or even 'block-search-0'? Can we do this without breaking javascript modules?

I agree with bevan about adding a unique index to the end of the form element id. However, I propose that it be a universal behavior of FAPI - not something limited to forms inside blocks.

Since, in simple cases, FAPI is supposed to handle generating XHTML for the module author, in theory, FAPI's default behavior should be to generate correct XHTML if it's called correctly. However, given FAPI's current design, it will generate incorrect XHTML in even relatively trivial cases:

Assuming "mymodule_somenavigationform" has any elements in it with auto-generated element IDs, that code will generate invalid XHTML in the current implementation and with current proposed patches.

What if we generate a more unique form element id, such as being based on the form id (as the patch proposes) but add a unique index for each instance of the rendered id? So instead of simply doing "edit-search-theme-form-submit", we do "edit-search-theme-form-submit-0" for the first instance of that particular element, and ++ for every subsequent element.

This could be done fairly simply by maintaining a static associative array in form_builder. (However, I don't know enough about Drupal core to know if that would work. For example, is caching likely to cause problems with this sort of implementation?)

Now, observant readers will point out that I'm proposing a "solution" which will put Javascript developers like myself in just as much of a jam as I was complaining about earlier. After all, if I want to do some javascript that interacts with the form, I still need to know what the element IDs are going to be.

However, this is really all just a matter of documentation. If the proposed method of automatically-generating truly unique form id is clearly documented, developers can depend on it for future work. So, by using the '-0' variant of the element ID, developers are guaranteed to pick out the first (and probably only) instance of that element. Even better, the solution scales to cases where the module developer wishes to interact with more than one copy of an otherwise-identical form.

And besides, if "#id" is clearly documented as being an alternative to using the auto-generated IDs, module developers will have the option of using it instead. (As for theme developers, it's been pointed out that class is probably more appropriate than id for situations like this.)

So, to recap, I think it makes sense to fundamentally alter FAPI so it is guaranteed to generate unique element IDs. (Unless, of course, the module author stupidly sets a non-unique #id in the form itself.)

What do some other developers think? This is a big change, but I remember having similar problems since drupal 4.x, and imho it's about time for a good solution.

ttyl
--Alex

As this and other issues have clearly illustrated, Drupal's FAPI currently fails to guarantee the uniqueness of generated form elements, despite the fact that the (X)HTML specification specifically demands it.

Currently, installing Drupal HEAD and enabling the search module will result in not one, but three instances of id="edit-submit" at search/node. Which fails w3 validation and makes it difficult or impossible to reference those elements in the DOM.

Not only does this design flaw cause problems for this extremely common case, it also makes it impossible to call the same form multiple times in the same page. (For example, if the developer wants a drop-down navigation form both above and below a page full of content?) (I give a pseudo-code example above.)

My patch does two things: 1) Adds the form_id to the element id as proposed by #38, and 2) Alters form_clean_id to guarantee that all auto-generated form IDs are unique. (It does this by simply keeping track of all passed IDs. The first instance of any given form id is passed without alteration, but subsequent instances of that same ID are made unique with the addition of a '-N' at the end.)

So, the idea here is that all auto-generated HTML ID attributes should be passed through form_clean_id() to guarantee sanity by removing incorrect characters and guaranteeing uniqueness.

For those of you who wonder why the "while" is not an "if", this is simply to avoid buggy behavior in this case:

If you're interested in seeing the patch handle a severe edge case, please see my sandbox installation of drupal 6 HEAD. It currently contains four instances of "search_block_form" on the front page, and not only do all four forms work properly, the page validates at w3.

There's a chance that a patch like this might break compatibility with some javascript-based modules. There's also a chance that other side-effects might happen that I simply can't predict. However, I think that with proper documentation and effort, this patch could significantly improve drupal's ability to automatically generate valid XHTML even in edge cases.

I know 6 is already in beta, but is there any chance a fix for this design flaw could get merged for 6? (I would have stepped up to the plate on this issue sooner, but I actually thought it had been fixed circa 4.7... I guess I've been out of the loop for a while.)

Thanks for your time! ttyl
--Alex Markley

Please pardon yet another follow-up... Still trying to make #59 work 100%.

It was brought to my attention that my patch breaks the teaser splitter. This is actually not a problem with my patch, but a problem with the way node module constructs body elements in node_body_field(). Specifically, node_body_field() incorrectly assumes that the body element ID which is auto-generated by FAPI will be "edit-body".

I have attached a patch here which corrects this, and now teaser splitter works correctly on my copy of drupal 6 HEAD with patch #59 installed.

I have begun testing more various features of drupal 6 to see if #59 breaks them (for example, it seems as though auto-completion still works correctly) but I need your help! Please let me know if #59 appears to break anything else so I can provide the appropriate patches.

ttyl
--Alex Markley

This approach looks sane to me.

Shouldn't the two patches combined into one?

The patch may break javascript in some places. We need to figure out where and fix those in the same patch.

Patches need to be combined. Also, all presumptions about id at form generation time is going to be fragile -- add a #process attribute. And are you sure it's the teaser splitter -- anyone inspected every JS file for id dependency? Or they are class bound?

I can roll a patch for both changes. The reason I didn't before is because #60 and #59 are independent. (Ie, #60 functions perfectly even without #59.) (And also my ignorance of protocol 'round these parts.) ;)

As for presumptions, you're right. It's a problem for code to assume anything which is not clearly documented. In this case, since form element id auto-generation doesn't have clearly documented behavior, any code which depends on form element ids may easily do it incorrectly.

Teaser splitter is an example of code which does this incorrectly in HEAD. Autocomplete is an example of code which does it correctly. (Even though autocomplete does use IDs, it "intelligently" figures them out at runtime in a way which works despite #59.)

I've been scouring core for any more examples of ID dependency which will break with the introduction of #59, but I'm limited in the amount of time I can devote to this. If people can help me out with this, I'm confident we can get this stabilized sooner than later.

@62: chx, I'm not sure I understand what you mean regarding the #process attribute. Isn't using '#id' sufficient to preemptively take control of the ID before the form is rendered?

I'll put more hours into this this evening if I can, and I'll roll+attach a combined patch then.

ttyl
--Alex Markley

I suggested using #process to set up stuff for JS once #id is in place.

*nods* makes sense to me... But somebody who is more familiar with the teaser splitter code and Drupal's best practices may need to help me. (I'm not 100% clear on how the '#teaser' property is getting pushed out to the Javascript code... I need to look into it more.)

Essentially, the way I fixed it in #60 works fine on my HEAD, although I'm not completely sure it conforms with Drupal's best practices.

My thought is that auto-generating element IDs should only be the default behavior, not the only "correct" way of doing it. If a module cares what the element's ID is, I think it should be responsible for allocating one itself and setting it with '#id'. Especially if form_clean_id()'s purpose is expanded to guaranteeing uniqueness, it can be used by modules to generate valid and unique IDs on-demand.

If '#id' is clearly documented as being an alternative to FAPI's auto-generator in situations where auto-generation would be sub-optimal, module authors would have a much cleaner way of knowing their elements' IDs as well as communicating them with their client-side javascript code.

Keeps the code simple. ;)

If somebody has a better alternative to #60, please post it here and I will roll it with #59 later this evening once I'm done auditing Drupal core for id dependency.

ttyl
--Alex

IMO this patch fixes a bug and should be part of 6.0. Does anyone consider this a feature?

Is there a way we can automate a search for hard-coded dependencies on FAPI's current default IDs? Perhaps with some Regex a la coder.module?

IMO, I think it makes more sense to deal with these issues in separate issues filed under different the corresponding modules / components. We can help the module / component maintainers/developers with those issues by providing documentation and possibly some tool to find bad code.

This code would be useful for module developers and could be included in the upgrade guide at http://drupal.org/node/114774.

I'm happy to write the documentation for this, and contribute to node 114774.

I'm getting 5 warnings about duplicate IDs at http://sandboxb.malexmedia.net/ Although I think they're not related to FAPI.

@66 - I agree, IMHO this is a bugfix not a feature enhancement.

@67 - yes, I was experimenting and there were some non-FAPI-related duplicate IDs on the page. It should be fixed now. ;)

I have rolled a new patch combining the uniqueness patch and the node module fix. Thanks to chx the fix for node module actually works and is much simpler now.

I'm still looking for other things which are broken by this patch so they can be fixed. Please test and report problems!

ttyl
--Alex Markley

Continuing to work this out here. I've attached a patch which fixes the color selector module and the user module. As you can see, these fixes are very simple. They simply depend on the more specific method of auto-generating form element IDs.

(I've said before that depending on the auto-generated IDs is a bad idea, but that's only because the method of auto-generating IDs is currently undocumented and dangerous. IMHO, this patch suggests a new method of generating IDs which could actually be documented and depended upon.)

After careful grepping through Drupal HEAD, it looks like only book module and openid module may still have problems after this patch is applied. I'm working on fixes for both of those now.

Can I get anybody to weigh in here about whether or not a patch like this might make Drupal 6? If not, where do we go from here?

ttyl
--Alex Markley

Odd problem found. With the patch, inside the node edit form there's a new button present to "Change book (update list of parents)". Clicking on it presents this error.

notice: Trying to get property of non-object in /modules/taxonomy/taxonomy.module on line 447.

Without it, that button never appeared and I never seen it before this patch.

It looks like this patch almost solves a hidden issue.

Book module seems to work now! This new revision of the idUniqueness patch adds two lines to book.module which seem to restore book module to its usual state of functionality.

Note that instead of altering the javascript to point to a very specific ID, I altered book.module to request a more generic ID. This is because the book form elements in question may appear in any number of different forms, so having form_id appear in the ID for these elements is sub-optimal.

If, by some odd coincidence, the ID book module wants has already been allocated by another form element, we will simply receive a different form element, and the javascript code should fail gracefully. (Same supposedly "graceful" failure that would happen without the uniqueness patch, just minus all the validation headaches and undefined behavior.)

@70, according to comments I found in the code, that book module button you "found" is supposed to be for non-AJAX clients. That is, clients whose javascript functionality is non-existent or minimal.

Essentially, it's part of the whole "graceful failure" fallback mechanism which is apparently in place for book module. If clicking that button results in errors, that's a whole different set of issues that probably needs to be addressed.

ttyl
--Alex Markley

Great to see this coming a long! Thanks Alex! :)

It seems like this is about ready for some basic documentation already. Could you provide me few pointers as to what a module developer needs to do to safely use a FAPI id in their javascript? I'll use this to write a documentation stub and expand on later. Assuming this patch will make it into 6, it will be good to have the documentation ready when this gets committed so that there is somewhere to refer contrib module developers to when we break their module! :)

Do you mind if I use excerpts from your patches as examples?

The holy grail of patches! I finally wrapped my brain around this problem enough to create a simple fix which has practically zero chance of breaking contrib code.

The root problem we're trying to solve here is ID uniqueness. Previous attempts at solving this problem altered the id to be more specific. I like specificity, so my initial attempt at solving this problem inherited those changes. However, because the patch altered the default auto-generated id it broke a large amount of javascript code both in core and in contrib.

This patch simply avoids altering the default auto-generated ID unless that ID is going to cause a collision with another ID in the current page.

It might seem like common default IDs like "edit-submit", when run through the uniqueness filter, will be harder to predict. That's true, but most javascript code interacts with elements with much more unique names in the first place.

Besides, here's the key thought that unraveled this whole puzzle for me: Changing the ID to avoid a collision will only break Javascript which would already be broken by the collision itself!

So, in conclusion, this patch solves a serious design flaw in FAPI and avoids breaking anything that works already. I respectfully request that this patch be tested by anyone who can do so, and that this patch be considered for inclusion with Drupal 6.

ttyl!
--Alex Markley

@72 - Thanks for being willing to champion this, bevan! It's really nice to see that other people want this fixed too. ;)

As you can see with my latest patch, I've radically changed directions in my approach. My current Drupal HEAD has only that patch installed, and not only do all the pages validate, all of the javascript/ajax functions still work!

As for documentation, I think the best thing to do would be to simply make module developers aware that simple form element names like $form['submit'] are likely to be assigned IDs erratically. So if they want to safely interact with an element, they should name it something more specific either indirectly ($form['mymodule_foo_element']) or directly ($form['submit']['#id']).

I should stress again that my latest patch only alters IDs if they would otherwise cause a collision, so (in theory) no working code should stop working with the application of this patch.

ttyl!
--Alex

Malex, such a simple solution. Should have been the first approach. :)

Tested:
node splitter
clean urls
floating table headers
auto complete field in node authoring info
checkbox ranges in tables
editing outlines
collapsible field sets
password strength and match indicator

No errors and no dupe ID's.. I'd say this is RTBC.

Uhm, doesn't conform to coding standards. I'll up a new one.

It also doesn't need the while loop. That's an edge case we shouldn't have to work around. Some pages could have tons of id's to be cleaned.

Achieves the same thing. Hope you don't mind Malex.

And here's the page on coding standards. http://drupal.org/coding-standards

You were using $unique = $id.'-'.$seen_ids[$id];. there needs to be a space between the variable and the "." for string concatenations.

@77 - Thanks for the help with the coding conventions dvessel. My personal coding style is very different, so I don't quite 'get' the style drupal has standardized on. Still, must carry on, eh? ;)

I must argue for the while loop though. I know it looks scary wasteful, as if it's spinning and wasting CPU time, but it's actually not. (I put a lot of thought into this design.) Since I store the last index in the static array, it shouldn't ever need to actually spin unless it runs into a situation like this:

If those two pseudo-forms show up on the same page, the necessity of the while loop will come into play. So it might be an edge case, but it's not unthinkable. And like I said, it doesn't eat up any additionally CPU resources unless it really is needed, so I suggest that we leave it in.

ttyl!
--Alex

Ack! Patch at #78 is corrupted due to extreme exhaustion. :-P Comments at #78 still apply.

If we're going to guarantee id uniqueness, let's guarantee it for real. This version of the patch won't eat up any more CPU than #77, and I really can't picture this version of the patch using up very much more memory in anything but the most extreme edge cases.

ttyl
--Alex

Ugh, still trying to wrap my poor, weary brain around Drupal's coding conventions... Also incorporated some of Dvessel's more subtle suggestions.

+1

drupal.org doesn't validate because of this. XHTML1 Strict validation is good public relations!

This patch would have saved me some work.

Please apply to D6!

Malex, I've never seen that case in #78. On top of that, with the wile loop all it does is append another "-num".

for example, "form-id-of-1" set from the form would produce "form-id-of-1-1" when your patch sees doubles. That makes no sense. The core commiters don't like over engineering and it's a bit hard to read.

It should be as simple as possible to get the job done.

Malex, I've never seen that case in #78.

And you never will if FAPI isn't capable of properly rendering it. Here's another, possibly more common situation:

Again, this relatively simple set of forms will trigger an ID collision under your proposed revision to the patch logic.

On top of that, with the wile loop all it does is append another "-num". for example, "form-id-of-1" set from the form would produce "form-id-of-1-1" when your patch sees doubles.

I'm not sure I understand the point here. The goal here is not to produce "pretty" HTML IDs, or even "easy to understand" HTML IDs. The goal is to simply force them to be unique using the simplest and fastest logic possible.

That makes no sense.

What doesn't make sense to me is crippling perfectly reasonable and bulletproof logic by making it less reliable.

The core commiters don't like over engineering and it's a bit hard to read. It should be as simple as possible to get the job done.

I just want to see some progress made toward getting this bug fixed in drupal 6. In my opinion, your revision to my logic is simply lower quality. If you don't agree, that's fine, but in the interest of working toward merging an actual fix, perhaps we should get some core committers to actually weigh in here?

Imho I've gone way above and beyond the call of duty here. If one of the core committers wants to alter my logic or completely ignore my contribution, that's fine. Let's just get this bug fixed already.

ttyl
--Alex Markley

Let me clarify.. Appending *-num makes no sense for the loop your creating. The patch I posted does the *exact* same thing but it's easier to read.

I assumed that the while loop did something special to prevent that until I tested it. I may not be an uber coder but over engineering doesn't mean it's better quality. I learned all my coding through Drupal thanks to the readability of the code. What your doing is not necessary.

But I must say, thanks for pushing this forward. Using only form_clean_id() keeps it simpler and doesn't break anything else.

Let me clarify.. Appending *-num makes no sense for the loop your creating. The patch I posted does the *exact* same thing but it's easier to read.

Eh, I must respectfully disagree. I just tested your code again, and it breaks under the exact circumstances I suggested it would:

I'm not an "uber coder" either, but my logic was correctly and precisely engineered. Readability is completely subjective. Logic is not.

Dvessel, I very much appreciate your help in testing and campaigning to get this bug fixed. No matter what logic the core committers use, I don't care... As long as the bug gets fixed in drupal 6!

Can another third party (preferably a core committer!) review the patches at #80 and #77 and weigh in with some thoughts/suggestions? Dare I ask, even an RTBC?

ttyl
--Alex

All I can say is that it's *effectively the same*. Is there a real world issue where we'd encounter your use case?

And you should care what logic Dries or Gabor considers or it won't get in at all. One way or the other, I hope this relatively minor issue is fixed. It's been mocking me for a while now. :)

I'll say no moe.

I can't see why not to make it better and more safe, when it's possible. The version with while gives better results (we surely can't say that we always know *all* possible use cases, right?), and is even shorter than the other (by one else). Readability is good for me, I didn't have any difficulty to understand what it does, and I don't think that this is any more complicated than things I already saw in Drupal. So I vote for #80 (not tested yet, just my opinion to the #77 vs. #80 story).

@ #73 Sorry to put a wet blanket on it all. I'm not yet convinced this is a better approach. With this approach, how would a module module or theme developer target, for example, the submit button on the theme's search form with javascript. With the approach in #73 on, the ID could be edit-submit or edit-submit-N where N is any positive integer.

We could tell developers they MUST set [#Id] if they want to reliable get that element, or that they should use #search-theme-form .form-submit. But wouldn't it make developer's jobs easier if the id of the submit button was already unique and predictable (with documentation) without having to explicitly set #id? Consider modules that work on, edit or ajaxify other module's forms -- not only developers writing fresh forms.

@88 - I agree, for maximum flexibility we should have more specific auto-generated IDs. And, I think it would be a good idea to push for such a thing in d7.

At the moment, however, I'm mostly focusing on fixing this bug in time for d6. Since the situation you're describing (AJAX interacting with the theme search form's submit button) already won't work by directly selecting the ID, this patch shouldn't really make the situation any worse.

And really, that's become my goal for this particular patch: solve the validation bug and don't make the javascript situation any worse.

Now, if some core committers were to express interest in completely solving the design flaw despite breaking modules... and have it done in time to go out with drupal 6, that'd be a different story. ;)

ttyl
--Alex

Only the higher authorities will be able to decide.

I say #80 is good to go: it fixes the problem while avoiding messing with most IDs, and not breaking the javascript.

I've tried #80 it works well for me and I understand what it's doing (it's readable)....and believe me my understanding of most code is small =). also implemented on my 5.2 site and for the first time in a long time I can validate all my pages successfully....thank you thank you...thank you!!

Thanks for clearing that up for me Malex. I understand your point and goal, but I still think that your original approach is better. I think we have to wait for some "higher authorities" (lol) to make a call on this -- which is unlikely to happen till after DrupalCon is over btw.

+1 for either patch, preference to #71.

@91 - Thanks for the encouragement, StuartMackenzie! I'm so glad to hear that this patch is working for you. It's always nice to know that one's work is appreciated. ;)

Just in case I came across negatively in #92 (I didn't mean to); I'm really glad that this will (probably) be fixed in 6.0 -- and I really appreciate your effort Alex.

By "+1 for either patch, preference to #71." I was also trying to communicate both approaches are great and I really appreciate both. :)

Thanks Alex!

@94

:-D Not an issue, I got #92 no problem. In fact, I really wholeheartedly agree with you. I think #73 is the more solid approach from a flexibility/scalability/maintainability perspective, and if somebody could champion it (or something similar) for drupal 7, I think that'd be best.

I'm just such a newbie to the whole "creating patches against drupal core" thing... Proposing an API change at the eleventh hour like this would probably scare the willies out of anybody, let alone someone as inexperienced as myself.

I'm really grateful for how supportive you guys have been helping this along and pointing out my many fallacies.

Hopefully we'll hear from some "higher authorities" early next week after drupalcon is over. ;)

ttyl
--Alex Markley

C'mon higher authorities! Discipline us, I dare you! lol! :)

subscribe

subscribe

So far #77 looks like an appropriate solution without costly loops, counting the number of occurances nicely and with good coding standards compliance. But the RTBC did not seem to be switched on for that. Are there objections against #77. Usually when an issue grows as long as this one, a summary post is in order, which would be great here too.

Have someone tried this patch in Firefox? I remember there is a error if the first letter of an ID is a number. The following is an example error message i already reported in http://drupal.org/node/146650 (duplicate). I think the above patch doesn't solve this bug!?

line 114 column 46 - Error: value of attribute "id" invalid: "3" cannot start a name

@100 - Hass, I use only firefox. ;) This patch does not place numbers at the beginning of the ID, only the end.

@99 - Gábor Hojtsy, #77 does not function properly as I demonstrated in #85. However, my patch (#80) does function correctly under all circumstances, and the "loop" it uses does not, in fact, loop. My patch also keeps track of the current number, so it never needs to loop unless there is an edge case.

Please allow me to reiterate, my while loop is not costly in any way.

I'm not sure I'm the right man for a summary post, since I might be a tad biased toward my own patch. ;) But if nobody else steps up to the plate I'll do it.

ttyl!
--Alex Markley

Here's a simple summary and another version of my path simplified even further..

This patch:

Malex's approach:

And I asked this question which Malex never replied to.

All I can say is that it's *effectively the same*. Is there a real world issue where we'd encounter your use case?

So, the argument is.. Was Malex's issue *ever* encountered. If so, I'd say go with his patch but I've never seen this and I don't think Malex did himself.

And again, the issue is about forms appending numbers to the end of it before form_clean_id() even sees it. And when it does, the form must generate more than one duplicate and in some odd order. IMO, that's over thinking the problem.

Sorry for not answering your question before dvessel, I have in fact seen this case before. Since FAPI itself appends numbers to the end of the form id depending on the structure of the form, some of my more complicated forms exhibit exactly this behavior.

Since FAPI itself appends numbers to the end of the form id depending on the structure of the form.

Malex, That's news to me.. Can someone verify this? FAPI appending numbers on it's own?

#83 is a specific example. Since the form is is a collapse of #parents, one or more of the array keys being a number will result in numbers in the form id.

Cross-linking a related issue: http://drupal.org/node/180897

#102 looks good to me. I've never seen such output malex describes. Additionally, please bear in mind that you can still override any form element id using form_alter().

Sorry, it's not. Go to admin/settings/filters/add and see:

If you mean the slash in id - well, below is a snippet from my test install, where the patch was never applied:

As a side note - I always prefered the #80 version, because I agree that a while whose condition is true only once, as compared to if ... else in the other version, makes no significant difference in performance, as well as readability, and #80 is more robust.
But however, if others decide to go with #102, I'm OK with that too. Let's just try and get *something* in :)

The same install with patch #102 (I assume you tried this one):

It's caused by filter_list_all(). Not sure if that's a bug itself but it eventually makes it down into _form_builder_handle_input_element().

And those numbers are not set arbitrarily. They coincide with the id of the input filters and roles.

So then this is not a part of the FAPI auto-generated IDs, and so I believe it's out of scope for this issue. IMO we need further review to decide if/which patch is ready to go / or needs work. Also moving from beta1, as 6.x-dev is far ahead now.

I'd still vote for #102. Drupal should support anything that is possible in core. If non-core modules produce XHTML invalid output, it's their fault.

If non-core modules produce XHTML invalid output, it's their fault.

The #80 concern is not about contribs producing anything invalid, as far as I understand it. It's about contrib-defined legal IDs possibly colliding with FAPI-cleaned ones (#102 patch generating invalid IDs on its own, under certain circumstances, being given a valid input).

Of course this all only applies to the case, where duplicate IDs exist - I consider this a 'valid input' to the FAPI for the sake of this argument, because even FAPI itself produce duplicates (and that's exactly why we want this patch in).

But since this is an edge case, and we need this issue solved, I don't move from RTBC. Just wanted to clarify my doubts. Basically I just don't understand why not to choose more robust solution, when available. Sorry for repeating myself ;)

Okay, let's stop the #80 vs. #102 discussion. Each one of us have own preferences, but basically both the patches solve the problem for common use cases, both apply with just an offset now (21 / 24 lines) and both work. So I hope we can get this in.

There is a strange bug related to this: if you place a form with an id="edit-submit" (e.g. the search block) somewhere before the main page content, then the ahah at admin/build/block works correctly twice, then mysteriously stops working the third time. (Firefox 2.0.0.7 on Ubuntu Dapper)
#80 and #102 both fix this problem.

I decided to commit #102. Thanks for all the help folks. :-)

The version that was committed has a pretty significant flaw -- there are a number of normal situations in core in which the builder function for a form is called twice -- once for validation and a second time for rendering of a 'fresh' version of the form. (Poll.module in core is an example). In these scenarios, the form displayed to the end user will have "-1" appended to the HTML ID of EVERY single element in the form. This breaks all JS code and any CSS code that relied on IDs.

The underlying problem here is forms that are printed on screen multiple times. The ID checking can't, by definition, happen at this layer unless we move HTML ID generation to the theming/rendering layer instead of the builder.

We need to roll the patch back, IMO, as it breaks pretty much everything ID-related in core in preview scenarios.

Also bumping this up to 'critical'

There are a couple of possible solutions.

1) Move element ID generation to the rendering layer, which could work but would require VERY detailed examination of our APIs to ensure it doesn't break other things.
2) Tell people to use elementname.classname selectors in all their CSS and JS code, rather than #id selectors. This strikes me as profoundly flawed, almost as much as generating duplicate IDs, since it means that HTML IDs are effectively unreliable and thus useless in any HTML generated by Drupal.
3) Don't generate IDs at all -- just insert them if someone manually specifies an ID in the element definition. This might be the best solution, but is probably very disruptive for late-in-the-game core changes.

If i could choose, i would prefere the first sollution. Sounds definetly better then the rest.

I make no promises that it's even possible to DO this far past freeze.

This is a bug fix, not a feature request, and a fairly serious one at that, considering Drupal claims to be valid XHTML Strict. I have seen things slip into core since the freeze that I would consider features. Granted, your propositions are not simple and are likely to require quite a bit of coding to work.

As I have started to try to communicate in the Drupal Markup Style Guide;

ID attribute's should NOT be assumed. And should not be set at low levels like drupal core and contrib modules.

Custom modules, themes, and even contrib modules and core, often pull chunks of html into a page multiple times. There is nothing wrong with that. However having the ID pre-set in any such markup is problematic.

As for JS that depends on ID attributes of it's own, or other module's markup -- those are broken anyway (when it's markup occurs multiple times) should be considered bugs and be patched. Here is a guide to what they should use instead, to find DOM elements to perform their JS / AJAX trickery on. In order of preference;

1. use class="" attribute instead
2. if it's a theme or custom module (but not core or contrib) it could be known that this this chunk of markup will only occur once the page, so set a unique ID, probably in the module's namespace (e.g.; id="custommodule-customdata") and/or with a fair level of uniqueness (e.g. id="content" and id="header" is acceptable as these are recognized as id's reserved for page-level theme elements (i.e. set in page.tpl.php) which only occurs once on a page).
3. if it's contrib or core and for some reason class="" is no good (why?), then the id="" attribute should be explicitly set (and never assumed) with a high level of uniqueness (e.g. 'form-submit' is no very unique, 'form-FORMID-submit-default' is more unique). The developer should also consider adding an integer to ensure that uniqueness, as the patches here have done.

I can't think of a reason why the class="" attribute is not useful. I'm not a frontend developer, but my understanding is that finding DOM elements by class is very similar to finding them by id -- especially with jQuery. The only difference being that the JS may have to handle multiple DOM elements in the result. Core or contrib JS that doesn't handle multiple occurrences of it's markup is problematic anyway -- whether they adhere to the above guide or not.

I feel quite stupid now, as no one of us involved here caught the problem before commit. Sorry!

I think there was also a proposal to just make FAPI-generated id's a bit more specific, including form id or something like that. It would mean not to clean invalid id's for compliance afterwards, it would mean building compliant id's in the first place, which is IMO the basic underlying bug here. It was discussed near top of this issue, patch #33 seems to address this.

There were some doubts later (including Eaton's serious ones - #47), but now compared to the three proposals above (and the commited verion being broken), I tend to like that approach again.

This is a bug fix, not a feature request, and a fairly serious one at that, considering Drupal claims to be valid XHTML Strict. I have seen things slip into core since the freeze that I would consider features.

Breaking any JS or CSS that relies on element IDs is pretty serious, too.

This is nontrivial. I didn't have time during the D6 cycle to figure out a clean solution But 'solving' one problem by breaking everything else is unacceptable. The current solution is worse than removing HTML IDs from Drupal's rendered output entirely: it preserves the presence of generated IDs, but breaks any javascript or CSS that depends on them whenever drupal_prepare_form() is called twice. IE, during preview-validation, with any AHAH or AJAX enabled forms, and so on.

This concern was raised earlier by both chx and myself. You have my sincerest apologies for not checking in every day to make sure that the patch didn't get committed as it stood.

Granted, your propositions are not simple and are likely to require quite a bit of coding to work.

In a perfect world, we would prefix every element ID with the ID of its parent form, then in the theming layer, where we actually print out the $element['#id'] property, call a drupal_ensure_unique_css_id() function or something along those lines. The latter -- passing IDs through drupal_ensure_unique_css_id() -- would be an effective stopgap solution at the theme layer, but also requires us to modify every single form element theme function.

An alternate solution would be to pass elements into drupal_increment_css_id() in drupal_render() itself. THAT, in turn, would ensure that if an #id property exists on a renderable element, it's unique.

Along with that we would need extensive warnings to developers working with CSS or AJAX functionality about when to use IDs and when to use tag + class selectors. No matter WHAT solution we adopt, it's going to change how people have to do things with JS and CSS in some of these custom cases.

Anyone think that would be workable?

(The above quote is not a quote, but added for emphasis -- it's a summary of a section of the markup style guide.)

That more or less relates to Eaton's #2 and #3. Yes, this is probably problematic for other areas of drupal. But those areas are already broken anyway because they assume the form ID. Whether we fix all of these now, or in d7 is up to d6 team, but this is definitely a bug, and I believe we are not in 'bug-freeze'?

Eaton, what is 'flawed' about using class instead of ID? ID's should not be set in core or contrib and should therefore not be considered reliable.

Can some front end / JS developers tell me if when or why class is not useful but ID is? I fear my lack of front-end development experience might mean I'm missing something.

The only case I can think of is with inline anchors; e.g. <a href="#content">skip to content</a> which requires a dom element has id="content". jQuery tabs module does something like this and is enough reason to need id's. What other core/contrib cases would need ids insteadof classes?

@126 Eaton:

'solving' one problem by breaking everything else is unacceptable. The current solution is worse than removing HTML IDs from Drupal's rendered output entirely

So what about doing that? Why don't we NOT set the id?

@126 Eaton:

No matter WHAT solution we adopt, it's going to change how people have to do things with JS and CSS in some of these custom cases.

Exactly. I'm beginning to think that we should either do nothing, or the full monty. All of these stop-gap solutions just seem to complicate the issue. I'm uncertain about not-setting-the-ID -- does that break anything?

Anyone think that would be workable?

It has to be at some point -- if not for d6 then for d7.

The only case I can think of is with inline anchors; e.g. skip to content which requires a dom element has id="content". jQuery tabs module does something like this and is enough reason to need id's. What other core/contrib cases would need ids instead of classes?

That is definitely a big one.

Yes, this is probably problematic for other areas of drupal. But those areas are already broken anyway because they assume the form ID.

Handling uniqueness checks at the form_prepare level (as this patch does) breaks any any form based JS or CSS that uses IDs, when the validation case is handled. I just don't see a solution to this problem that ensures unique IDs in common scenarios AND preserves the usefulness of IDs for JS and CSS selectors -- and those scenarios are the only reason we bother auto-generating them.

So, if we're talking about moving to classes as selectors, we return to the earlier statement: why bother with auto-generated HTML IDs in all elements? The cases where they are useful would be ruined by randomly adding incremental numbers. This raises other performance concerns about jQuery selector performance (IDs are fast, for example), but that's the way it goes.

Sorry if I sounded testy in my earlier message -- I'm just trying to figure out what possible solution could get things back on track without reducing the HTML IDs to uselessness.

Exactly. I'm beginning to think that we should either do nothing, or the full monty. All of these stop-gap solutions just seem to complicate the issue. I'm uncertain about not-setting-the-ID -- does that break anything?

Well, the problem is that FormAPI is hard-coded to generate an ID for every element at the moment that reflects its position in the form tree. We don't break FormAPI by changing that -- the HTML 'name' property is what we use to map things to the form structure on $_POST handling -- but moving to class based selector logic would definitely have possible repercussions for themes and modules already ported to 6.

I'm beginning to think that the real problem is our auto-generated IDs. Any IDs important enough for someone to set manually will NEVER work with auto-incrementing, and adding complex behavior to work around it will break code for the sake of making XHTML validate strict. It might be better to see how we can eliminate unnecessarily-generated HTML IDs entirely, then simply treat specific instances of hardcoded duplicate IDs as individual bugs in the modules that output them.

Thoughts? Before tavkling D6 vs D7, I at least want to figure out if this is the right approach...

@130 Eaton:

I just don't see a solution to this problem that ensures unique IDs in common scenarios AND preserves the usefulness of IDs for JS and CSS selectors -- and those scenarios are the only reason we bother auto-generating them.

I think that's because there isn't one! :)

why bother with auto-generated HTML IDs in all elements?

I don't think we should be auto generating them.

This raises other performance concerns about jQuery selector performance (IDs are fast, for example)

Oh. This is what I was missing about front-end development. Bugger! Hmmm...

Sorry if I sounded testy in my earlier message

You didn't sound testy -- at least not to me. Sorry too if my other post sounded asshole-ish.

PS. Yay!! Wohooo!!! My Dell 30" monitor just arrived.

Here's the bit about jQuery performance by the way: http://www.learningjquery.com/2006/12/quick-tip-optimizing-dom-traversal

The “speed hierarchy” goes like this, in order of fastest to slowest:

1. ID : $(’#some-id’)
2. Element : $(’div’)
3. Class : $(’.some-class’)

jQuery uses JavaScript’s native getElementById() to find an ID in the document. It’s fast — probably the single fastest way to find something in the DOM — because it’s only looking for a single unique thing, and it’ll stop looking once it has found it. So, if we have <h2 id="title">This is my title</h2>, we would use $('#title'), not $('h2#title') to access it.

To find a bare class name, as in #3 above, jQuery goes through every element in the entire DOM. To find an element, on the other hand, it uses getElementsByTagName(), thereby limiting the number of elements it has to traverse right from the start. Therefore, if we know which element the class is tied to, we can speed up the traversal significantly by referring to it, using, for example, $('div.some-class') rather than $('.some-class').

@131

I'm beginning to think that the real problem is our auto-generated IDs ... then simply treat specific instances of hardcoded duplicate IDs as individual bugs in the modules that output them.

Exactly! :)

@133 -- thanks! V. interesting

@131 Eaton

It might be better to see how we can eliminate unnecessarily-generated HTML IDs entirely, then simply treat specific instances of hardcoded duplicate IDs as individual bugs in the modules that output them.

Sounds good to me. Originally, before I ever posted to this thread, I was thinking this probably should be addressed at the module level, where element IDs were manually generated in the module that created the element, but when I found that the IDs were autogenerated and that the form element couldn't (shouldn't?) even set an ID, I went that way instead.

@130 Eaton

Sorry if I sounded testy in my earlier message

I didn't think you sounded testy, but I did think you were taking a revert-the-patch-and-sweep-the-issue-under-the-carpet stance. I do agree with you that it should be corrected, and I understand by your follow ups that you weren't taking that stance.

recap:
XHTML Strict requires unique element IDs
patched form_clean_id checks for element ID validity and uniqueness

Notwithstanding a need to re-think when or which forms should auto generate an ID - or in what layer those IDs should be generated - and the tradeoffs for wanting to provide some core Drupal user experience enhancements via javascript.... Eaton has pointed out that since during a preview operation element IDs are checked twice on the same page load - the second pass through will think that the element IDs already exist and give them new IDs - AND as a result certain core forms that implement javascript that depend on a specific DOM element ID will break.

The problem can be solved by flushing the static $seen_ids variable IF the form processing has got past validation.

This feels mighty hackish but attached is a patch that adds an extra param to form_clean_id called flush. The patch also adds a new call to the function just after form validation to do the flushing. After that stage the form will go through its render process with a fresh $seen_ids and not think that the elements already exist.

I have tested this in two ways. One by previewing a node form (a simple test case). The second was by enabling the search module and enabling its search block and going to the search page (creating four instances of 'edit-submit' button on the same page) and then submitting each form empty (producing an error and redrawing the page) and with a value (producing a result and redrawing the page). The results are what you would expect. ID uniqueness is retained AND ID indexes aren't artificially high.

This issue has obviously uncovered an inefficiency in the Form API (having form_clean_id being called twice as many times as it should in certain situations), but at this stage it would require the rewrite of a major portion of core forms to solve the inefficiency AND this issue specifically.

I think given the circumstances this is a decent solution for the time being.

We can think on better approaches after it's rolled back. Also, after Eaton and myself have raised the JS issue earlier in the issue it was quite an unpleasant surprise to see this committed all of a sudden breaking all the JS.

Wrong patch attached.

OK, rolled back to let development flow and discussion reach a good solution here.

Eaton:

You have my sincerest apologies for not checking in every day to make sure that the patch didn't get committed as it stood.

chx:

Also, after Eaton and myself have raised the JS issue earlier in the issue it was quite an unpleasant surprise to see this committed all of a sudden breaking all the JS.

Sorry for pointing this out, but none of you wrote a single word to this issue for over 3 months, while the others discussed the issue extensively. We addressed the JS concern carefully by *not* changing the ID's unless already being duplicate, so it's not that your input was ignored.

We only just didn't notice, that the builder is somewhere called twice. That's a serious mistake, I admit, which is probably the result of no FAPI-guru being involved in the discussion.

I'm not shouting to anyone (need to explain that, as my English sometimes bite me), just wanted to make a little side-note on this.

As for the problem itself - unfortunately, I've no solution... :-( Indeed, if something is broken by the patch, it needs to be reverted and then re-considered.

Sorry, cross-posted accidentally...

Sorry for pointing this out, but none of you wrote a single word to this issue for over 3 months, while the others discussed the issue extensively. We addressed the JS concern carefully by *not* changing the ID's unless already being duplicate, so it's not that your input was ignored.

It's fine. And, as I said earlier, I apologize for testiness in my comments. I was focused on trying to help with several of the AHAH-related patches, and the cases that trigger the buggy duplicate-handling are triggered by those areas, so I saw it quickly. My frustration was due to the fact that the problem was very obvious when using a core module in a very simple case (clicking preview), and did exactly what we'd warned about. On the other hand, there are quite a few core modules and it's nearly impossible to catch them all.

This is, more than anything else, an argument in favor of 1) a quick guide to 'what stuff almost always breaks when FAPI changes, and how to check it first when testing patches', and 2) automated tests that remove the burden of manually clicking around to double-check every time a function changes.

After extensive discussion here in the thread, discussion with chx, and testing on tricky forms like poll.module, Andre's additional 'flush' parameter seems to solve the worst of the problems. I've re-rolled it along with the additional code and added comments explaining in more detail why some of the checks are there and how it's called.

This patch WILL result in broken JS and CSS when they are coded against specific element IDs, and those element IDs appear multiple times on the page. However, the JS and CSS in question is arguably broken to begin with, since it's using IDs when it needs to use element-plus-class selectors. It's like preventing duplicate primary keys in the DB; even if something worked with the dupes, it was doing something wrong.

The real problem is our relatively 'promiscuous' HTML ID generation code in FormAPI. Things stay unique inside a given form but we generate lots of non-unique IDs at the page level. This patch trades 'semantically incorrect broken-ness' for 'semantically incorrect broken-ness that modules should account for anyways.'

In the future we may want to consider removing the auto-generation of element IDs in FormAPI, and making #id an optional manually-set parameter by developers. That, too, would help eliminate the problem though I'm not sure if it's the right solution.

Sorry, that should read:

This patch trades 'semantically incorrect broken-ness' for 'semantically correct broken-ness that modules should account for anyways.'

Minor comment typo: the possessive "id's" should be "IDs".

After even closer examination, there are definitely edge cases where duplicate IDs can still be generated. Because the ID uniqueness check is done at the build level, the IDs get cached. That means that if a form is cached, and when it's next displayed a different form is shown (say, in a sidebar block), the code that validates uniqueness can't do its checks. I can't think of a case where that actually HAPPENS, but the possibility is there.

I tested the form on a page with three copies of the same form, and a fourth copy of another form. Previewing and submitting things worked, the uniqueness check worked in those cases as well. It's not foolproof, but it's definitely catching the 90%.

At the very least, we can say that this is better than our current approach -- which generates duplicate IDs in several locations in a 'stock' install of Drupal core. While it doesn't handle every edge case, it is a workable stopgap that (at least) forces people to use better JS/CSS selectors in problematic areas and prevents the bulk of the duplicate IDs.

OK. Let's do this , commit the fix that's basically a mix of the earlier commited patch, Andre's fix and Eaton's nice comments.

And definitely come back in Drupal 7 to think more. There is a lot that can be done after a code thaw... but even if we would get the exception, the changes are sweeping and we do not have time anyways.

Wow, didn't realize it broke. Obviously FAPI is not my strength.

Thanks guys for the fix. :)

The docs on $flush does not seem right:

- we do not specify the type of parameters on the @param line
- we use TRUE an FALSE and so we should do in the code documentation of the parameter too

OK, committed this latest patch. Although I did a small phpdoc modification. AFAIK we are supposed to do one line function summaries on top of functions (although there are obviously deviations in core), so I modified form_clean_id()'s comment to stand on one line and have the details on an explanatory line.

Thanks!

I think the 'one line summary' rule is going to become problematic. While brevity is obviously a must, there are a lot of function summaries that just won't fit nicely in a single line. The PHPDoc comment that was committed is fine, but also feels a bit clunky for being broken up the way it is. It does two specific things -- describing one on the 'summary' line and the other below it seems to be an indication that our one-line-rule is not ideal...

That aside, hooray. :)

The one-line summary's role is to be able to display function summaries on command line help or in IDE tooltips or in by an IRC bot. All these have limited space.

Oooooh. That's an excellent point, thank you. I'll keep that in mind when writing them.

Yay! So we seem to have gone for the stop-gap fix for d6 -- right? What about a full fix for this problem for d7? What are your thoughts?

one line summaries: I'm not sure about other IDEs but zend will display the whole phpdocs for tool tips. In other words the length of the documentation may not need to be limited to one line.

specifying type parameters on the @param line: thats a (good) habit of mine, but I'll try and break myself of it when documenting for core.

andremolnar, the first line summary rule is for the api.module. you can read all about drupal's doc standards at: http://drupal.org/node/1354

I just got around to doing this, and I wanted to share it with everyone who was interested. Here's a copy of the chx's patch from #151 for Drupal 5.x, including Gabor's modifications.

Cool! Thanks. :) I wonder what is broken in d5 with that patch. I've doing this in themes to make the pages validate and haven't found anything broken yet;

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

val_id.info:

+1

This also should be fixed in the 5.x codebase IMO

another quick fix inspired of #23 but without a global variable:

Erm, I think that will return 'edit-submit-1' everytime because $elementCountForHack is reset to 0 every time the function executes. Try this instead;

I think no... Static variable is not reseting, the value is assigned only on the first call. See http://ch2.php.net/manual/en/language.variables.scope.php#language.varia...

But is also a good idea to do that with edit-name id (problem with login form and contact module). You see another double id?

You're right! :) I didn't know that. I think I must have acquired that from C#. I'll have to try it out sometime. Thanks! :)

just note to #166 - this wont work as expected.
In the str_replace argument
$mixed_replace. $count_double_id++ will return strings like 'Array0', 'Array1', etc.

I would use following code (just two str_replace instead simgle with array arguments):

PHP says differently:

• If search and replace are arrays, then str_replace() takes a value from each array and uses them to do search and replace on subject.

arrgh I see know the edit-name don't go through submit function...

This patch keeps slipping through the cracks. Any possibility of getting this fix incorporated into the next Drupal 5 release?

This isssue are automaticly closed... This bug are present in D6?

6.x-dev does not appear to have this error any more. It adds a "-{n}" suffix to all IDs that occur more than once to distinguish them.

I concur that this is a sufficiently nasty bug to be fixed in 5.x too, though not critical.

i tried the patch http://drupal.org/files/issues/fapi-5.x-dupe-id_111719-151.patch but was still getting the error

Patch from #159 is working for me on Drupal 5.7

#162 works perfectly for me using Drupal 5.7 :) Good temporary solution until core changes are made.

#159 works great, but misses a required change to theme_form() to clean a form's #id.

The same change is missing in D7 as well, so I'm attaching two patches for review. :)

Lets get 7 fully-fixed and then worry about the stable versions.

I've thoroughly tested the patch in #177. The only kind of bug I was able to find is that the system_modules form on admin/build/modules gets the ID "system-modules-1", although it's displayed only once on that page. This happens in all versions of Drupal core, so it seems that something else invokes theme_form() again.

@sun. I've noticed the same issue. It appears that in the FAQ module for example, a form with id 'faq_weight_settings_form' is sent through form_clean_id and later a form with id 'faq-weight-settings-form' is received. The str_replace in form_clean_id is replacing _ with - so it thinks that the form id has been received twice and adds the '-1'. I'll try running this patch without replacing _.

in my personal opinion, faq_weight_settings_form and faq-weight-settings-form should be considered the same, as this fix correctly does, however, it is actually a bug on the module side if they're using both faq_weight_settings_form and faq-weight-settings-form as the differences are so trivial between those two that the text is actually the same....

also putting this back to critical, as this breaks XHTML validation. (which Drupal should be, as stated by the doctype ;) )

The last submitted patch failed testing.

I wish there was a way to subscribe without having to post...

+1

The ideal would be a subscribe and unsubscribe without having to post. If a problem is in 5, I use 5, subscribe, then upgrade to 6, I may want to unsubscribe.

The problem becomes more complicated when I switch on another module. How do I pick up the new problems with that module? Perhaps someone could start a project to let us subscribe by release and module to receive an email when a new issue is created for a subscribed module or an existing issue has another release added. Would not need an email for every change, just a note of the new issue so we can look at it and see if it is applicable to our site.

A related problem is the version assignment for issues. I think it should be a multiple selection. Perhaps keep the current version as the version worked on then have a summary list of all the other versions mentioned in posts. This issue was against 5 and is now assigned to 7.x-dev. It does not show up against the Drupal versions I am using, 6.4, 6.5 or 6.6. For a subscription system to work, I would want a way of selecting all the versions of Drupal I have across my sites and see all the applicable issues even if they are assigned to 7 dev with a view to later backporting. When the backporting does occur, this issue might be backported to 6 then 5 and and up listed as 5 even though it also applies to 6.

Unsubscribing would also reduce database activity as people upgrade to newer releases, letting old issues fade away. You could purge old issues when there is no one subscribed to the issues.

"I wish there was a way to subscribe without having to post..."

please open another issue for that, this issue is about #111719: FAPI fails to check auto-generated IDs for uniqueness - XHTML validation fails.

Re-test.

Patch in #177 for Drupal 5.12 worked perfectly for me - with one small exception: field sizes setting on Compact Forms module no longer has any effect. Field sizes have to be set manually in CSS.

A most worthwhile tradeoff to have pages validate - thanks so much! Great work.

The last submitted patch failed testing.

subscribing

form_builder() in D7 already takes care of cleaning the form #id, so that patch is no longer required.

I'd say it's too late for back-porting. Hence, reverting status.