Problem/Motivation

In #1987398: field.module - Convert theme_ functions to Twig a bug was introduced that incorrectly translates the label of multi-value fields twice:

     $header = array(
       array(
-        'data' => '<h4 class="label">' . t('!title !required', array('!title' => $element['#title'], '!required' => $required)) . "</h4>",
+        'data' => array(
+          '#prefix' => '<h4 class="label">',
+          'title' => array(
+            '#markup' => t($element['#title']),
+          ),
+          '#suffix' => '</h4>',
+        ),
         'colspan' => 2,
         'class' => array('field-label'),
       ),

The $element['#title'] value is already translated at this stage:

  • As translated string, in the case of config fields
  • As TranslatableMarkup in the case of base fields

This provides a wrong translation in the first case and throw an InvalidArgumentException in the 2nd case.

Proposed resolution

  1. Remove the t() that wraps $element['#title'].
  2. Remove also the 'title' wrapping of #markup item. That seems superfluous.
  3. Add test.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Original report, by @ChristianAdamski

Hey,

the issue described here is probaby more widespread than just the scenario. I therefor assume there already is an issue for this, I just couldn't find any

Reproduce Problem:
- define an entity reference BaseFieldDefinition in a custom entity
- Use entity_reference_autocomplete
- use setLabel(t("Hello World")); as done across core
- allow more than one entry
- WSOD when trying to edit

Reason:
In template_preprocess_field_multiple_value_form() in line 1580 you'll find
'#markup' => t($element['#title']),
but $element['#title'] already contains translatableMarkup. I do not know what happens afterwards, as the result is also translatableMarkup, but it is different and results in WSOD.

Fix:
Remove the t(), the field label is already translated at this point.

Comments

ChristianAdamski created an issue. See original summary.

joelpittet’s picture

Version: 8.0.0-rc1 » 8.0.x-dev
Issue tags: +Twig, +D8MI

Thank you @ChristianAdamski, that shouldn't be there for sure.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Quickfix

Oh and this is RTBC

swentel’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Hmm, we should definitely have a test for this, because we've been running around in circles with t() in those methods.

Berdir’s picture

Here's a quick test for this. Name and location is bad, copied it of an existing test that dealt with that entity type. Was hoping we'd also have an existing web test for that but we don't. And multiple fun bugs prevented that from happening actually, like the fact that that entity type shared the same base table, which doesn't give you an error but then isn't the table structure you expect when you save ;)

Berdir’s picture

By the way, this bug got introduces with #1987398: field.module - Convert theme_ functions to Twig, which incorrectly converted the previous code:

     $header = array(
       array(
-        'data' => '<h4 class="label">' . t('!title !required', array('!title' => $element['#title'], '!required' => $required)) . "</h4>",
+        'data' => array(
+          '#prefix' => '<h4 class="label">',
+          'title' => array(
+            '#markup' => t($element['#title']),
+          ),
+          '#suffix' => '</h4>',
+        ),
         'colspan' => 2,
         'class' => array('field-label'),
       ),

So this was never intended to be translatable like this.

The last submitted patch, 5: avoid-double-translate-2584837-5-test-only.patch, failed testing.

joelpittet’s picture

Yeah that was wrong before and after the twig conversion for different reasons. We shouldn't be doing t('!title !required') either.

But that h4 should really be in the template.

joelpittet’s picture

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

This seems to do the trick, a couple nit picks. @Berdir is there more you'd like to do here? New name/location for the tests?

Otherwise it seems to be testing what it needs to test.

  1. +++ b/core/modules/field_ui/src/Tests/EntityFormDisplayWebTest.php
    @@ -0,0 +1,52 @@
    +  public function testBaseFieldComponent() {
    +
    +    $user = $this->drupalCreateUser(['administer entity_test content']);
    ...
    +    $this->assertText('entity_test_base_field_display ' . $entity->id() . ' has been updated.');
    +
    +
    +
    +  }
    

    nit: excess whitespace.

  2. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestBaseFieldDisplay.php
    @@ -70,6 +72,18 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +        'weight' => 11,
    ...
    +        'weight' => 11,
    

    nit: weights likely not needed?

slashrsm’s picture

Status: Needs work » Needs review
FileSize
3.67 KB
1.73 KB

Fixed. I kept weights as all other fields defined there have it.

Cottser’s picture

If possible let's try to combine efforts with what I think might be a duplicate and filed as critical: #2585483: Fatal error when generating form for multivalued base field

Berdir’s picture

#8: It wasn't actually as stupid as you'd think ;) Think right to left languages, like russion. They want to have the label first and then the required marker.

And yes, definitely a duplicate of that other one.

claudiu.cristea’s picture

Priority: Major » Critical
Status: Needs review » Needs work

@Cottser, the patch provided here seems to me more compact. Also this issue is older (with few hours!) than the other one. Let's go with this one by:

  1. Close #2585483: Fatal error when generating form for multivalued base field as duplicate.
  2. Raise this to Critical.
  3. Ask a core committer to add here credits form the other one.

Review:

  1. +++ b/core/includes/theme.inc
    @@ -1577,7 +1577,7 @@ function template_preprocess_field_multiple_value_form(&$variables) {
               'title' => array(
    -            '#markup' => t($element['#title']),
    +            '#markup' => $element['#title'],
               ),
    

    Let's drop also the 'title' wrapper. Is there a good reason for wrapping the #markup?

  2. +++ b/core/modules/field_ui/src/Tests/EntityFormDisplayWebTest.php
    @@ -0,0 +1,48 @@
    + * Contains \Drupal\field_ui\Tests\EntityFormDisplayWebTest.
    ...
    +namespace Drupal\field_ui\Tests;
    ...
    +class EntityFormDisplayWebTest extends WebTestBase {
    

    Is this test belonging to Field UI module? As far as I know, field_ui is providing administrative UI for managing fields, display modes and displays. This bug exists even if Field UI is disabled. Moreover, the crash is revealed by a base field that has nothing to do with field or field_ui modules. I propose to move it under Drupal\system\Tests\Entity. Also the name of the class sounds unrelated to the issue. How about MultipleCardinalityFieldTest? Or something similar?

  3. +++ b/core/modules/field_ui/src/Tests/EntityFormDisplayWebTest.php
    @@ -0,0 +1,48 @@
    +  public static $modules = ['field_ui', 'field', 'entity_test', 'field_test', 'user', 'text'];
    

    Some of them are not used, some of them are already installed upstream. It should work only with 'entity_test' module in the list :-)

  4. +++ b/core/modules/field_ui/src/Tests/EntityFormDisplayWebTest.php
    @@ -0,0 +1,48 @@
    +  /**
    +   * Tests the behavior of a field widget for a base field.
    +   */
    +  public function testBaseFieldComponent() {
    

    testLabelWithMultipleCardinality()? If you agree, adapt also the docblock.

claudiu.cristea’s picture

Issue summary: View changes

Fixed IS.

dawehner’s picture

  1. +++ b/core/includes/theme.inc
    @@ -1577,7 +1577,7 @@ function template_preprocess_field_multiple_value_form(&$variables) {
               'title' => array(
    -            '#markup' => t($element['#title']),
    +            '#markup' => $element['#title'],
               ),
    

    Yes, at least in core #title is coming from $this->fieldDefinition->getLabel(), see \Drupal\Core\Field\WidgetBase::formMultipleElements

  2. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestBaseFieldDisplay.php
    @@ -70,6 +72,18 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +      ->setLabel(t('A field with multiple values'))
    

    What about adding some <script>alert(123)</script> to ensure we escape the result or at least xss filter it somewhere?

claudiu.cristea’s picture

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestBaseFieldDisplay.php
@@ -84,6 +84,19 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    // A multi-value base field with a dangerous label.
+    $fields['test_display_another_multiple'] = BaseFieldDefinition::create('text')
+      ->setLabel("<script>alert('another multi-value');</script>")
+      ->setCardinality(FieldStorageDefinition::CARDINALITY_UNLIMITED)
+      ->setDisplayOptions('view', array(

Base fields didn't have this problem, configurable fields. So to test this, you need to additionally add a configurable field.

dawehner’s picture

Yeah totally agree with @Berdir, we need a test with a configurable field.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
4.28 KB
3.15 KB

OK. Moved to configurable field.

effulgentsia’s picture

+++ b/core/modules/system/src/Tests/Entity/EntityBaseFieldTest.php
@@ -0,0 +1,70 @@
+  public function testLabelOnMultiValueBaseFields() {

What about moving this test to Drupal\field\Tests\FormTest? Also, since a configurable field is now also tested, perhaps "Base" should be removed from the method name? Or should we split into 2 test methods: one for base, one for configurable?

claudiu.cristea’s picture

FileSize
4.17 KB
4.35 KB

What about moving this test to Drupal\field\Tests\FormTest?

Yes, I think it's a good idea.

Also, since a configurable field is now also tested, perhaps "Base" should be removed from the method name?

Renamed to testLabelOnMultiValueFields()

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Great back to RTBC, thanks for all the test help!

ChristianAdamski’s picture

Just asking:

-          'title' => array(
-            '#markup' => t($element['#title']),
-          ),
+          '#markup' => $element['#title'],

The patch is replacing the 'title' directly with '#markup'. The 'title' is not referenced anywhere?
If not: thumbs up.

joelpittet’s picture

@ChristianAdamski good question, though this is in preprocess, so unless someone in contrib is overriding this template or preprocess and trying to deconstruct the built up table on this admin field. It's highly unlikely this would be referenced somewhere else. If they were overriding they would likely build up that table(or other structure) using the original value in $element['#title']

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/theme.inc
@@ -1576,9 +1576,7 @@ function template_preprocess_field_multiple_value_form(&$variables) {
-          'title' => array(
-            '#markup' => t($element['#title']),
-          ),
+          '#markup' => $element['#title'],

I think removing the title level here is unnecessary. Since we're in RC we should not be making unnecessary change - even in the service of a critical.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
4.14 KB
680 bytes

Changed that back as it's unnessasary change for RC. I think it's an improvement but not a necessity.

RTBC because it doesn't change the output and result whatsoever.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, one less t() to worry about :)

Committed 26d5a5f and pushed to 8.0.x. Thanks!

  • alexpott committed 26d5a5f on 8.0.x
    Issue #2584837 by claudiu.cristea, Berdir, joelpittet, slashrsm,...
effulgentsia’s picture

I think it's an improvement but not a necessity.

I think it would make for a fine 8.1 issue if someone feels like opening that.

joelpittet’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.