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:

to:

petermoulding.com/web_architect

Changed search.module search_box from:

to:

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

Hope this helps. :-)

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:

This should fix the problem with ALL forms, UNLESS you are unlucky enough to have two submit buttons on the same page given the same number by the random function (any three digit number between 100 and 999).

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:

Attachment	Size
drupal5formfixer.tar_.gz_.patch	721 bytes

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.

Attachment	Size
form.inc__3.patch	655 bytes

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.

Attachment	Size
unique_form_ids.patch	656 bytes

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.

Attachment	Size
111719-edit-submit-form-id.patch	706 bytes

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

Attachment	Size
111719-edit-submit-form-id-2.patch	785 bytes

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

Attachment	Size
id_uniqueness_patch.diff	1.72 KB

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

Attachment	Size
uniqueness_teaser_splitter.diff	1.34 KB

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

Attachment	Size
iduniqueness_20070916A.diff	2.65 KB

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

Attachment	Size
iduniqueness_20070917A.diff	4.24 KB

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.

Attachment	Size
book_edit_pict.png	76.71 KB

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

Attachment	Size
iduniqueness_20070917B.diff	5.32 KB

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

Attachment	Size
iduniqueness_20070917C.diff	1.28 KB

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

Attachment	Size
id_uniqueness.diff	1.19 KB

@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

Attachment	Size
iduniqueness_20070917E.diff	1.2 KB

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

Attachment	Size
iduniqueness_20070917F.diff	1.28 KB

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

Attachment	Size
iduniqueness_20070917G.diff	1.22 KB

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