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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

theborg’s picture

Status: Active » Needs review
FileSize
23.03 KB

First patch with a) method. If needed it could be divide by module for easier testing.

theborg’s picture

Issue summary: View changes

Writing error.

theborg’s picture

Issue summary: View changes

Updating with more info.

theborg’s picture

FileSize
24.88 KB

Added forum_admin file.

theborg’s picture

Issue tags: +core
FileSize
24.65 KB

Patch 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']

Status: Needs review » Needs work

The last submitted patch, translated_button_c.patch, failed testing.

theborg’s picture

Status: Needs work » Needs review
FileSize
25.1 KB

Retest with a taxonomy correction.

theborg’s picture

FileSize
25.08 KB

Re-rolling to adapt to new core commits.

droplet’s picture

I hit this problem when at D6 before. sub.

will it able to backport to D7 ?

theborg’s picture

I think so, if it is admitted i'll build a patch for D7.

Gábor Hojtsy’s picture

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

theborg’s picture

FileSize
25.3 KB

Ok, re-rolling first patch that uses trigger element. Gábor?

YesCT’s picture

Issue tags: -core

#10: translated_button2.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +core

The last submitted patch, translated_button2.patch, failed testing.

star-szr’s picture

sun’s picture

There'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

shameemkm’s picture

Status: Needs work » Needs review
FileSize
12.92 KB

Status: Needs review » Needs work

The last submitted patch, translated_button2-reroll.patch, failed testing.

YesCT’s picture

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
}
aaronott’s picture

aaronott’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

Forgot to change status.

Status: Needs review » Needs work

The last submitted patch, translated_button2-1279688-19.patch, failed testing.

Gaelan’s picture

Status: Needs work » Needs review
FileSize
5.96 KB

Rerolled!

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, drupal.transalate_button.1279688.22.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
Issue tags: +Novice
hussainweb’s picture

I am not sure why was this in the patch:

     Drupal::service('plugin.manager.block')->clearCachedDefinitions();
   }
   if ($form_state['values']['op'] == t('Delete')) {
+
     $title = $form_state['values']['title'];
     // Unset the title.
     unset($form_state['values']['title']);

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.

ben.kyriakou’s picture

Issue tags: -Novice

Status: Needs review » Needs work
Issue tags: +Novice

The last submitted patch, drupal-transalate_button-1279688-25.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
4.35 KB

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

The last submitted patch, drupal-transalate_button-1279688-28.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updating

mgifford’s picture

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

YesCT’s picture

https://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:

commit 2e8b1a44ca68506f54fdb2d250c8298490d7a8aa
Author: webchick <webchick@24967.no-reply.drupal.org>
Date:   Fri Dec 6 15:32:13 2013 -0800

    Issue #2127725 by ParisLiakos: Remove category handling from aggregator.

#2127725: Remove category handling from aggregator

YesCT’s picture

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

  • (easy) make a new patch without those category hunks.
    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.

  • (more difficult) evaluate each of the points for wont fixing this in @sun's comment #14 (might include reading all the comments, and could include updating the issue summary https://drupal.org/contributor-tasks/write-issue-summary)
  • (medium) find all the places in core where this pattern still exists. and post a grep (or awk script, etc) that finds them.
Unitoch’s picture

I'm at the Austin code sprint and I'm going to work on the second and third tasks outlined in #32.

Unitoch’s picture

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

Unitoch’s picture

Issue summary: View changes
botris’s picture

Issue tags: -Novice +Amsterdam2014

Removing Novice tag, as there needs to be a consensus on #14 if this "won't fix", which is not an novice task.

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.

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.