I think that issue is known for a while, but iam not sure. If you have a form with to identical names submit buttons the FAPI code will get consfused when it tries to detect which button has been pressed later.
The form api trys to detect the button to run the submit / validation methods of this button.
Why is this critical?
- Well Drupal is used more and more as an application framework where forms or parts of forms are included. You cannot garantee at all, that there are no identical named buttons
- Due to ahaha / CCK you kind of have a garantee that you will end up with 2 submit "save" buttons (depending ont the visual context for the user though )
- Due to the l10n_update translations will get much more spreaded. In this cases, translations can break systems by accidently translating a button of to different modules used on the node form (or somewhere else). So actually translators have to verify the "uniqueness" of a button - well thats simply impossible to maintain
Proposal:
- Form elements have a xpath in the assoc array defined by the FAPI. This path is what we will use as the unique identifier for the button
- Those identifier is used for the name value of the button ( which is never shown to the user )
- The identifier is unique by definition. If to elements have the same xpath - it is the same element.
- So a $form['cck']['filefield']['save']['#type'] ='submit'; will have the identifier : 'cck/filefield/save'
Well lets give an example:
We have following FAPI defintion
$form['cck']['filefield']['save'] = array( '#type' => 'submit', '#value' => t('Save'));
$form['save'] = array( '#type' => 'submit', '#value' => t('Save'));
$form['latch'] = array( '#type' => 'submit', '#value' => t('Save'));
We will end up with the following html-code
<input type="submit" name="cck/filefield/save" value="Save">
<input type="submit" name="save" value="Save">
<input type="submit" name="latch" value="Save">
You see that we can also handle submit buttons on the same level ( deepness ).
I think it should be clear that we cannot at all base the fapi on the label which can be changed so easily and intuitve (translation, string override, form_alter)!!
In the backend we actually use the xpath to identify the button which has been pressed ( using $post and the xpath of the button ). Then we restore the old name of the button, run the submit / validation handlers. Cant see how this actually break the current systems, should only affect the backend. This is totaly internal API AFAICS.
There might be cases where people use JS and the button "name" to attach something to the button, this will break. But this is ok, as this is bad by design. They should use the id here, if they want to identify a button.
Settings this on needs review to get any response on the proposal. If there wont be any cons i will provide a patch.
Comment | File | Size | Author |
---|---|---|---|
#27 | drupal.form-op.27.patch | 15.45 KB | sun |
#25 | drupal.form-op.25.patch | 3.88 KB | sun |
#15 | drupal.form-remove-op.15.patch | 2.19 KB | sun |
Comments
Comment #1
EugenMayer CreditAttribution: EugenMayer commentedTitle :)
+ AFAICS this bug is also in D7
Comment #2
bspellmeyer CreditAttribution: bspellmeyer commentedI totally agree that the translation of a form button should under no circumstances have any impact on the forms functionality.
Comment #3
miro_dietikerSubscribe.
This situation also leads to stupid situations, when a form_alter tries to change a button text. A thing that seems absolutely trivial and do'able.
Presentation text MUST be completely outside application functionality.
Comment #4
Blackice2999 CreditAttribution: Blackice2999 commentedi totally agree too. Yesterday i runned in this problem. See here: http://drupal.org/node/684426#comment-3191128
The second problem that i see is in combination with l10n_update + localize.drupal.org its possible to destroy many pages i need only to approve the button "Upload" from Filefield to "Speichern" (german) and we can see how many pages will be run in this problem... this is a big problem.
regards
Dennis
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe fix bugs in the most recent Drupal version first. Bumping to Drupal 7.
Other then that, it sounds like a good idea. I disagree on the critical: we shipped 10+ versions of Drupal with this inconvenience, it would not be the end of the world to ship on more.
This said, it's probably now or never because it will be a fairly significant API change, so I doubt we will be able to backport it to Drupal 6.
Comment #6
EugenMayer CreditAttribution: EugenMayer commentedI did not confirm this in Drupal 7 100%, this still is to do Damien. Did you do so? ( just to be sure we dont waste time of the 7 devs).
For the critical part: I think this is pretty critical and leads also contributors on the wrong patch how to handle submits on buttons. With the rize of the Localisation server this issue will become the blast.
For the implementation, i dont think it will touch the normal API the devs see or the output. I think this is 100% internal processing and can be changed without to much problems. I pretty much think this can be handled completly behind the "Interface"/curtons. But well thats still to discuss.
Comment #7
sunExtremely significant API change. Requires to additionally set and account for #name.
AFAIK, eff slightly investigated this area already, perhaps he can share some insight.
Comment #8
EugenMayer CreditAttribution: EugenMayer commentedIts only a significat change for people which did NOT use the API ( checking for the button label on 'global' submit handlers instead of attaching submit-hanlders on buttons themself).
It would be rather intersting to see if the "Speichern" string in l10n_update would get approved and seeing ~ 50.000 german Drupal-Installations break. Would be interesting how you will start to argue here afterwards, if thats a "feature request" or mayb "a smaller bug".
If we cannot change the internal processing of the submit button without influencing the Form "API" .. how are we able to call it API at all? I mean, is there a public interface or not?
What we are about to change is _ONLY_ the _name_ of the submit button - nothing more. Everything else will stay exactly the same.
Well if this should stay a 'feature request' please rather close it or mark it "by design" or place it on "Drupal 10 feature request". With the upcoming Localization server and drupals "i18n" effort we will properly see drupal 7 failing 2 years in spreading localisations because of this bug. just to wait for the "feature" to apear in Drupal 8.
Comment #9
miro_dietikersun, damien, eugen
Introducing this capability doesn't mean we need to break everything not considering it..
Obsolete code could just still test for the label translated, while fixed code will rely on the button name.
So why not trying to fix it in D7 context and push this issue?
Documentation issue:
- Document limitation of D6 and D7 fapi
-- No identical button labels allowed
-- Don't change button labels in form_alter
(any further limitation to document?)
Comment #10
EugenMayer CreditAttribution: EugenMayer commentedMiro you _cannot_ write any documentation for the translation case or CCK. Lets say we have to CCK field one for user relation one for file attachments.
Both buttons to add something are named "add". One is for "add user", oder is for "add attachment". Here you go, broken. Eventhough both CCK fields are _independent_ of eachother and are implemented for themeself. What you would need to put into documentation is a force of namespaces for buttons.
So every button is called "Entity action", in our case "add user", "add file". That kind of kills any usability approach and "context" placing, but well that would work out for all installations.
And thats not a fix, thats a dirty dirty hack.
With this open "feature" you cant consider Drupal to be i18n at all, something other CMS have in core already. We will wait quiet some time (until 8) until we can really push this of.
Just keep in mind that translating the action buttons is one of the _most important part of a translation_. So not beein properly translateable this beats the i18n of the UI.
Comment #11
miro_dietikerSo Eugen you/we need to decide the category of this issue.
Either it is a feature request addressing D8 (due to evolution of D7).
Or this is a bug and we'll address D7 and even D6 depending its severity and priority.
I fully understand: D7 feature requests can't be considered at this time.
The documentation of the technical limitation will differ for each of the cases. And sure - even if it's a bug - in case it won't be fixed in D6 we much more need to add it to the documentation of D6. This kind of issue needs a prominent place to describe. If not, we'll waste tons of developer time.
Comment #12
EugenMayer CreditAttribution: EugenMayer commentedI see your point Miro. Well for me this is a bug without a doubt. And this bug seems to be included in d7 and d6 and its critical.
Iam not at all trying to push a feature-request into D7 - we have a string freeze there so there is not even a discussion. Iam just seeing that one developer comes arround and gives this issue a completely new category.
- D8
- Feature request
- normal
This kind of simply kills this issue. Nobody will even look into it and considering if this category is right. Sun just did it without further discussion - something what i would expect on this issue at least.
Comment #13
AndreU CreditAttribution: AndreU commentedBug?: Yes, as some already pointed out: It is foreseeable that this will cause damage in the future if Drupal continues to support i10n.
Critical?: Not at first sight, as it is "only" critical for Drupals i10n capability. So if Drupal drops i10n this issue can be closed or stay as a feature request. If Drupal likes to continue i10n support, this is critical.
Comment #14
sunI'm not really eager to perform this change for D7, but it's probably never the right time to change this; so if there will be sufficient work/momentum on this issue and buy-in from the people mentioned above, then it might be possible.
Comment #15
sunRaw shot.
Comment #16
sunThis breaks the testing framework. drupalPost() works with simple button values, too.
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commented@sun: it should not break the testing framework. But a lot of tests will break because we have 'op' all over the place.
Comment #18
tstoecklerI just tried this out, and EugenMayer you are not correct.
I had a file field and an image field on a node (both having 'Upload' buttons) and everything worked quite fine. There might be edge-case scenarios to be considered here, but this is *definitely* not critical.
Comment #19
tstoecklerSorry, missed a bit of the conversation.
Comment #21
sun@tstoeckler: In which version of Drupal did you test?
It's possible that I'm mistaken, but IIRC, we had (or still have?) special processing code for multiple AHAH/AJAX buttons of the same field/type/value within a form -- not sure where I've seen that though.
--
Is it this code?
http://api.drupal.org/api/function/field_multiple_value_form/7 defines the "Add another item" button,
1) overriding
'#name' => $field_name . '_add_more'
, and2) additionally setting a
'#field_name' => $field_name
property,so http://api.drupal.org/api/function/field_add_more_submit/7 is able to identify and work with the triggering_element/clicked_button... nope.
Actually looks like removing #name from system_element_info() is all that needs to be done for the actual change here. Most probably, plus plenty of secondary changes of code relying on "op" - as the test results prove. ;)
Comment #22
sunaggregator.admin.inc failures need to be resolved via #767894: Add a confirm form for feed delete
Comment #23
tstoecklerLatest HEAD. Now I have neither looked into the code, nor the patch, nor do I have any knowledge about the code involved, but I know that it works.
I did a minimal install + fiel_ui, file, image; added a content type with an image field and a file field; added a node with both an image and a file. No problem.
Comment #24
sunSince #tree defaults FALSE, most form buttons may even share the same #name, unless we enforce
#name = implode(#array_parents, '][')
or similar.Comment #25
sunComment #26
sunAPI change, as #name of buttons are differently populated into $form_state['values'], which might clash with identical key of other form element. I.e., this might have worked before (although it's silly):
Button no longer uses 'op' but 'preview' as #name; same as textfield.
Comment #27
sunTechnically, all of those 'op' conditions should be directly replaceable. So let's try this. All of this code actually shouldn't exist (but use different #submit handlers instead).
Comment #29
sunPassing this issue/patch off to the original change requesters.
Remaining todos:
1) Figure out and fix what's wrong with Field's add more button and that field_add_more_submit().
2) Double-check that WTF in aggregator_form_opml_submit().
3) Fix those other tests.
Comment #30
sunAnyone?
Comment #31
sunSince no one moved this patch forward, it won't happen for D7.
Comment #32
EugenMayer CreditAttribution: EugenMayer commentedWhat a pitty.
Comment #33
cesarpo CreditAttribution: cesarpo commentedI've just got hit by this, subscribing
Comment #34
jdleonardThis is causing me pain right now in d7. Hoping bug status (which seems deserved) will give this a bit more visibility.
Comment #35
star-szrCross-linking with #1279688: Do not use translated button caption to determine clicked buttons., similar problem space but a different solution.
Comment #36
sinasalek CreditAttribution: sinasalek commentedI usually use the following code to determine the clicked button
Comment #37
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedRetitling this a bit to clarify the issue.
Moving away from 'op' entirely might be too much of an API break for either Drupal 7 or 8 at this point, but see some discussion in #1342066-24: Document that buttons with the same #value need a unique #name for the form API to distinguish them, or change the form API to assign unique #names automatically for other possible approaches.
Comment #43
joachim CreditAttribution: joachim as a volunteer commentedSomething we could possibly do sooner: #2943055: throw an exception is elementTriggeredScriptedSubmission() detects multiple possible triggering buttons if there is more than one AJAX button with the same label.
Comment #45
Rob230 CreditAttribution: Rob230 as a volunteer and at CTI Digital commentedSurely there is something better and cleaner to use than the translated text on the button to determine what was pressed. I think buttons should have a property that we can check, something like:
You can't use
['#attributes']['id']
because this gets random stuff appended to it during AJAX, and['#name']
always appears to be 'op' regardless of which button was clicked.