Wrong "all languages" in multilingual paragraphs

#2532284: HTML is escaped in "all languages" in UI after SafeMarkup change should fix the double encoding of all languages.

I was discussing this issue with Berdir and plach. We came up with a quick fix, but we were not totally satisfied with the implementation.

So core does set the #multilingual flag in core/modules/content_translation/content_translation.module:303 in regard of the field definition - which is correct, assuming there are no nested or complex fields. But having widgets/modules like Paragraphs or InlineEntityForm being not translatable, but their nested forms - this will result in the misleading multilanguage indication on the children.

There are two ways of implementing this:
1. wait for core issue #2575771: Do not overwrite #multilingual, if already set to be submitted and this patch will work, or
2. implement (and duplicate core code) the core/modules/content_translation/content_translation.module:content_translation_form_alter and do the nested multilingual check in the paragraphs module

So I was stepping through the code trying to implement a valid version of core/modules/content_translation/content_translation.module:content_translation_form_alter for paragraphs.module, but failed to descent into the widgets fields by the core. This is then a core issue. I commented it in #2575771: Do not overwrite #multilingual, if already set.

ATM removing the multilang indication is the only possible negotiation between core and nested field modules...

Should not add core tags, sorry :) Anyway, I wanted to add that this is a systemic question with field translation in general. It is always the case that a field may be a reference, and while the reference may not be translatable, the referred may be.

I did the following workaround for IEF in #2494959: Add translation integration:

+      '#field_title' => $this->fieldDefinition->getLabel(),
+      '#after_build' => [
+        [get_class($this), 'removeTranslatabilityCue'],
+      ],
     ] + $element;

+  /**
+   * After-build callback for removing the translatability cue from the widget.
+   *
+   * IEF expects the entity reference field to not be translatable, to avoid
+   * different translations having different references.
+   * However, that causes ContentTranslationHandler::addTranslatabilityClue()
+   * to add an "(all languages)" suffix to the widget title. That suffix is
+   * incorrect, since IEF does ensure that specific entity translations are
+   * being edited.
+   */
+  public static function removeTranslatabilityCue(array $element, FormStateInterface $form_state) {
+    $element['#title'] = $element['#field_title'];
+    return $element;
+  }

This is a workaround. It will be simpler when #2575771: Do not overwrite #multilingual, if already set lands into core (cause we shoudl be able to just do $element['#multilingual'] = TRUE).

The patch fixes the issue. Tests are missing and this needs to be tested with different field types in different configurations.

I just tested the latest patch #14, and it removes unconditionally all (all languages) title postfixes,
even the one that actually are not translatable

Yes, see my comment that I just posted in #2731715: Hide shared fields when translating, related to that. We need to discuss this.

Now the "all languages" is only being removed from the label if the field is translatable.

now respects the untranslatable and nested fields/paragraphs. perfect, thx.

here's a patch passing the tests - hopefully ;)

What do you mean by nested paragraphs? Didn't it work with my patch? I made two small improvements.

nested paragraphs are working with your patch Lukas.

Tests added. :)

1. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -542,6 +543,15 @@ class InlineParagraphsWidget extends WidgetBase {
   +        foreach (Element::children($element['subform']) as $fieldname) {
   +          $translatable = $paragraphs_entity->{$fieldname}->getFieldDefinition()->isTranslatable();
   +          if ($translatable) {
   +            $element['subform'][$fieldname]['widget']['#after_build'][] = [
   

   Looping over subform elements and then unconditionally accessing the field seems problematic?

   Either loop over fields and check if the exist in the form or check if hasField($fieldname).

2. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -1179,4 +1189,26 @@ class InlineParagraphsWidget extends WidgetBase {
   +    if (isset($element['#title'])) {
   +      foreach (Element::children($element) as $delta) {
   +        $element[$delta]['value']['#title'] = $element['#title'];
   +      }
   

   this hardcodes value.

   what happens if you have an entity reference field, for example, that is/is not translatable?

True, it didn't work with reference fields, also added tests for them. It even works with Nested paragraphs :D

1. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -542,6 +543,17 @@ class InlineParagraphsWidget extends WidgetBase {
   +              $element['subform'][$field]['widget']['#after_build'][] = [
   +                get_class($this),
   +                'removeTranslatabilityCue'
   +              ];
   +            }
   

   I don't like this syntax too much, static::class looks nicer to me (and works: https://3v4l.org/Uj6gJ ;) but that's minor.

   I think "static::class . '::removeTranslatabilityCue'" would be even nicer, but apparently only works in PHP 7: https://3v4l.org/fW1st ?

2. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -1179,4 +1191,28 @@ class InlineParagraphsWidget extends WidgetBase {
   +        foreach (Element::children($element[$delta]) as $field) {
   +          $element[$delta][$field]['#title'] = $element['#title'];
   

   Can we check if the element there actually has a title?

   Wondering what will happen with form widget with multiple elements. Maybe instead of loop, we should look at the main property and try to use that?

One thing to check could be a date field, but I think those actually hide the title.

Maybe we could compare the titels and only update if they contain the title we want to replace it with or something?

True, there was some problems in the previous patch for some fields like image, link. Added tests for link field and changed the function to make it work. Also tested with double nesting works fine.

1. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -1205,11 +1205,18 @@ class InlineParagraphsWidget extends WidgetBase {
   +    foreach (Element::children($element) as $delta) {
   +      if (isset($element[$delta]['#title']) && strpos($element[$delta]['#title'], $suffix)) {
   +        $element[$delta]['#title'] = str_replace($suffix, '', $element[$delta]['#title']);
   +      }
   +      foreach (Element::children($element[$delta]) as $field) {
   +        if (isset($element[$delta][$field]['#title']) && strpos($element[$delta][$field]['#title'], $suffix)) {
   

   As Miro already said I think, some comments here to explain why we are doing it like this (widgets could have multiple elements with their own titles, so remove the suffix if it exists, do not recurse lower than this to avoid going into nested paragraphs or similar nested field types)

2. +++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
   @@ -1205,11 +1205,18 @@ class InlineParagraphsWidget extends WidgetBase {
   +      if (isset($element[$delta]['#title']) && strpos($element[$delta]['#title'], $suffix)) {
   

   usually, a strpos() check is always done as a !== FALSE because it could also be 0, but since we don't expect that string to be at the first position, that seems OK here.

3. +++ b/src/Tests/ParagraphsTranslationTest.php
   @@ -345,20 +346,34 @@ class ParagraphsTranslationTest extends WebTestBase {
        $this->assertNoUniqueText('untranslatable_field (all languages)');
        $this->assertNoUniqueText('untranslatable_ref_field (all languages)');
   +    $this->assertNoUniqueText('untranslatable_link_field (all languages)');
        $this->assertNoText('Text (all languages)');
    
   

   I think assertText() is enough, assertNoUniqueText() makes me think too much about what we are actually trying to do. Just making sure the text is there like that is enough.

True, now should be fine. :)

This comment makes more sense.

Agreed.