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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

EugenMayer’s picture

Title: Identical names submit buttons brake form api » Identical names submit buttons break form api

Title :)

+ AFAICS this bug is also in D7

bspellmeyer’s picture

I totally agree that the translation of a form button should under no circumstances have any impact on the forms functionality.

miro_dietiker’s picture

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

Blackice2999’s picture

i 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

Damien Tournoud’s picture

Version: 6.17 » 7.x-dev
Priority: Critical » Normal
Status: Needs review » Active

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

EugenMayer’s picture

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

sun’s picture

Title: Identical names submit buttons break form api » Allow for identical submit button labels
Version: 7.x-dev » 8.x-dev
Category: bug » feature

Extremely significant API change. Requires to additionally set and account for #name.

AFAIK, eff slightly investigated this area already, perhaps he can share some insight.

EugenMayer’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: -Needs documentation

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

miro_dietiker’s picture

Version: 8.x-dev » 7.x-dev
Issue tags: +Needs documentation

sun, 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?)

EugenMayer’s picture

Version: 8.x-dev » 7.x-dev
Issue tags: +Needs documentation

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

miro_dietiker’s picture

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

EugenMayer’s picture

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

AndreU’s picture

Bug?: 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.

sun’s picture

Title: Allow for identical submit button labels » Replace reliance on 'op' with 'triggering_element' to allow for identical submit button labels
Category: feature » task
Issue tags: -Needs documentation
  1. We can try.
  2. This issue is definitely not critical, since, as Damien already mentioned, Form API was designed this way and this behavior exists and was not altered since Form API was born. The chance/probability for encountering the bug described in the OP is the same as in all versions of Drupal since 4.7. The fact that this was triggered by a localized string is unimportant for the actual issue at hand.
  3. To understand why Form API works and was designed this way, this change request needs a lot of input from long-term Drupal masters and Form API maintainers; i.e., chx, moshe, eaton, effulgentsia, Damien, quicksketch, frando, fago, and others. We will also need to heavily advance on our Form API tests.
  4. The actual change from 'op' to 'triggering_element' should be "prepared" to 90% already, as many parts of Form API were heavily rewritten for Drupal 7's AJAX features. http://api.drupal.org/api/function/_form_button_was_clicked/7 and calling/related/referenced functions are the heart of the button detection. The volume of documentation in that and surrounding functions probably clarifies the complexity involved.
  5. Any resulting patch needs to be reviewed by the security team. Additionally, it will likely have to be tested manually in older browsers.

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

sun’s picture

Status: Active » Needs review
FileSize
2.19 KB

Raw shot.

sun’s picture

This breaks the testing framework. drupalPost() works with simple button values, too.

Damien Tournoud’s picture

@sun: it should not break the testing framework. But a lot of tests will break because we have 'op' all over the place.

tstoeckler’s picture

Title: Replace reliance on 'op' with 'triggering_element' to allow for identical submit button labels » Allow for identical submit button labels
Category: task » feature
Status: Needs review » Active
Issue tags: +Needs documentation

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

tstoeckler’s picture

Title: Allow for identical submit button labels » Replace reliance on 'op' with 'triggering_element' to allow for identical submit button labels
Category: feature » task
Status: Active » Needs review
Issue tags: -Needs documentation

Sorry, missed a bit of the conversation.

Status: Needs review » Needs work

The last submitted patch, drupal.form-remove-op.15.patch, failed testing.

sun’s picture

@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', and

2) 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. ;)

sun’s picture

aggregator.admin.inc failures need to be resolved via #767894: Add a confirm form for feed delete

tstoeckler’s picture

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

sun’s picture

Since #tree defaults FALSE, most form buttons may even share the same #name, unless we enforce #name = implode(#array_parents, '][') or similar.

sun’s picture

Status: Needs work » Needs review
FileSize
3.88 KB
sun’s picture

Issue tags: +API change

API 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):

  $form['preview'] = array('#type' => 'textfield');
  $form['actions'] = array('#type' => 'actions');
  $form['actions']['preview'] = array('#type' => 'submit', '#value' => t('Preview'));
  return $form;

Button no longer uses 'op' but 'preview' as #name; same as textfield.

sun’s picture

FileSize
15.45 KB

Technically, 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).

Status: Needs review » Needs work

The last submitted patch, drupal.form-op.27.patch, failed testing.

sun’s picture

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

sun’s picture

Anyone?

sun’s picture

Version: 7.x-dev » 8.x-dev

Since no one moved this patch forward, it won't happen for D7.

EugenMayer’s picture

What a pitty.

cesarpo’s picture

I've just got hit by this, subscribing

jdleonard’s picture

Category: task » bug

This is causing me pain right now in d7. Hoping bug status (which seems deserved) will give this a bit more visibility.

star-szr’s picture

Cross-linking with #1279688: Do not use translated button caption to determine clicked buttons., similar problem space but a different solution.

sinasalek’s picture

I usually use the following code to determine the clicked button

$parents_reverse = array();
if (isset($form_state['triggering_element'])) {
  $parents_reverse = array_reverse($form_state['triggering_element']['#parents']);
}
if ($parents_reverse[1] == 'actions' && ($parents_reverse[0] == 'submit')) {
  //do something
}
David_Rothstein’s picture

Title: Replace reliance on 'op' with 'triggering_element' to allow for identical submit button labels » Replace reliance on 'op' with 'triggering_element' to allow for identical submit button labels without having to manually specify a unique #name
Category: Bug report » Task

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

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Rob230’s picture

Surely 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:

$form['my_button'] = [
  '#type' => 'button',
  '#value' => t('My button),
  '#op' => 'my_button',
];
$triggeringElement = $form_state->getTriggeringElement();
if ($triggeringElement['#op'] == 'my_button') {
  // Do the things.
}

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.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.