Change "users" to "user(s)" in user actions for consistency.

I have already tested this but it's not working even after clearing cache.

You mean update.php or clearing cache will not work, I have to create new database and reinstall site.

after reinstalling it is working but I don't think that it is right way as if someone is having drupal 8 site and S(he) wants to upgrade the version then they are not going to reinstall the site and change the database.

Hi all,

A cache clear won't be enough on this case, because the label for administrator is actually saved in Database, "config" table with name "system.action.user_add_role_action.administrator".

Maybe a hook_update_N along with the label path, this could be done.

Tried to upgrade the label on hook_update_N, seemed to work in my tests.

#11It doesn´t work for me, you forgot to add the previous patch to yours. So , when a new role is added the option is still wrong.

Before applying #11

After applying #11

I merged both patched into one

Hi people

I applied the patch and the result is:

Zoom:

Good Work and Good Week.

Regards.

You're right @tar_inet .. i forgot to merge the 2. The patch I provided was for the exception situation reported before that it wouldn't change label for "old" roles.

#13 @renatog did you run the update.php? It looks like you haven´t.

Hi people.

You are right @tar_inet

After run update.php works good for me. Follow screenshot with result.

Thank you very much for yours contributions guys.

Good Work.

Regards

+1 for adding consistency to core! This issue needs a new title that explains the task succinctly because it becomes the commit message.

1. +++ b/core/modules/user/user.install
   @@ -98,3 +98,30 @@ function user_update_8100() {
   + * Add user(s) to dropdown options for User list role add and remove.
   

   This comment will be a bit unclear when updates are executed. Is the update hook adding user(s) or "user(s)" to the dropdown option? I know the answer but I can imagine confusion. I think the dropdown can be referred to as "user actions". Why is "User" capitalized? Take a look at other hook_update_n comments for inspiration.

2. +++ b/core/modules/user/user.install
   @@ -98,3 +98,30 @@ function user_update_8100() {
   +  // Config name prefix for plugin user_add_role_action.
   +  $config_prefix_add = 'system.action.user_add_role_action.';
   

   This comment isn't really necessary and neither is the variable. The same applies to the following comment and variable.

3. +++ b/core/modules/user/user.install
   @@ -98,3 +98,30 @@ function user_update_8100() {
   +      $config_add->set('label', t('Add the @label role to the selected user(s)', array('@label' => $role_name)))->save(TRUE);
   

   We use the new array syntax now ;-)

4. +++ b/core/modules/user/user.install
   @@ -98,3 +98,30 @@ function user_update_8100() {
   +      $config_remove->set('label', t('Remove the @label role to the selected user(s)', array('@label' => $role_name)))->save(TRUE);
   

   It should be "Remove the @label role from..."

Thanks @cilefen, I hope this changes are better now

looks good to me!

It works :D Marking as RTBC

1. +++ b/interdiff-281072-22.txt
   @@ -0,0 +1,38 @@
   + ¶
   ...
   + ¶
   ...
   + ¶
   

   Trailing whitespace.

2. +++ b/interdiff-281072-22.txt
   @@ -0,0 +1,38 @@
   +- * Add user(s) to dropdown options for User list role add and remove.
   ++ * Change "users" to "user(s)" in user actions for consistency.
   

   This should be a single line (but still less than 80 characters).

Added the patch.

I messed it up, sorry. #26 @jansete patch looks good.

Added required proper comments.

It looks good to me.

This should be a single line (but still less than 80 characters).
#26 it's okay

Hi people.

#28 it's ok.

Good Work and Good Week.

Regards.

+++ b/core/modules/user/user.install
@@ -98,3 +98,24 @@ function user_update_8100() {
+    $config_add = $config_factory->getEditable('system.action.user_add_role_action.' . $role_machine_name);
+    if (!empty($config_add->get('label'))) {
+      $config_add->set('label', t('Add the @label role to the selected user(s)', ['@label' => $role_name]))->save(TRUE);
+    }
+
+    $config_remove = $config_factory->getEditable('system.action.user_remove_role_action.' . $role_machine_name);
+    if (!empty($config_remove->get('label'))) {
+      $config_remove->set('label', t('Remove the @label role from the selected user(s)', ['@label' => $role_name]))->save(TRUE);
+    }

Hm, there is no guarantee that the configuration was created in the language that the update is running with, so this may break the strings in the configuration (make them appear in a different language then they should).

Also it is possible to customize this label isn't it? Why would we overwrite customized labels with our own?

I was under the impression that a configuration label should always be first and foremost in English, and translatable.

I am not sure about if "it is possible to customize this label"

You can absolutely edit the action:

Forcibly updating the existing configured label to something else is IMHO not a good idea. If it was the same as the originally generated one, then *maybe*, but even then it might be best not to mess with the existing site IMHO.

thanks for pitching in :) @gabor .. I see it now.

My 2 cents.. maybe then just patch with the first original patch that only changed the string, and didn't do an hook_update to the label.. this would bring consistency for the newly installed core D8 for that particular label, right?

Yeah I think so yes. There is also the problem of which language are these config entities getting created in but that is for another bug report :)

I agree with Gábor, thanks for noticing. Applied the original patch in #2, it worked cleanly. Please see the screens. I am resubmitting the patch in #2 so that original patch comes on top.

I've tested and #37 is ok!

Committed 4971d77 and pushed to 8.4.x. Thanks!

Funnily enough the actions aren't editable via the UI because #2605042: Cannot edit actions created by user_user_role_insert().

It's nice that these labels are not consistent with the other labels.

I'm crediting @saurabh rawat for testing the patch on existing sites and noting the lack of an upgrade path - even though there should not be an upgrade path it is always good to think about existing sites. I'm crediting @cilefen for the thorough review. I didn't credit @tstoeckler because their review was just very minor coding standards nits.

@kporras07 thank you for reviewing this issue! Just saying it works is not enough evidence for review credit. What we do need people to review is whether the issue has a correct scope, whether it passes the core gates, whether the solution completely fixes the problem without introducing other problems, and whether it's the best solution we can come up with. See the patch review guide for more information. When you do post a review, be sure to describe what you reviewed and how. This helps other reviewers understand why you considered the issue RTBC (and is considered for issue credit).