Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Follow up from #384992-64: drupal_html_id does not work correctly in AJAX context with multiple forms on page. See the demo module included there. The following code in form_builder() is the problem.
if ($form_state['programmed'] || (!empty($form_state['input']) && (isset($form_state['input']['form_id']) && ($form_state['input']['form_id'] == $form_id)))) {
$form_state['process_input'] = TRUE;
}
It needs to match on #build_id either instead of or in addition to $form_id.
Comment | File | Size | Author |
---|
Comments
Comment #1
sunThat OP sounds like this patch. Will likely fail though ;)
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedThis fixes it and the demo module works if
$form_state['cache'] = TRUE;
is added to myform(). @rfay: can you please convert your excellent demo module to a test?Comment #3
sunHm. Why can't we make a uncached form build re-use the form_build_id in $_POST, if existent?
Comment #5
effulgentsia CreditAttribution: effulgentsia commentedRe-use what form_build_id? The problem is that drupal_get_form($form_id) is being called multiple times during the page request, for different forms, but with the same $form_id (see the demo module). But we want 'process_input' being TRUE only for the form that was submitted. $form_id doesn't identify it uniquely enough. So we need to use #build_id, but that only has meaning if it was cached. If we default forms to reuse the build_id that's in $_POST, then all of the drupal_get_form() calls will end up with the same build_id, and that makes no sense.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedBTW: I agree that #2 is ugly, both in terms of the new $form_state key it introduces, and because it requires form authors to set $form_state['cache'] when $form_id won't be unique for the page (not at all intuitive). I see this as another problem of using the word 'cache' to mean 'persist': #512026: Move $form_state storage from cache to new state/key-value system, but even a terminology change wouldn't really help here as there are simply a lot of interrelated things that happen as part of form "caching": $form is persisted AND $form_state is persisted AND $form_build_id identifies a form where $form_id isn't sufficient (this issue) or can't be trusted (the reason ajax_process_form() sets $form_state['cache']).
So, alternate ideas for how to fix this would be most welcome!
Comment #7
rfayDespite my faulty recollection, this did *not* work in D6 as is. (I backported the bug demonstration module.) Apparently my elaborate dance to work around the form_clean_id() issue also worked around this. I only *encountered* the problem in AHAH context and thought it was strictly a form_id problem.
Working on a test and I'll see if I can look at why the tests failed as well.
I do think that making this work will remove another major WTF. This is a *very* common use case.
Comment #8
sunNot wanting to decrease emotions and passion about this issue :), but in my entire lifetime as Drupal developer thus far, I can't recall to ever have wanted the identical $form_id to appear more than once on a page. Yes, my grammar needs work. But this still sounds like an edge-case to me.
Comment #9
rfay@sun: This is done in core (I think) on the triggers admin page. It's a no-brainer in E-commerce: You've got an add-to-cart form next to each item for sale on a page. It makes sense to have them all be the same form. That's what's going on with amazon_store.
This also happens in other contexts. For example. you have a page that has a form in it. The same form also happens to appear in a block on that page.
It really should have sane behavior. Yes there are complicated ways to work around it. And probably there's a "best practice" workaround that I don't know about yet.
Comment #10
rfayImagine my surprise when I wrote the test and it passes with the patch... and without it.
Apparently setting $form_state['cache'] = TRUE makes this a possibility without any changes. Hmm.
This patch is the test only, but has $form_state['cache'] = TRUE.
Edit: It doesn't actually work in my application, though. Too late tonight to figure out why.
Comment #11
effulgentsia CreditAttribution: effulgentsia commentedI'm not sure why the #10 patch passes. It shouldn't. Something else is going on that's weird.
Anyway, I thought of a way to fix this issue without relying on the form being cached. We could change the hidden input field for 'form_id' to have the value of $form['#id'] instead of $form['#form_id']. Because of #384992: drupal_html_id does not work correctly in AJAX context with multiple forms on page, #id uniquely identifies the form. There's a few details to work out to make that work, but just wanted to share that as an idea: I'm not sure when I'll next have time to pursue it.
But, one problem that immediately comes to mind with that idea is that drupal_process_form() calls
drupal_static_reset('drupal_html_id');
. This only makes sense if all form processing for the page occurs before all rendering for the page, but that's not always the case, so I'm hoping there's a way to remove that while still addressing whatever that line is meant to accomplish. That line also causes problems if there are multiple instances of the same form, and one of them triggers a validation error.Comment #12
rfayAccording to the commits in chx's multiform module he made that work with multiple forms with the same form id.
Comment #13
effulgentsia CreditAttribution: effulgentsia commentedThis patch is a variant of what chx has in the multiform module. I reworked and cleaned up the test from #10, ensured it fails in HEAD, and passes with this patch.
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedSame patch, fixed file name to not be misleading.
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedYay, bot!
Comment #17
rfayI tested this in a number of contexts, including the original code that gave me grief, and it seems to solve all issues.
+1
Comment #18
rfayI extended my example to include #ajax, and it worked. The forms have to be aware enough to make #ajax['wrapper'] unique, but with that, it worked perfectly. I believe that the underlying original issue had to do with D6 AHAH forms and the form_id.
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedRe #18: adding a #ajax to a form results in $form_state['cache'] being set by ajax_process_form(), which due to a separate bug in drupal_build_form()'s test for when to retrieve from cache makes things appear to work, but it's a misleading illusion. This patch will need more tests added to uncover that illusion and fix the bug.
Since there are several inter-related problems being discovered, I'm trying to merge them into this one issue. So, this patch includes #260934-84: Static caching: when drupal_execute()ing multiple forms with same $form_id in a page request, only the first one is validated and #799356: _form_set_class() is too aggressive in assigning the 'error' class. I will work on additional tests to demonstrate the bugs being fixed.
Leaving as "needs review" to get a bot check, but really, "needs work".
Comment #20
effulgentsia CreditAttribution: effulgentsia commentedCool. Bot likes it so far. I need to add more tests though.
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedWhile this issue isn't critical, it touches on deep enough FAPI internals that I think it deserves the "D7 Form API challenge" tag, lest we forget about it.
Comment #22
Damien Tournoud CreditAttribution: Damien Tournoud commentedSorry boys, but this really sounds like a feature request. Having several forms with the same form_id on the same page *never* worked, neither in Drupal 5 and below (where it couldn't possibly work because the form cache wasn't introduced), nor in Drupal 6.
I don't see a big need for this, either. In the "add to cart" use case, we already generate a unique form_id per "add to cart" form. Do you really believe it's better to force every of those form on a page (there could be hundreds of them) to store its state in the form cache? :)
Comment #23
rfayThis worked (except with AHAH) in Drupal 6.
Comment #24
JacobSingh CreditAttribution: JacobSingh commentedSince there is no documentation that you can't do this, and it causes the form API to behave badly if you do it, I think this is a bug. In fact, the side effect is that validation doesn't run on the 2nd form and you may not know it. That's pretty bad IMO.
-J
Comment #25
sunIt's neither really a feature nor really a bug, it just was never considered to be required.
IMO, this belongs into drupal_build_form(). drupal_get_form() is just a convenience helper to ease handling of most forms.
Also. If I'm going to want to use the same form multiple times on the same page, then I will only want to do that, because I need that form with different arguments, right?
In other words:
$form_state['build_info']['instance'] = hash('sha256', $form_state['build_info']['args']);
At least, I fail to see a use-case for outputting the _identical_ form multiple times. (and that should actually work already)
41 critical left. Go review some!
Comment #26
JacobSingh CreditAttribution: JacobSingh commentedyeah, you'd need different arguments. Most common case IMO is if you want a form where you want to edit 5 nodes at once, they would all have the same form ID unless you knew that this broke in some silent deadly way and implemented something in hook_forms...
Also common perhaps is a login form you want to show at the top and bottom of a page, or perhaps a "rate this article" form that goes along with every article? There are really lots of cases for showing the same form multiple times on a page.
I still say it's a bug because the system breaks in a silent nasty way and nothing in the docs say you can't do this + modules could just interact and do it by accident in some cases. But if you insist on changing the type, I'm not going to argue.
Comment #27
rfayI think it's a bug too, of course.
Comment #28
sunBasically, this. Code comments explain the situation and idea.
re #26: The login form at top + bottom already works today. All other examples should work with this patch.
Comment #30
sunTested manually. This successfully fixes the bug uncovered in #845774-31: Regression: Anonymous users can post comments in the name of registered users
Any clues why Drupal installation fails?
Powered by Dreditor.
Comment #31
andypostStrange, installation final step form does not submits. Second attempt submit works.
I mean _install_configure_form() does not submits from first attempt
Comment #32
effulgentsia CreditAttribution: effulgentsia commentedNot sure if we'll manage to fix all of the bugs related to multiple forms with the same $form_id coexisting in the same page in time for D7. I hope we do, but if we don't, #757154: Base form_id via hook_forms() not taken into account for #validate, #submit, hook_form_FORMID_alter() makes the workaround currently employed by D6 contrib modules of using hook_forms() to map unique form ids to a particular base form, much easier.
Comment #33
sunTested manual installation. It indeed fails for yet unknown reasons when trying to submit the install_configure_form - it gets simply displayed again, without any submitted values.
The logic is triggered by this code, but that's not the cause:
So 'executed' is not set, but 'executed' is merely:
Whereas 'submitted' is merely:
So it must be 'process_input', which happens to be altered here, and which should be fixed in attached patch. :)
Comment #35
sunok, I'm officially baffled now. The patch failing again to install means that only my second attempt to submit the install_configure_form worked out, while the first does not... (?)
Comment #36
sun$install_state is specially prepared during installation.
Comment #38
sunForm rebuilds might be an issue though... trying to cope for them.
Comment #40
sunAlthough badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).
Comment #41
Dave ReidMarked #1027818: Login form being colored when the name is missing as a duplicate of this issue.
Comment #42
bjaspan CreditAttribution: bjaspan commentedI stumbled upon this problem and proposed a different approach in #1057968: Support dynamically generated form ids. The approach here embeds unique information about the form in the 'instance' property, passed as a hidden value through the browser. My approach embeds unique information in the form_id and offers a direct method of specifying the correct form builder function. I have not written a patch, but I suspect that my approach is simpler though it does not protect against accidentally putting the same form (with the same id) on a page twice and then having the wrong form processed and/or the second form submitted without validation.
Does anyone want to argue that one or the other approach is definitely better?
Comment #43
drm CreditAttribution: drm commentedFYI, I'm running into an issue with Commerce where a View has the same product in it multiple times, and thus multiple versions of the same add-to-cart form. When you update what we always called an attribute, the FAPI ajax causes some of the duplicated add-to-cart forms to disappear. Multiple copies of the same product are showing up because the View is actually displaying a different node type that has references to products, and with this other content type, more than one node can reference the same product.
All we want is for the add-to-cart forms to stop disappearing. I'm not sure if this is the exact same bug, but it seems so.
Comment #44
attiks CreditAttribution: attiks commentedsubscribing for #1239910: META: tracking other issues about validation
Comment #45
Anonymous (not verified) CreditAttribution: Anonymous commentedI ran into this issue today as well. I'm displaying multiple add to cart forms on the same page, and when one gets submitted, it is the first instance of the form that is submitted, not the one I clicked.
If we could get this put into Drupal 7, that would be a real lifesaver :)
-Leighton
Comment #46
rfay@wildkatana I accomplished exactly the same thing in Amazon Store using hook_forms(). Quirky, but do-able. I'd rather see this one actually solved though.
Comment #47
Anonymous (not verified) CreditAttribution: Anonymous commentedI was just about to post the same thing rfay. The hook_forms() functionality lets you create multiple versions of the same form with different ids, that's what I just did as well. If this patch were committed, we wouldn't need to do it, but at least this bug isn't a showstopper.
-Leighton
Comment #48
andypostClosely related issues #1575060: ajax_html_ids are broken for forms with file element (encoding=multipart/form-data) and #799356: _form_set_class() is too aggressive in assigning the 'error' class are both in D8 so what's left here?
Comment #49
arnoldbird CreditAttribution: arnoldbird commented#42 works for me.
Comment #50
jibranNo D8 patch yet.
Comment #51
Fabianx CreditAttribution: Fabianx as a volunteer commentedThat is a major bug in my book and not a normal task ...
Comment #52
Wim Leers#51: Please clarify why.
Comment #53
effulgentsia CreditAttribution: effulgentsia at Acquia commented>4 years since #33, so unassigning. I don't see any way for this to be backported to 7.x, so removing the "D7 Form API Challenge" tag: we missed that boat.
I agree with this being a bug, especially for D8, because:
- You can place multiple instances of a block on a page, so for any block that presents a form, this can be triggered via that.
- In #2110951: Remove hook_forms(), we removed hook_forms(), so I'm not currently clear on how a contrib module could work around this problem in the way it could have in prior versions.
I think something along the lines of #38 has promise.
Comment #54
effulgentsia CreditAttribution: effulgentsia at Acquia commentedTagging "Contributed project blocker", because it would be great to investigate / get feedback on whether there are contrib modules hard blocked on this. For a module that controls its own forms (e.g., Commerce's "add to cart"), the module can implement its own dynamic getFormId() method. But I wonder if there are contrib modules that build some kind of multi-form-instance functionality around forms they don't control, for which they could have used hook_forms() in 7.x, but can't in 8.x.
Comment #55
BerdirThe way a module can work around it is by implementing something that returns a unique string from the getFormId() method. We no longer need the hook as we don't call the form by the form id anymore, we call it by the class.
See SimplenewsSubscriptionBlock::build() for an example how we solved it with simplenews subscription forms in blocks. (wouldn't be surprised if some of the recent/upcoming) form changes will require some changes there.,..
I don't really see a way around enforcing unique form ID's but maybe all the form submission changes do allow something here, I haven't been following them closely.
There were two challenges in making it work for simplenews:
a) Injecting a value into the form class that we can use *before* getFormId() is called. As you can see, that does require quite a bit of code right now. Further complicated in our case by the fact that it is an entity form.
b) The fact that blocks no longer have a (reliable) unique ID associated to them. We have to generate/maintain a UUID ourself in our configuration to actually have something to identify a certain block form. I've discussed with @timplunkett a long time ago that it would make cases like this a lot easier. That unique ID has to exist anyway (the config entity ID for core blocks, the UUID for page manager blocks), it's just not accessible.
Comment #56
andypostMasquerade module affected too
Links to form could exist in blocks and at the same time the form could be loaded with modal dialog...
Comment #57
andypostAlso more then one comment form could exist same time on one page so modules like ajax_comments could have issues
Comment #58
Dave ReidIt would be nice if File Entity didn't have to rely on the hacky Multiform module in order to support editing of multiple file entities after doing a bulk upload.
Comment #59
Berdir@Dave Reid: I'm fairly sure that's a different problem, your use case is having multiple, usually separate entity forms as a single form that you can submit together. So there really is only a single form. This is about having the same (configurable/re-usable) form multiple times on the same page, without knowing about the others. So kind of the opposite use case..
Comment #60
tim.plunkettComment #63
Andre-BFollowing up on https://www.drupal.org/node/766146#comment-10018657:
We are running into this issue at the moment with https://www.drupal.org/project/votingapi_widgets
Inside of VotingApiWidgetBase we are calling entity form builder for a vote entity.
These forms will be printed separately on the requested page. Examples are:
In the last case core currently takes the first form it renders for all of those ajax requests. I am trying to investigate further and will have a look on the patch if it fixes that issue for us. In our case we could still extend EntityFormBuilder and change the logic of getForm() to add unique form ids. Issue in votingapi_widgets: #2832153: Multiple VotingAPI Widgets on the same page do not work correctly
Comment #64
BerdirYou need to let it generate a unique form ID. You have the entity there, so override getFormId() and return something unique, including the entity type and ID should be a good start, then you'd only get in trouble if the same entity is displayed more than once, but that's then also kind of the same form anyway.
Comment #65
Andre-B@Berdir, thats exactly what I tried and figured out it's not supported. since form_state is not passed onto FormInterface::getFormId in see FormBuilder::getFormId for reference.
To solve this I extended FormBuilder and Injected this custom FormBuilder in an new EntityFormBuilder instance. But this was only possible because I have some control over how I retrieve the form. I think extending FormInterface::getFormId by optional $form_state and passing this over in EntityFormBuilder can help others - or enabling multiple form ids.
Comment #66
tim.plunkettInside your EntityFormInterface::getFormId you have access to the entity object, isn't that enough?
Comment #67
Andre-B@tim.plunkett missed that one. thank you! http://cgit.drupalcode.org/votingapi_widgets/commit/?id=6257f65
Comment #68
BerdirRight. And if that is not enough or if you have a plain form, you can instantiate the form object yourself and then pass in data, e.g. using setters and pass an object to getForm: http://cgit.drupalcode.org/simplenews/tree/src/Plugin/Block/SimplenewsSu...
Comment #71
rp7 CreditAttribution: rp7 commentedI have a use case where I need to display multiple profile forms (https://www.drupal.org/project/profile) on the same page. The first form was always saved, not the one I was editing. Took me a while to find this issue but here we are.
I was able to fix this issue by overriding the ProfileForm handler. It overrides the getFormId() function in order to return a form id based on the entity the form is being built for. Just putting this out here for people needing to do the same for entity types they don't have control over (eg. core/contrib ones).
Comment #73
Shashwat Purav CreditAttribution: Shashwat Purav commentedChanging the wrapper fixed the issue:
E.g.
If there are two forms and has same wrappers as below:
$form['wrapper'] = [
'#markup' => '
',
];
Change one of the wrappers and IDs:
$form['wrapper-form'] = [
'#markup' => '
',
];
Comment #75
markus_petrux CreditAttribution: markus_petrux commentedGenerating different form ids is not ideal because it affects template suggestions... so you have to build custom template suggestions to workaround that.
Comment #78
geek-merlinComment #82
smustgrave CreditAttribution: smustgrave at Mobomo commentedis having multiple forms with a same ID not cause an accessibility issue?
Comment #83
andypostNo, real IDs are different and if it's 2 node forms then development surely added wrappers or other visual notes about which form is for
Comment #85
AnybodyJust ran into this issue in Homebox (3.0.x) development and think I can add a good example from there:
All Homebox portlets (boxes) are separate forms, based on the same form (id). So there can be unlimited number of these "same" forms on the same page.
Technically, they are extending
PluginBase
and implement FormInterface. (The ECA module does similar things and for this software design, this is needed).Here's the class implementation:
When implementing the AJAX functionality, I was wondering, why
always returned the wrong element! It always returned the first buttom from the very first instance of the form on that page.
The interesting part is, that the AJAX call body contained the clicked element as triggering element.
So I tried changing the getFormId() implementation in that plugin from:
to
like in #71 and it works for now! But it cost me hours to find this.
I guess the reason for this is a security check in the form to ensure the AJAX-given triggering element exists.
Perhaps a first step would be to log a watchdog warning in such a case to let the developer know the mismatch?
Hopefully my implementation for the dynamic form ID won't have other side-effects! For now it works...
So this is still an issue in 11.x-dev!
Re #82 see #1852090: Cached forms can have duplicate HTML IDs, which disrupts accessible form labels
Comment #86
AnybodyComment #87
aragon-usr CreditAttribution: aragon-usr commentedHad same problem,
I have multiple same forms on one page generated in loop.
I met the problem when I wanted to inject HttpClient to Form but back then was passing customValue through constructor.
My approach to solve this issue was define Form as service and pass custom id via property setter:
.services.yml
CustomClass:
Form Class:
Maybe it's not ideal, but I think it could be helpful for others.