Motivation
There are some core modules that are using t() function inside conditional code to determine which button was pressed after a form is submitted. Problems:
- Checking against button caption is a bad practice.
- Changing the value of the button via alter hooks generates errors.
- Using t() when is not necessary.
If not defined, the default name of a button is 'op' and form api stores in this value the translated caption of the clicked button.
Example of problematic code: if ($form_state['values']['op'] == t('Save role'))
Proposed resolution
There are two proposed resolutions of this issue.
theborg proposed a resolution based on replacing the op checking by adding a #name value and checking that instead.Ex:
$form['actions']['submit'] = array(
'#type' => 'submit',
+ '#name' => 'create', <== switch on this
'#value' => t('Create new account'),
sun has proposed in #14 that this particular issue be "won't fix" because the cleaner solution here is to make this form logic broken out into custom submit handlers, and not switch on any
Most patches on this thread to this date (June 6, 2014) have been using theborg's solution of simple replacement logic, and have additionally focused on removing a lot of unnecessary calls of the t() function.
Further explanation of theborg's solution:
a) Add #name and use the $form_state['triggering_element']
that form api populates with values of the clicked button. That generates valid html that travels with form and can be used with $_POST.
b) Same as a) but adding a custom value like #op. Field_ui is using this method but can cause some problems when looking at $_POST['op'].
c) Alter form api to force storage of an untranslated string in$form_state['values']['op']
when buttons name is not using the default value:
$info = element_info($form_state['triggering_element']['#button_type']);
if (!isset($form_state['values'][$info['#name']])) {
$form_state['values'][$info['#name']] = $form_state['triggering_element']['#name'];
}
Remaining tasks
1. Create an updated list of affected files and update the below list on the issue summary accordingly.
2. Come to a consensus whether this issue should be addressed by moving from checking #op to #name on the button objects, or whether this logic should be rewritten in new form submit handlers.
3a. If checking #name, continue working on the patches in this issue (latest in #28 at time of this writing) and clean up these issues until all references are complete.
OR
3b. If rewriting to new submit handlers, then start writing new patches, using the list to find the files affected and adding custom submit handlers to the relevant buttons and adding those functions to the code.
Core files affected
1._ user.admin.inc
2._ node.admin.inc
3._ content_types.inc
4._ taxonomy.admin.inc
5._ locale.pages.inc
6._ dblog.admin.inc
7._ aggregator.admin.inc
8._ comment.pages.inc
9._ image.admin.inc
10._ forum.admin.inc
Original report by [ardas]
Issue: http://drupal.org/node/208790
"After I changed a text on "Create new account" button to any other text using form_alter hook (ex: "Register now") admin/user/user/create form stopped to function properly - it doesn't do anything. However, user/register form works"
"A button must have a permanent key (internal short name, no spaces, no capital letters, etc.) which should be used in the code."
Comments
Comment #1
theborg CreditAttribution: theborg commentedFirst patch with a) method. If needed it could be divide by module for easier testing.
Comment #1.0
theborg CreditAttribution: theborg commentedWriting error.
Comment #1.1
theborg CreditAttribution: theborg commentedUpdating with more info.
Comment #2
theborg CreditAttribution: theborg commentedAdded forum_admin file.
Comment #3
theborg CreditAttribution: theborg commentedPatch for option c).
Added code in form api that, in case button's name is not defaulted to 'op', copy untranslated key #name to $form_state['values']['op']
Comment #5
theborg CreditAttribution: theborg commentedRetest with a taxonomy correction.
Comment #6
theborg CreditAttribution: theborg commentedRe-rolling to adapt to new core commits.
Comment #7
droplet CreditAttribution: droplet commentedI hit this problem when at D6 before. sub.
will it able to backport to D7 ?
Comment #8
theborg CreditAttribution: theborg commentedI think so, if it is admitted i'll build a patch for D7.
Comment #9
Gábor HojtsyI agree we need to solve this problem. I don't think overloading 'op' for this is a good idea. Core already has some places where it checks the trigger element, right? Why not expand from there? Too much code on the "API user" side?
Comment #10
theborg CreditAttribution: theborg commentedOk, re-rolling first patch that uses trigger element. Gábor?
Comment #11
YesCT CreditAttribution: YesCT commented#10: translated_button2.patch queued for re-testing.
Comment #13
star-szrCross-linking with #852520: Replace reliance on 'op' with 'triggering_element' to allow for identical submit button labels without having to manually specify a unique #name, similar problem space.
Comment #14
sunThere's actually a third issue somewhere, which replaces all of these form submission handlers with proper, separated form submit handlers; i.e., removing the conditions on the form action entirely.
I searched, but unfortunately, I wasn't able to find it. Perhaps someone else is able to.
I consider this issue as won't fix, because
1) we actually want cleanly separated form submission handlers in the first place
2) #name is something that developers should only have to manually futz with in advanced cases, and is not something that should be specified in many forms (one of the things it breaks is embedding forms into other forms, and similar things)
3) #852520: Replace reliance on 'op' with 'triggering_element' to allow for identical submit button labels without having to manually specify a unique #name
Comment #15
shameemkm CreditAttribution: shameemkm commentedComment #17
YesCT CreditAttribution: YesCT commentedrelated? #1916928: Document that clickLink() is case sensitive
Comment #18
sinasalek CreditAttribution: sinasalek commentedI usually use the following code to determine the clicked button
Comment #19
aaronott CreditAttribution: aaronott commentedComment #20
aaronott CreditAttribution: aaronott commentedForgot to change status.
Comment #22
Gaelan CreditAttribution: Gaelan commentedRerolled!
Comment #24
hussainweb#22: drupal.transalate_button.1279688.22.patch queued for re-testing.
Comment #25
hussainwebI am not sure why was this in the patch:
It should have changed the comparison line. I am fixing it in the attached patch.
I also grepped and found another use of $form_state['values']['op'] in locale.pages.inc. I am fixing that in the patch too. I also fixed certain places where #name was not being set. Let's see how it goes.
Comment #26
ben.kyriakou CreditAttribution: ben.kyriakou commented#25: drupal-transalate_button-1279688-25.patch queued for re-testing.
Comment #28
hussainwebThe patch is greatly reduced as this comparison was no longer necessary for many of those forms. Instead, some of the forms used #submit handlers directly, or through inheritance, which removed the code to define action buttons from these modules entirely. I have attached a patch which includes the remaining few checks.
Comment #29.0
(not verified) CreditAttribution: commentedUpdating
Comment #30
mgiffordWhat happened to core/modules/aggregator/lib/Drupal/aggregator/Form/CategoryAdminForm.php?
That and all 3 chunks of this patch failed to apply to core/modules/dblog/dblog.admin.inc
There is one chunk in CommentController.php that is still relevant, but a lot has changed since September it seems.
Comment #31
YesCT CreditAttribution: YesCT commentedhttps://feeding.cloud.geek.nz/posts/querying-deleted-content-in-git/
led me to:
$ git log -- core/modules/aggregator/lib/Drupal/aggregator/Form/CategoryAdminForm.php
the most recent commit is:
#2127725: Remove category handling from aggregator
Comment #32
YesCT CreditAttribution: YesCT commentedpatch seems to have gotten smaller and smaller during the rerolls.
(would have been helpful to include a comment when posting a rerolled patch that explained what the change was, like: code removed, code moved, code refactored already to triggering_element #name pattern, ...)
next steps here are some or all of:
the issue that removed them, explains that category for aggregator is moved to contrib.
so it's not a case where we have to find where that code went in core, and redo the changes in said new location.
Comment #33
Unitoch CreditAttribution: Unitoch commentedI'm at the Austin code sprint and I'm going to work on the second and third tasks outlined in #32.
Comment #34
Unitoch CreditAttribution: Unitoch commentedI've Updated the issue summary.
First step is to get consensus here before spending further effort rolling patches.
My opinion jives with sun's. The cleanest solution here is probably to rewrite these to be custom submit handlers on the button, rather than checking any kind of attribute on the button.
Comment #35
Unitoch CreditAttribution: Unitoch commentedComment #36
botrisRemoving Novice tag, as there needs to be a consensus on #14 if this "won't fix", which is not an novice task.