Double translation in template_preprocess_field_multiple_value_form()

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

Oh and this is RTBC

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

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 ;)

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.

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.

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?

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

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

#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.

@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.

Fixed IS.

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?

Fixed points from #13 and #15.

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

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

OK. Moved to configurable field.

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

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()

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

@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']

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

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.

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

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

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.

@effulgentsia done #2592025: template_preprocess_field_multiple_value_form() remove nested render arrays