Problem/Motivation

In user_user_role_insert we create actions per user role being created. We do this like this:

    $action = entity_create('action', array(
      'id' => $add_id,
      'type' => 'user',
      'label' => t('Add the @label role to the selected users', array('@label' => $role->label())),
      'configuration' => array(
        'rid' => $role->id(),
      ),
      'plugin' => 'user_add_role_action',
    ));
    $action->trustData()->save();

At first sight the t() looks wrong because it means config will be saved with the translated text - but actually I think this is fine since the configuration entity will have the language that the user has when they create the role. But what is more problematic is saving the escaped role label. This causes two problems - double escaping on admin/people if the role label contains an & and also potentially getting out of sync with the role label if that changes.

Proposed resolution

no idea yet

Remaining tasks

Find a solution.

User interface changes

None

API changes

?

Data model changes

?

Why is this an RC target?

  • Escaping the role label during the creation of the action label leads to double escaping. This patch fixes 27 out of the 32 fails found by #2571065-14: Find escaping due to Twig autoescape. Without this patch going in we can't implement some sort of generic double escaping test for all of our WebTestBase tests.
  • Not keeping the action label in sync with the action label could result in a very confusing UI experience (image if role labels are swapped - yes that would be dumb but we all do dumb things sometimes)
  • The fix creates a generic solution for actions to add arguments for it's labels. Given how actions are created based on other config entities this is likely to be needed again

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
5.66 KB

Here's a stab at a solution that lets you keep the action label in sync with the role label.

There is one problem though if you install the Action module this lets you edit the label but because of the way ActionFormBase adds the label and then delegates to the plugin - the plugin can't affect the label form element - which we need to do otherwise the user will save the version with the label replaced.

alexpott’s picture

This will solve a number of the escaping issues discovered by #2571065: Find escaping due to Twig autoescape which is how I came across this issue.

Status: Needs review » Needs work

The last submitted patch, 2: 2597608-2.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.57 KB
7.43 KB

So this makes the form work - but atm you actaully can't save such an action because of dots in the machine name!

Status: Needs review » Needs work

The last submitted patch, 5: 2597608-5.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.6 KB
10.36 KB

Fixing the tests

Status: Needs review » Needs work

The last submitted patch, 7: 2597608-7.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.34 KB
5.02 KB
16.7 KB

Fix the last test fail and adding tests.

joelpittet’s picture

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

alexpott’s picture

@joelpittet you are missing something!

+++ b/core/modules/system/src/Entity/Action.php
@@ -155,4 +159,24 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
+  /**
+   * @inheritDoc
+   */
+  public function label() {
+    $plugin = $this->getPlugin();
+    $label = new FormattableMarkup(parent::label(), $plugin->getLabelArguments());
+    // Remove the safeness from the label as the label can contain user input.
+    return PlainTextOutput::renderFromHtml($label);
+  }

This does the replacement when the label is used - not when the configuration entity is saved - thereby allowing the role label to change and be reflected in the action label.

The last submitted patch, 9: 2597608.test-only.patch, failed testing.

joelpittet’s picture

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

alexpott’s picture

Improved comments to explain the point @joelpittet raises in #10.

Also since calling parent::label() in a method other than label is super ugly - wrapped this in a protected method. The only reason do this is because the labelling system is super flexible... and alterable on the entity type...

  public function label() {
    $label = NULL;
    $entity_type = $this->getEntityType();
    if (($label_callback = $entity_type->getLabelCallback()) && is_callable($label_callback)) {
      $label = call_user_func($label_callback, $this);
    }
    elseif (($label_key = $entity_type->getKey('label')) && isset($this->{$label_key})) {
      $label = $this->{$label_key};
    }
    return $label;
  }

... if we didn't do this we could just do $this->label and access the property - and maybe we should just do this - since EntityTypeInterface::getLabelCallback() is deprecated.

alexpott’s picture

The idea of using $this-label() in #14 is a good one.

dawehner’s picture

  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

alexpott’s picture

Issue tags: +rc target triage
xjm’s picture

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.

alexpott’s picture

Issue summary: View changes

Added information about why this should be an RC target.

alexpott’s picture

Re #16

  1. Fixed
  2. Technically - but it is also a bug fix. The current code means that an action can only add new form elements whereas the interface implies that it should be able to change the initial form. I discussed this with @timplunkett and he agreed that this was correct.
  3. TBH I'm not really sure what you are suggesting here, but, I think just directly accessing $this->label to get the stored version is totally fine.
  4. Fixed
dawehner’s picture

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

Well, its protected ...

effulgentsia’s picture

Title: User role actions are created with a translated label » User role action labels get double-escaped and out of sync with the corresponding user role label
Issue tags: -rc target triage +rc target

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.

effulgentsia’s picture

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

effulgentsia’s picture

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.

effulgentsia’s picture

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.

effulgentsia’s picture

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.

xjm’s picture

Issue tags: -rc target

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.

Julfabre’s picture

UP

How can we help with the integration of this fix?

cilefen’s picture

Consider the last few review comments then write a patch.

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.