form_clean_id() doesn't correctly generate unique element IDs when called in an AHAH context.
form_clean_id's job is to make element IDs unique on a page even though there may be many version of the same form on the page. It works in a normal page by looking at all the existing form id's and just generating a new one if it finds a conflict. It uses a static variable to keep track of what it's seen.
However, in the context of AHAH, form_clean_id() doesn't see the other forms, so this approach doesn't work. A new technique for unique form id's will have to be developed, perhaps UUID, or use the form token or something.
TO REPRODUCE: In a drupal page with multiple copies of the same form, process one of the forms in an ahah callback.
WHAT YOU WOULD EXPECT: Form element IDs which are unique on the page, as is the intent of the function. Preferably have the element IDs remain the same as they were previously, but at least have them be unique, as required.
WHAT YOU GET: No change made to the form element id (because form_clean_id sees no reason to alter it, since it knows about no conflicting IDs).
Comment | File | Size | Author |
---|---|---|---|
#64 | drupal_html_id_bug_demo.tgz | 696 bytes | rfay |
#57 | drupal.ajax-ids.57.patch | 28.95 KB | effulgentsia |
#56 | drupal.ajax-ids.56.patch | 29.17 KB | effulgentsia |
#55 | drupal.ajax-ids.55.patch | 29.49 KB | effulgentsia |
#50 | drupal.ajax-ids.50.patch | 25.19 KB | effulgentsia |
Comments
Comment #1
Scott Reynolds CreditAttribution: Scott Reynolds commentedThis happens in D7 as well still. http://api.drupal.org/api/function/drupal_html_id/7 like form_clean_id before it uses a static cache. A static cache is preserved only across the time it takes to generate a page. An AHAH/AJAX callback is a separate page therefore it starts with an empty static cache.
So when caching the form based on form_build_id, we cached along with it all the $seen_ids, it would allow drupal_html_id to start with those ids.
In ajax_get_form
Just have to figure out how to write and pull from the cache. Sortof build up a 'page id'.
Comment #2
Scott Reynolds CreditAttribution: Scott Reynolds commentedrelated #684568: drupal_html_id() should use $drupal_static_fast pattern deals with using the drupal_fast_cache pattern.
Comment #3
Scott Reynolds CreditAttribution: Scott Reynolds commentedExotic idea, do hook_page_alter add in a JS setting that puts the unique cache_id (user session . REQUEST_TIME?) into the js settings and writes the data to the cache table.edit: Decided I didn't like that idea, that was a little strange.
Comment #4
effulgentsia CreditAttribution: effulgentsia commentedSubscribing for now. Will contribute ideas as I take time with this, but I'm also interested in Scott's and other ideas. I like the idea of a page build id, if we can come up with a core-worthy way of doing it. Such a thing would also help #561858: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework as it would allow tracking of js/css already on the page, so only new js/css would need to be sent back. It's a concept I explored in a contrib module for D6: http://drupal.org/project/ahah_page_storage, but I'm not too happy with that particular solution; I think we can do better for D7 core.
Comment #5
rfayChanging title for correctness in D7
Comment #6
sunYes, badly needed. Also required for Dialog module.
Silly idea: see attached patch :)
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedClever. I like it. I'd still like to work on a "page storage" idea for D7, so that only a single page build id needs to be transferred over the network rather than a potentially big string of all html ids, but that can be a future performance optimization issue, whereas #6 allows the bug to be fixed, so we should proceed with that.
Ideas for improvement of #6:
- add same code to drupalPostAjax(), so a test can be written.
- add a test.
- drupal_process_form() calls drupal_static_reset('drupal_html_id'). Ideally it should reset to $_POST['ajax_html_ids']. If we had confidence that system_init() occurred before the first time drupal_html_id() was called, it could just call
drupal_static('drupal_html_id', $_POST['ajax_html_ids']);
, and that would give us what we want. But is that assumption too brittle?[EDIT: or, um, how about removing the code in system_init() and just making drupal_html_id() use $_POST['ajax_html_ids'] as the default value in the drupal_static() call?]
Comment #8
effulgentsia CreditAttribution: effulgentsia commentedAlso, I wonder if there should be an opt-out or opt-in mechanism. Consider a FieldAPI "add more" button. It's not going to render most of the form, but it does form_builder() the entire form. Do we really want all the element #ids to have a suffix? Also, what it does render will replace what is on that part of the page, so even for the part that's rendered, do we want a suffix on ids?
Comment #9
sunre EDIT in #7: yeah, just figured that, too :)
re #8: I have no idea what HTML ids are good for these days (other than JS behaviors attaching to dynamic ids). Thus, no clue. I could live without opt-in/out.
Comment #10
sunActually, yeah, default value makes even more sense. :)
Comment #11
effulgentsia CreditAttribution: effulgentsia commenteds/system_init()/drupal_html_id()/
148 critical left. Go review some!
Comment #12
sunyay, this even works :-D (didn't actually test before)
of course, only stuff that's using drupal_html_id() gets the magic.
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedThis still needs a test added, but how's this so far? A little more complicated than #12, but I think it's clear why.
Comment #15
sunI think I've read the new code + comments up to 3 times now, and can only slowly get behind the reason for that new parsing ;)
I do not understand why we're munging the originally passed id here.... or perhaps, just now I understand, duplicate hyphens would break the explode() during initialization, so we need to prevent that here.
Overall, we can probably move most of the high-level docs contained as inline comments within this function into the function docblock, and rather replace those inline comments with explanations/reasonings for why stuff is done (and not what is done). :)
148 critical left. Go review some!
Comment #17
effulgentsia CreditAttribution: effulgentsia commentedMade tests pass, and tried to improve comments as per #15. I don't think the comments are perfect yet, so suggestions welcome :)
Comment #18
sunajax.js puts the element ids into keys, i.e. an associative array. This here looks like it'd create a indexed array?
Powered by Dreditor.
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedNope. #12 did but I changed that in #14. Because the associative array technique from #12 doesn't really reconstruct $seen_ids correctly. Parsing is needed, and if that's the case, makes more sense to pass in a "plain" array.
Also, see #750008: Add test for HTML ID uniqueness. Quite a few ID uniqueness failures in HEAD. Still lots of code that does not use drupal_html_id(). For just one example, see #710172: HTML ids for blocks should not contain underscores.
Comment #20
RobLoachHmmm, interesting. Could this potentially result in a large amount of IDs being passed back and forth after a while?
Comment #21
sunThe ids are only passed "forth" and not back, which is the fundamental idea. :) So this will result in potentially large $_POST arrays in AJAX requests, but the only limitation here is PHP's post_max_size, which defaults to 8MB, if I'm not mistaken.
Comment #22
Scott Reynolds CreditAttribution: Scott Reynolds commentedMy original idea was to store the ID's in a cache table. This has the advantage of feeling less hacky and could provide a mechanism for knowing what scripts and CSS have already been added to the page. Take this issue as an example: #745468: ctools_ajax_command_scripts() in ctools_ajax_render() causes unwanted scripts to be loaded (and potential js conflicts).
I know its off topic of this patch and I know Im adding some noise, but I think we are passing on an opportunity.
Comment #23
effulgentsia CreditAttribution: effulgentsia commented@Scott: I agree with you (see #7), but not a cache table, and instead a page_state table, just like we need to move $form_state away from a cache table: #512026: Move $form_state storage from cache to new state/key-value system. I think the place to refine the idea of a persisted $page_state for AJAX is #561858: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework, and once we have a working solution of that, we can replace the use of $_POST['ajax_html_ids'] with it pretty easily. But in the meantime, proceeding with $_POST['ajax_html_ids'] lets us work on the other issues here, like the inconsistencies of when drupal_html_id() is called (#9/#19).
Comment #24
sunThis looks RTBC to me.
Comment #25
sunah, we still need to amend this test case with a new test using drupalPostAJAX()
140 critical left. Go review some!
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedMy thought for the test is to enhance drupalPostAJAX() to process the returned 'insert' commands and modify $this->content as ajax.js would. This would enhance our ability to do other AJAX testing too. Then, to add a assertNoDuplicateIds() method to DrupalWebTestCase, which can have similar code as in #750008: Add test for HTML ID uniqueness. Then, a specific test of a page callback that returns a page array containing 2 instances of the same form, where that form includes a simple AJAX-enabled button, and to call assertNoDuplicateIds() after a drupalPostAJAX() of each button: that's basically the test for the OD of this issue. Anyway, I was hoping to have that all done by now, but I haven't gotten to it yet. I plan on getting to it within the next few days, but just wanted to share these thoughts in the meantime. If someone has another idea for a test, by all means go for it, and I can add mine later, either as part of this issue or another one.
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedHere's my work so far. Part of why it's big is because it includes the patch from #656782: ajax_process_form() results in settings being returned for elements that aren't re-rendered as part of the AJAX request inside it: a necessary step for making the test in this patch work.
The basic test is form_test_two_instances(), which returns a page with two 'page_node_form' forms on it. Simple concept. Surfaces many issues. Seems to be working in manual testing, but the simpletest still needs troubleshooting. I'll pick this up again when I have a chance. Not sure yet how quickly that will be.
Comment #29
effulgentsia CreditAttribution: effulgentsia commentedChasing HEAD. This will fail, but it should at least apply.
Comment #31
effulgentsia CreditAttribution: effulgentsia commentedCareless typo. This one should only have the new test fail. That's the one that still needs troubleshooting. As I said in #27, it works manually, so something in the new code that adds round-trip AJAX testing to the test framework is still buggy.
Comment #33
sun#656782: ajax_process_form() results in settings being returned for elements that aren't re-rendered as part of the AJAX request landed.
Comment #34
effulgentsia CreditAttribution: effulgentsia commentedyep, working on a re-roll, cleanup, and trying to troubleshoot the remaining test failure now.
Comment #35
effulgentsia CreditAttribution: effulgentsia commentedStill struggling with PHP's DOMDocument, but here's where I am so far.
Comment #37
effulgentsia CreditAttribution: effulgentsia commentedAnother re-roll for HEAD changes. Only the new test should be failing.
Comment #38
andypostsubscribe, drupal_html_id() was used in simpletest itself but now we removing this in #601398: Simpletest does not allow assigning actions to triggers
Comment #40
effulgentsia CreditAttribution: effulgentsia commentedFinally figured out how to work around DOMDocument troubles: this one should pass!
Comment #41
effulgentsia CreditAttribution: effulgentsia commentedBumping to critical since waiting until Drupal 8 before we're allowed to have multiple node edit forms on a page seems like a really bad idea. We want a contrib module that implements a good AJAXy UI for nodereference fields. Other use-cases too plus HTML validation issues.
Comment #42
sunwow. :)
Let's create a follow-up issue for that and link to it from here.
I think this should be @var DOMDocument
Looks like we should get that other issue in ASAP.
I think this method should be moved into the AJAXTestCase helper.
If I'm not mistaken, then we do not use parenthesis for clone.
139 critical left. Go review some!
Comment #43
andypostAlready fixed in #601398: Simpletest does not allow assigning actions to triggers
Comment #44
effulgentsia CreditAttribution: effulgentsia commentedCleanup for HEAD changes. Also, once I figured out the DOMDocument::getElementById() problem in #40, I realized I could remove the changes to drupalSetContent() from #40, so this patch has less changes to DrupalWebTestCase. This covers most of #42.
From #42 (referring to new DrupalWebTestCase::assertNoDuplicateIds() method):
I disagree. We'll want tests other than AJAX to assert this. For example: #710172: HTML ids for blocks should not contain underscores. IMO, ideally we should assert this after every drupalGet() as in #750008: Add test for HTML ID uniqueness.
Comment #45
effulgentsia CreditAttribution: effulgentsia commentedpriority change was unintentional.
Comment #46
effulgentsia CreditAttribution: effulgentsia commentedWhat's still needed here before it's RTBC?
Comment #47
andypostI tested this and I think it good to go. Any other issues could be solved with follow ups
Comment #48
chx CreditAttribution: chx commentedGood to go. Although it fishes stuff out of POST straight but then those are treated with a ++ so it's casted to int. No security weakness AFAIK.
Comment #49
Dries CreditAttribution: Dries commentedReviewing the patch without reading up on the issue, it is not at all clear what is going on, or why. We still need better code comments.
Can we add a code comment describing why we do this?
Can we add a comment describing why we add the IDs to the POST environment?
Can we add a code comment describing why we do this?
Comment #50
effulgentsia CreditAttribution: effulgentsia commentedThe comment additions requested by #49 only. Straight to RTBC for Dries to review.
Comment #51
Dries CreditAttribution: Dries commentedI think we're really close. One more thing:
I understand IDs need to be unique on any given page. However, the difference between "must not only be unique across this page request" and "not duplicate what is already on the base page" is not clear. What confuses me is "across this page request". What does it mean for something to be "across a single request"? I'm not native English so this might be a stupid language issue ... If so, sorry about that.
Comment #52
sunWell done. Not sure whether there's more we could explain. Even more high-level docs should rather go into a bird-level explanation of the AJAX framework functionalities on a d.o handbook page, I guess.
Comment #54
sunüber-cross-post.
// If this is an AJAX request, then HTML ids must be unique for this request, but also take into account what is already on the base page.
...or similar...
Comment #55
effulgentsia CreditAttribution: effulgentsia commentedThis has the comment tweak for #51. It also includes a change to drupal_rebuild_form() to make the test from #50 pass. And while I was in testPreserveFormActionAfterAJAX() I saw a @todo linking to this issue (that I added from another issue) and decided to address it.
Comment #56
effulgentsia CreditAttribution: effulgentsia commentedBah. #55's change to drupal_rebuild_form() makes no sense. This removes that hunk and instead changes the test and adds a comment explaining it. The only difference this has vs. #50 is comments and modification to FormsRebuildTestCase::testPreserveFormActionAfterAJAX(), but leaving it as "needs review" so that the test change can be reviewed.
Comment #57
effulgentsia CreditAttribution: effulgentsia commentedThis patch cleans up this convoluted issue with the id by using the class instead. We just need a way of targeting the node form and not some other possible form on the page, so using the class is fine.
Powered by Dreditor.
Comment #59
sun#57: drupal.ajax-ids.57.patch queued for re-testing.
Comment #60
sunBack green.
Sidenote: If we would have had this idea some weeks/months ago, then Overlay most probably wouldn't be in core today the way it is. Among other minor issues, this was the major, #1 reason for the decision to go with Modal Frame API as base idea/approach for Overlay. This patch allows to turn pure awesomeness into reality via http://drupal.org/project/dialog, which also affects http://drupal.org/project/wysiwyg. With this patch, I can see real (as in: ) UX improvements drive Drupal's future.
Comment #61
Dries CreditAttribution: Dries commentedI agree that this is ready. Committed to CVS HEAD.
Comment #62
rfayYeah!
The original complaint is an existing issue in D6. I'm going to assume we don't have the energy to pursue that. Certainly not in this exhaustive way.
Comment #63
ksenzeeRe #60: I hadn't started working on overlay yet when the iframe decision was made, but in my experience, loading independent page content into a div inside a page that isn't expecting it causes more problems than a simple id mismatch. We've run into this with the Drupal Gardens Themebuilder, which creates an admin interface at the bottom of the page. It's really, really difficult to keep the base page's CSS from affecting the Themebuilder interface. We eventually decided to put the admin interface into a div -- a decision I was happy about, because I hate iframes with a hot, hot hate -- but there's no way we could have done it if we didn't control every div and span and line of CSS in that admin interface. Placing an entire Seven-themed page inside a div in a Garland-themed page is a recipe for a total theming nightmare. And that's just Garland, which we do control. I can't even imagine doing it with a contrib theme as the base page.
I'm enthusiastic about the dialog project, and I bet wysiwyg will do fabulous things with it. But the overlay is more than a dialog box, and I believe iframes will continue to be the right solution for the problem overlay is solving -- even if I do hate iframes almost as much as blink tags.
Comment #64
rfayI apologize that I wasn't very active in this issue, but my base expectation was this:
Multiple identical forms on the same page would work "out of the box" without any form id mucking and the like.
Today I got my first chance at this, working with upgrading the module that originally inspired this module. But (at least the way I'm using it) my expectations aren't met at all.
The attached bug demo module just creates a page with 5 forms created from the same formbuilder, but slightly different based on an extra parameter.
I expect that clicking the submit of #5 will get its form and form state, etc.
But what happens is that the submit always gets the first form/form state on the page. Just like the old days.
Do I misunderstand our objective in this one?
Again, I apologize for not being more active and obviously didn't grok this adequately.
Comment #65
effulgentsia CreditAttribution: effulgentsia commentedExcellent find! Separate issue: #766146: Support multiple forms with same $form_id on the same page
Comment #66
effulgentsia CreditAttribution: effulgentsia commentedSeveral follow-up issues related to the question of when drupal_html_id() should be used.
#684568: drupal_html_id() should use $drupal_static_fast pattern
#710172: HTML ids for blocks should not contain underscores
#755566: Use of ID within theme_filter_guidelines() breaks HTML validation with more than 1 field item on a page
Comment #67
andypostRegression with file/image ajax uploads #770880: Files and images are not deleted after node saved
Comment #69
rfayI hate to say it, but I think this one may have been a bust. It may even be a bug.
Essentially, it means that HTML IDs are unique on a page, but because they're unpredictable, they can never be used for *anything*. So I don't see how we won anything, and we definitely lost a LOT.
We've all fixed "bugs" that resulted from this one, where css or js was relying on IDs. But is that wrong? Are those bugs? Or is this a bug? And why should the ID of a table change just because more items are added to it (as with the node-add form when you're using the "add-more". )
Comment #70
effulgentsia CreditAttribution: effulgentsia commentedNo, they can't be used for hard-coded CSS or JS. IMO, IDs (except ones in page.tpl.php) should never be used for CSS, that's what classes are for, but I'm willing to hear arguments from a CSS expert as to why I'm wrong on that. IDs in JS make conceptual sense, and it's a shame code needs to be updated to not rely on hard-coded IDs, but I think that's the price we pay to use a system like Drupal that supports very dynamic output in conjunction with the HTML requirement that IDs be unique within a document. But there are things that still use IDs, like Drupal.settings, which is pretty major, and in conjunction with behaviors, is the recommended way to write JS to handle the dynamic nature of Drupal, so I don't think it's fair to say we lost them for *everything*.
It's a shame that the implementation of the field.module "add more" button is to re-render the full field, rather than just to render and return the new item. Perhaps for D8, or in D7 contrib, we can improve this. But given that that's the implementation, then what's returned to the page is a *new* table to replace the *old* table, and therefore, it makes conceptual sense to me that it gets a new ID.