Greetings,

I noticed the following problem which was in Drupal 5.0 and looks still not fixed.

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.

It is quite a critical defect because in 9 of 10 projects clients are not satisfied with default "Create new account" text and would like to change it to something else.

The problem looks to be common - in some cases changing button text breaks form which is bad because form functioning should not be tied with text on the button.

Thanks.

Files: 
CommentFileSizeAuthor
#28 208790_user_admin_28.patch4.84 KBtrrroy
PASSED: [[SimpleTest]]: [MySQL] 40,529 pass(es). View
#24 208790_user_admin_2.patch4.83 KBtheborg
PASSED: [[SimpleTest]]: [MySQL] 32,940 pass(es). View
#23 208790_user_admin.patch4.83 KBtheborg
PASSED: [[SimpleTest]]: [MySQL] 32,926 pass(es). View
#21 208790_and_tests.patch13.56 KBtheborg
FAILED: [[SimpleTest]]: [MySQL] 32,810 pass(es), 103 fail(s), and 44 exception(es). View
#19 208790_user_admin.patch4.42 KBtheborg
FAILED: [[SimpleTest]]: [MySQL] 32,898 pass(es), 26 fail(s), and 19 exception(es). View
#14 drupal-user_admin_op-208790-14.patch644 bytesAgileware
PASSED: [[SimpleTest]]: [MySQL] 26,742 pass(es). View
#2 user_hardcoded_params_a.patch878 bytestheborg
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_hardcoded_params_a.patch. View

Comments

catch’s picture

Status: Active » Postponed (maintainer needs more info)

Why not use the language system or settings.php string array (which is new) to do this?

theborg’s picture

Status: Postponed (maintainer needs more info) » Active
FileSize
878 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_hardcoded_params_a.patch. View

Agree with @catch but hardcoding parameters is not the way to go.

This patch eliminates the t('Create new account') and t('Search') from user_admin function. I tested and it works but more testing is needed to verify that not affects other functionality.

theborg’s picture

Status: Active » Needs review
Gábor Hojtsy’s picture

Well, this would be quite a paradigm shift for Drupal form flows. We have this kind of op = t() check all over the place.

ardas’s picture

This patch looks clever for me. I don't understand why Drupal authors didn't implement this from the beginning.

ardas’s picture

Using button caption to determine which button was pressed is a completely bad idea.
This breaks one of the fundamental programming principle - variable data (especially UI data) should not be used as a key data for functionality.
A button must have a permanent key (internal short name, no spaces, no capital letters, etc.) which should be used in the code.

Does it make sence?

Gábor Hojtsy’s picture

Version: 6.0-rc2 » 7.x-dev

Yes, it makes sense, and it would be great to clean it up in Drupal 7. Unfortunately this is such a paradigm shift for core that it is late to do it right now in Drupal 6. We risk introducing numerous new bugs. Until then, you can try nasty tricks to put back the original button title in a custom validation callback. (I never tried but I'd expect that it works).

ardas’s picture

Sure, no problem.
I would be glad to see this fix in Drupal 7.

Thanks :)

birdmanx35’s picture

Status: Needs review » Needs work

This patch fails against core, so I am setting it to code needs work.

bryan kennedy’s picture

I just wanted to put in my vote for this fix. While the language system is the ideal, many regular drupal users can't afford the overhead to turn that on and need to resort to theming to control the display of form elements like this.

I would really appreciate seeing this fix in the core at some point in the future.

Damien Tournoud’s picture

Status: Needs work » Closed (won't fix)

Using hook_form_alter() or the theming system to change that is *not* the way to go.

You will want to use the language system (which don't require enabling the locale module at all) to change the strings. One easy way to do this is by using the String Overrides module, but you can also override strings from your settings.php file.

Given this, this is probably a non-bug.

aaronbauman’s picture

Just because there is a work-around (installing a module or updating settings.php) doesn't mean that this is a non-bug.

Changing the text of a button, whether through hook_form_alter or the theming system, should not break a form's functionality. This issue addresses the fundamentally bad practice of mixing the logic and UI layers.

Count this as my vote for a Drupal-wide fix.

honorfield’s picture

Version: 7.x-dev » 6.15
Assigned: Unassigned » honorfield
Issue tags: +create new account, +string override module

The "String Overrides modul" worked fine for me in Drupal 6.15. I could easily change the "Create new account" text with my own text:)
Thanks for the suggestion Damien. Although it is very annoying that i have to install a module to change the text of a tab!

Agileware’s picture

Version: 6.15 » 7.x-dev
Assigned: honorfield » Unassigned
Status: Closed (won't fix) » Needs review
FileSize
644 bytes
PASSED: [[SimpleTest]]: [MySQL] 26,742 pass(es). View

Any chance this could be revisited?

As mentioned in multiple places around drupal.org, op shouldn't be relied on.
For a good example of this see http://drupal.org/node/144132#op

Also, I have not seen anywhere any doco that says you can use form alter to modify forms, but whatever you do don't modify the text on form submit buttons as it might break your sites functionality.

It just isn't intuitive.

Would this work as a solution or would it cause issues?

tinny’s picture

bump. i have the same problem. d6

i tried: $form['submit']['#value'] = t('Add company');

user/register works but not admin/user/user/create (it takes me back to list users)

currently using string overrides as a work around...

scor’s picture

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

subscribe. we need to remove this old cruft, though it'll have to be fixed in D8 first.

I would be tempted to remove this line of code:

  $op = isset($_POST['op']) ? $_POST['op'] : $callback_arg;

from user_admin() and just rely on the callback argument. There is only one place in core where this op value seems to be used: openid.module.

adnanhader’s picture

Version: 8.x-dev » 6.20

I have used a very simple technique using CSS to override create new account button with my custom button image with required text on it. hook_form_alter of course involved in it. just use

if($form_id == 'user_register') {
	$form['submit']['#attributes'] = array('class' => 'create_account');
}

This way i have assigned a new class to create new account button. implement button image into input.create_account class. you can hide create new account / default text in this class.

aaronbauman’s picture

Version: 6.20 » 8.x-dev
theborg’s picture

Assigned: Unassigned » theborg
FileSize
4.42 KB
FAILED: [[SimpleTest]]: [MySQL] 32,898 pass(es), 26 fail(s), and 19 exception(es). View

The 'problem' still persists in d8-dev.
One possible solution for this issue and the rest of modules could be using $form_state['triggering_element']['#name'] that forms module is populating with the clicked button.

To do that:
1. Add a name attribute to all the buttons that needs checking.
2. Change the logic to avoid the use of t() in conditional code.
3. Update tests without language dependence.

Don't know if these changes also affect performance, in a positive way.

Status: Needs review » Needs work

The last submitted patch, 208790_user_admin.patch, failed testing.

theborg’s picture

Status: Needs work » Needs review
FileSize
13.56 KB
FAILED: [[SimpleTest]]: [MySQL] 32,810 pass(es), 103 fail(s), and 44 exception(es). View

New patch with tests updated.

Status: Needs review » Needs work

The last submitted patch, 208790_and_tests.patch, failed testing.

theborg’s picture

FileSize
4.83 KB
PASSED: [[SimpleTest]]: [MySQL] 32,926 pass(es). View

Retesting first #19 patch, updated user.module to add a named to 'Create new account' button.

theborg’s picture

Status: Needs work » Needs review
FileSize
4.83 KB
PASSED: [[SimpleTest]]: [MySQL] 32,940 pass(es). View
theborg’s picture

czigor’s picture

This appliies and works great on D7.

sinasalek’s picture

Great, we definitely need this for Drupal 7. field_group_ajaxified_multipage depends on this patch for some of it's functionality to work.

trrroy’s picture

FileSize
4.84 KB
PASSED: [[SimpleTest]]: [MySQL] 40,529 pass(es). View

This is the same patch as #24 but re-rolled for D7.22 and it's working for me.

Status: Needs review » Needs work

The last submitted patch, 208790_user_admin_28.patch, failed testing.

steveoliver’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review

Let's see if it'll apply against 7.x

steveoliver’s picture

#28: 208790_user_admin_28.patch queued for re-testing.

steveoliver’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

The proposal of this issue is sane. It allows submit buttons to display whatever #value is needed without breaking form submit functionality. It keeps developers from needing to implement additional #validate or #submit handlers to work around the unnecessary use of t().

However, as noted in #4 and #7, this will surely cause issues for numerous d7 sites, given the existing checks of $form_state['values']['op'] == t(), including the core example in drupal_form_submit() to set this value when programatically submitting forms.

So unfortunately, I see no way for this to make it into 7.x core at this point.

This is a bug, so a working patch will probably make it into 8.x (as scor pointed out in #16).

mgifford’s picture

Assigned: theborg » Unassigned
Issue summary: View changes
erdjordje’s picture

Issue tags: +Barcelona2015

I tested this problem in 8.x and it seems that the problem is fixed.

I used following code in hook_form_alter to change name of the button.

  $default_values = array(
    'id' => 'en',
    'name' => 'English',
    'direction' => 'ltr',
    'weight' => 0,
    'locked' => FALSE,
  );
  $default_language = new \Drupal\Core\Language\LanguageDefault($default_values);
  $translation_manager = new \Drupal\Core\StringTranslation\TranslationManager($default_language);
  $form['actions']['submit']['#value'] = $translation_manager->translate('Test');

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.

dpi’s picture

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

The form API changed considerably. The following code is longer an issue on Drupal 8:

function hook_form_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state, $form_id) {
  $form['actions']['submit']['#value'] = t('Hello world');
}

But I agree with #1 that the language system can be used for this: https://www.drupal.org/docs/7/howtos/change-default-strings-text-without...

Changing to 7.x