User role action labels get double-escaped and out of sync with the corresponding user role label

+++ b/core/modules/user/user.module
@@ -988,7 +988,7 @@ function user_user_role_insert(RoleInterface $role) {
-      'label' => t('Add the @label role to the selected users', array('@label' => $role->label())),
+      'label' => t('Add the @role_label role to the selected users'),

@@ -1001,7 +1001,7 @@ function user_user_role_insert(RoleInterface $role) {
-      'label' => t('Remove the @label role from the selected users', array('@label' => $role->label())),
+      'label' => t('Remove the @role_label role from the selected users'),

I may be missing something here but doesn't this need the placeholder args?

Oh wow that makes things complicated, I would have tried to fix that... bizarre.

1. +++ b/core/lib/Drupal/Core/Action/ActionInterface.php
   @@ -63,4 +63,11 @@ public function executeMultiple(array $objects);
   +   * Arguments for FormattableMarkup for a the label.
   

   that might be dawehner english but proper alexpott english, I'm not sure about that

2. +++ b/core/modules/action/src/ActionFormBase.php
   @@ -91,7 +91,7 @@ public function form(array $form, FormStateInterface $form_state) {
        if ($this->plugin instanceof PluginFormInterface) {
   -      $form += $this->plugin->buildConfigurationForm($form, $form_state);
   +      $form = $this->plugin->buildConfigurationForm($form, $form_state);
        }
   

   This is technically an API change, right?

3. +++ b/core/modules/system/src/Entity/Action.php
   @@ -155,4 +162,28 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
   +      $label = new FormattableMarkup($label, $label_arguments);
   +      // Remove the safeness from the label as the label can contain user input.
   +      $label = PlainTextOutput::renderFromHtml($label);
   

   So yeah I'm wondering in which way we should separate the stored value from the resolved value ... maybe an getStoredValue() method, maybe don't change the label() behaviour but call out to a new method ...

4. +++ b/core/modules/user/src/Tests/UserAdminTest.php
   @@ -95,6 +95,16 @@ function testUserAdmin() {
   +    // Assert escaped correctly.
   

   Nitpick: its also xss filtered here

So this is definitely a bug, but since it's confined to the creation and update of certain actions, I personally don't think it needs to be an RC target.

ut, I think just directly accessing $this->label to get the stored version is totally fine.

Well, its protected ...

This makes sense as an RC target to me, for the reasons mentioned in the issue summary. In a conversation today, @xjm agreed as well thanks to the clarifications since #18.

1. +++ b/core/lib/Drupal/Core/Action/ActionInterface.php
   @@ -63,4 +63,13 @@ public function executeMultiple(array $objects);
   +  public function getLabelArguments();
   

   I don't think we should add this to ActionInterface. Instead, I think we should make a new interface, such as FormattableLabelInterface or LabelWithPlaceholdersInterface. Then action plugin implementations can opt-in to that interface or not. And it becomes a pattern we can use for other plugin types.

2. +++ b/core/modules/system/src/Entity/Action.php
   @@ -155,4 +162,28 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
   +    if (!empty($label_arguments)) {
   +      $label = new FormattableMarkup($label, $label_arguments);
   +      // Remove the safeness from the label as the label can contain user input.
   +      $label = PlainTextOutput::renderFromHtml($label);
   +    }
   

   I think we should always PlainTextOutput::renderFromHtml() action entity labels, not make it conditional on there being arguments.

3. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
   @@ -292,7 +292,9 @@ public function viewsForm(&$form, FormStateInterface $form_state) {
   -        '#options' => $this->getBulkOptions(),
   +        // This is a select list therefore apply the plain text formatter to the
   +        // the options which are escaped for display in HTML.
   +        '#options' =>  array_map('\Drupal\Component\Render\PlainTextOutput::renderFromHtml', $this->getBulkOptions()),
   

   Doing the point above would allow us to remove this.

4. +++ b/core/modules/user/src/Plugin/Action/ChangeUserRoleBase.php
   @@ -63,6 +73,14 @@ public function defaultConfiguration() {
   +    // The label supports replacing @label with the role's label.
   +    $form['label'] = array(
   +      '#type' => 'textfield',
   +      '#title' => $this->t('Label'),
   +      '#default_value' => $this->configuration['original_label'],
   +      '#maxlength' => '255',
   +      '#description' => $this->t('A unique label for this advanced action. This label will be displayed in the interface of modules that integrate with actions. @role_label is replaced with the role\'s label.'),
   +    );
   

   I don't like the competition of responsibility of the 'label' element between this and ActionFormBase::form(). Instead, what if we added getLabelArgumentsDescription() to LabelWithPlaceholdersInterface which could return the last sentence of the above description, and then ActionFormBase::form() could be responsible for setting the appropriate description based on whether the plugin implements LabelWithPlaceholdersInterface or not. That would also allow us to completely remove 'original_label' from this patch, because ActionFormBase::form() could set the default value to $this->entity->get('label').

I think we should always PlainTextOutput::renderFromHtml() action entity labels, not make it conditional on there being arguments.

Actually, maybe that's wrong. But I think we should do something differently from what the patch is doing, because the meaning of a user-entered "<" character within an action label should not depend on whether the action plugin supports label arguments or not.

Something like:

PlainTextOutput::renderFromHtml(new FormattableMarkup(Html::escape($label), $label_arguments));

might be closer to what we want, in which case, that could be conditional on $label_arguments, because the above should be a no-op when $label_arguments is empty.

In which case, I'm also unclear on why #23.3 is needed at all.

If we do #25 (meaning, if the 'label' config property is intended to be interpreted as containing plain-text, not HTML), then we should also change:

+++ b/core/modules/user/user.module
@@ -988,7 +988,9 @@ function user_user_role_insert(RoleInterface $role) {
+      'label' => t('Add the @role_label role to the selected users'),

to:

'label' => PlainTextOutput::renderFromHtml(t('Add the @role_label role to the selected users')),

Because even though in this case, it might be unlikely that translators will add HTML here, the output of t() should always be treated as HTML, for consistency.

UP

How can we help with the integration of this fix?

Consider the last few review comments then write a patch.

Manually tested this, and this is still an issue. I think you could easily work around this and manually set this straight in the config entity after it got created, if it wasn't for #3150316: Configured actions created by User module cannot be edited which makes this impossible through the UI currently.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.