Configuration translation UI does not support plural sources/targets

I'm working on it

Attached is a first approach to this feature.

Now I have the problem that I need to create a submit function for the form to process the values for plural configuration translation to add the LOCALE_PLURAL_DELIMITER before save the current configuration. What could be the better approach to do that?

Added the tag for devdays review

It seems we need to be able to provide not only a text widget that handles these locale delimiters, but one that spawns into a different number of text fields depending on the language we are translating to.

So I think simply hacking around current form elements just won't work. We need to provide a new data type / widget for handling these strings, which may be interesting just to see how well our current config translation API supports other widgets.

These are the pieces we'd need to implement:
1. New config schema type.. 'plural_string' (that will extend 'string' and be 'translatable')
2. A new config translation form element... 'PluralString'

Btw, seeing the current views.field.numeric schema, it seems we are not thinking of other translatable elements, like Thousands marker, Prefix, Suffix, etc... shouldn't we translate those too? Wondering whether we should be adding a full 'number format' schema type somewhere instead of just 'plural_string'....

Thanks for your insight Jose Reyero. Last week I made a patch that support multiple plural forms, but didn't post it since it was too hackish and I haven't find how to save the config anyway.

I agree with you about the need to create a new type / widget, I'll try that this weekend.

Here is a first patch where I add the new data type.
For now, I justed copied the Textfield widget to see if it worked.
I will continue to work to build the widget for this new type.

@Gábor Hojtsy, @Jose Reyero, can you tell me if I'm on the right track?
I'm not sure about the naming of things, please tell me if there's better names, comments, etc.

@maxocub,

Yes, that's the idea. Now you only need to implement the FormElement class which I'm afraid will be the most complex part.

About the names I'd like more 'plural_string / PluralString' and it can extend 'string' (type: string, translatable:true) because it may not be really a 'label'.

This is still not a final patch, it's just to get some review.

So I found a way to implement the FormElement class, and I'd like to know what you think about it, @Jose Reyero.

Visually it's not perfect, since using a fieldset adds a border and a background that doesn't match between source and translation, but essentially, it seems to work.

Here's what it looks like:

Here's an updated patch where I add titles to the fieldsets and generate only one plural form source element.

I've found that with the previous patch, when we translate from a multiple plural forms language to a single one (like from Slovenian to English), we have multiple source elements for only one translation element. I looked at how it's done in TranslateEditForm::buildForm() and did the same here to always have one sigular form and one plural form in the source element.

Is that what is expected?

I think this looks good. I would set it to RTBC, but it would need a test, no?

Wrote a quick test (which is the only difference compared to the patch in #17).

Thanks eiriksm for the test, that was the next thing I was going to do.
Should we add more tests for when we have multiple plural forms?

Also, I'd like to update the patch to remove the 'TODOs' from FormElementBase:

    // @todo Should support singular+plurals https://www.drupal.org/node/2454829

And remove a useless array index:

    for ($i = 0; $i < $plurals; $i++) {
      $element[$i] = array(

I'll do that tonight.

Should we add more tests for when we have multiple plural forms

I don't see why that could hurt :)

The only reason I did not add that (and the actual checking of translations being saved and displayed) is that #2453761: Views numeric formatter's plural formatting setting incompatible with many languages adds a whole lot of tests for the functionality of translating the strings. Just not through the interface. But that was my thought, what do you guys think?

Looks like it's 'needs review'

I think it was still on 'Needs work' because the test eiriksm added failed, but I don't know why. I ran it localy and it passed. Also, I was thinking about adding more tests for languages with multiple plural forms, but I haven't had the time yet.

I posted a test-only patch (which by the way was the one that just was re-queued for testing). This test was meant to fail, as it shows it needs the patch to pass. Perhaps meaningless in the context of adding a new feature, still it shows the value the test was providing (namely test a new feature, that did not exist).

So when this test is run, it will also fail.

The first patch on the other hand, will pass (hopefully) as it includes the test and the patch provided by @maxocub :)

Oh, silly me, I haven't thought about that, of course it failed! Thanks @eiriksm!

I think we need a test translating to a language with multiple plurals. Direction of the patch looks good :))

Aside of that:

1. +++ b/core/modules/config_translation/src/FormElement/PluralString.php
   @@ -0,0 +1,75 @@
   + * Defines the plural string textfield elements for the configuration translation interface.
   

   Nitpicking, should fit in 80 chars.

2. +++ b/core/modules/config_translation/src/FormElement/PluralString.php
   @@ -0,0 +1,75 @@
   +          '!label' => ($i == 0 ? 'Singular form' : 'Plural form'),
   ...
   +        '#title' => $this->t('!label <span class="visually-hidden">(!source_language)</span>', array(
   +          '!label' => ($i == 0 ? $this->t('Singular form') : $this->formatPlural($i, 'First plural form', '@count. plural form')),
   +          '!source_language' => $translation_language->getName(),
   

   We should use @ here for the placeholders.

3. +++ b/core/modules/config_translation/src/FormElement/PluralString.php
   @@ -0,0 +1,75 @@
   +        '#markup' => '<span lang="' . $source_language->getId() . '">' . nl2br($values[$i]) . '</span>',
   

   We should use SafeMarkup::format.

We also need before/after screenshots in the issue summary.

1) Made the changes from #19 & #30
2) Modified the test from #18 to use a multiple plural forms language
3) Added before/after screenshots to the summary

LGTM!

Patch works and screenshots are descriptive.

1. +++ b/core/modules/config_translation/src/FormElement/PluralString.php
   @@ -0,0 +1,77 @@
   +        '#markup' => '<span lang="' . $source_language->getId() . '">' . SafeMarkup::format($values[$i]) . '</span>',
   

   What I expected was having #markup in the format. Actually wondering if it should be an inline template instead.

2. +++ b/core/modules/config_translation/src/FormElement/PluralString.php
   @@ -0,0 +1,77 @@
   +          '@label' => ($i == 0 ? $this->t('Singular form') : $this->formatPlural($i, 'First plural form', '@count. plural form')),
   

   It feels weird having:

   - First plural form
   - 2 plural form

   Could we improve that? Not sure if really possible.

Thanks for working on this one!

@penyaskito, the PluralString FormElement was based on the TranslateEditForm::buildForm() as proposed in the summary, your comments then apply to this one too.

1)

What I expected was having #markup in the format. Actually wondering if it should be an inline template instead.

Not sure I understand here.

2)

It feels weird having:

- First plural form
- 2 plural form

Could we improve that? Not sure if really possible.

If we do, we should do it to the TranslateEditForm::buildForm() too (see image below).

Shouldn't we do that in a new issue?

1. I see TranslateEditForm is mixing inline templates and string concatenation (see line 102). I guess we can leave this for a follow-up. What I meant is having all the HTML in the SafeMarkup::format() call.

2. Let's create a follow-up then for that too.

So we still need:
1. Create those follow-ups.
2. Add the beta-evaluation template

OK, I'll do those two things this weekend.

Should I correct the SafeMarkup::format() first or do I leave it for the follow-up?

Leave it for the follow-up, so we have same in both forms.
Hope that committers agree and we can RTBC this one.

Updated the summary and added the beta evaluation template.

Opened two follow-up issues: #2499639: Use correct labels for numeric fields when using a multiple plural forms language and #2499651: Use inline_templates in TranslateEditForm::buildForm() and PluralString::getTranslationElement()

For me this is RTBC now. Thanks for your continuous effort, @maxocub!

1. This looks like a really nice UX improvement - carefully inserting EXT sounds like a nightmare.

2. +++ b/core/modules/config_translation/src/FormElement/PluralString.php
   @@ -0,0 +1,77 @@
   +        '#markup' => '<span lang="' . $source_language->getId() . '">' . SafeMarkup::format($values[$i]) . '</span>',
   

   This looks odd. Why are we using SafeMarkup::format() here.

3. +++ b/core/modules/config_translation/src/FormElement/PluralString.php
   @@ -0,0 +1,77 @@
   +        '#title' => $this->t('@label <span class="visually-hidden">(@source_language)</span>', array(
   
   +++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
   @@ -89,6 +89,7 @@ protected function setUp() {
          ]
   

   This shouldn't be t()'d everything has already been translated. Use SafeMarkup::format instead.

4. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
   @@ -607,6 +608,54 @@ public function testViewsTranslationUI() {
   +    drupal_unlink($name);
   

   This is deprecated. We shouldn't be adding usages. Use \Drupal::service('file_system')->unlink() instead. However I'm not sure why we're doing this as the file should be cleaned up after the test anyway.

Back to RTBC, feedback was addressed.
We could change SafeMarkup for inline templates while we are editing those lines, but let's try to do that in follow-up #2499651: Use inline_templates in TranslateEditForm::buildForm() and PluralString::getTranslationElement(). I tagged that one with SafeMarkup for visibility.

+++ b/core/modules/config_translation/src/FormElement/PluralString.php
@@ -0,0 +1,80 @@
+    for ($i = 0; $i < 2; $i++) {
+      $element[] = array(
+        '#type' => 'item',
+        '#title' => SafeMarkup::format('@label <span class="visually-hidden">(@source_language)</span>', array(
+          '@label' => $i == 0 ? $this->t('Singular form') : $this->t('Plural form'),
+          '@source_language' => $source_language->getName(),
+        )),
+        '#markup' => SafeMarkup::format('<span lang="@lancode">@value</span>', array(
+          '@langcode' => $source_language->getId(),
+          '@value' => $values[$i]
+        )),
+      );
+    }
+    return $element;

What happens when a site is installed in a language which has more plural forms?

When a site is installed in a language with multiple plural forms, only the singular and one plural form are displayed on the source side. At first, I tried to display all of them, but I think it makes no sense for two reasons:

1. If we translate to a language with less plural forms, we would have more source elements than translation elements.
2. If we translate to an other language with multiple plural forms, they probably won't have the same plural formulas, so we would be associating source elements with translation elements that doesn't match.

Since formatPlural() take as arguments $count, $singular and $plural, I think it's logical to only show the singular and one plural form, and when #2499639: Use correct labels for numeric fields when using a multiple plural forms language will be resolved, it will make this form a lot clearer.

Also, this form was based upon TranslateEditForm::buildForm(), which have the same behavior.

@Gábor Hojtsy: OK, I see, I will change that, but is it the case only for configuration translation or should we do the same thing with TranslateEditForm::buildForm() ?

Updated patch with as many source elements as the source language's plural forms.

Here's what it looks like when translating from Slovenian to Russian:

And from Vietnamese to Irish:

Since Vietnamese doesn't have a plural form, formatPlural on the labels returns 'First plural form' for every translation elements (I think). This problem should be solved by #2499639: Use correct labels for numeric fields when using a multiple plural forms language, but it's postponed on this issue. What should we do? Make a temporary fix here before solving #2499639: Use correct labels for numeric fields when using a multiple plural forms language, or postpone this issue and fix #2499639: Use correct labels for numeric fields when using a multiple plural forms language first?

#54/1: I replace 'plural_string' by 'plural_label'. I also replaced the widget's name from 'PluralString' to 'PluralVariants', I hope it's OK.

#54/2: I changed the doc string to 'Defines form elements for plurals in configuration translation.'

#54/3: I added the @todos in PluralVariants::getSourceElement(), PluralVariants::getTranslationElement(), NumericField::buildOptionsForm(), and TranslateEditForm::buildForm().

#55: I changed the label to 'Plural Variants'.

The manual testing of the changes in #53 looks great but afaics we didn't extend the automated test coverage for this.

@Gábor Hojtsy: Absolutely, I just had no time last week, and sorry about missing the meeting.

1. +++ b/core/modules/config_translation/src/FormElement/PluralVariants.php
   @@ -0,0 +1,82 @@
   +    $plurals = $this->getNumberOfPlurals($source_language->getId());
   

   I might be totally on crack, but I can't seem to fathom where to find this function anywhere?

2. +++ b/core/modules/config_translation/src/FormElement/PluralVariants.php
   @@ -0,0 +1,82 @@
   +      '#title' => $this->t($this->definition->getLabel()),
   ...
   +        '#title' => SafeMarkup::format('@label <span class="visually-hidden">(@source_language)</span>', array(
   +          // @todo Should use better labels https://www.drupal.org/node/2499639
   +          '@label' => $i == 0 ? $this->t('Singular form') : $this->formatPlural($i, 'First plural form', '@count. plural form'),
   +          '@source_language' => $source_language->getName(),
   +        )),
   

   I think the <span class="visually-hidden"%gt; should be in the fieldset #title so it is next to the field name, like it is for other fields.

3. +++ b/core/modules/config_translation/src/FormElement/PluralVariants.php
   @@ -0,0 +1,82 @@
   +      '#tree' => TRUE,
   

   This should not be necessary, but should already be set in the form, no?

4. +++ b/core/modules/config_translation/src/FormElement/PluralVariants.php
   @@ -0,0 +1,82 @@
   +    for ($i = 0; $i < $plurals; $i++) {
   +      $element[] = array(
   

   The effect is the same, but I would find the code more readable if we did $element[$i] instead of $element[]. It just looks very non-standard to have form elements without an explicit key.

5. +++ b/core/modules/config_translation/src/FormElement/PluralVariants.php
   @@ -0,0 +1,82 @@
   +        '#markup' => SafeMarkup::format('<span lang="@lancode">@value</span>', array(
   +          '@langcode' => $source_language->getId(),
   

   Missing "g" in @lancode

All of theses also apply to getTranslationElement()

#59: I added a test to check the number of plural strings' source elements.

Now I'm going to address @tstoeckler's comments from #62.

@tstoeckler:

#62/1: It's in core/lib/Drupal/Core/StringTranslation/TranslationManager.php line 246

#62/2: I moved it to the fieldset.

#62/3: I don't think it is set on the form since the config doesn't get saved if I remove it.
EDIT: From core/modules/config_translation/src/Form/ConfigTranslationFormBase.php line 165:

// Even though this is a nested form, we do not set #tree to TRUE because                                                              
// the form value structure is generated by using #parents for each element.

#62/4: I added the index to the $element.

#62/5: Corrected.

Thanks for the review!

Great find on the #tree usage. I had totally forgotten about that, thanks for the explanation!

Test coverage looks totally sufficient now from my perspective.

I found last thing, that we should fix before this can be committed:

+++ b/core/modules/config_translation/src/FormElement/PluralVariants.php
@@ -25,18 +25,18 @@ protected function getSourceElement(LanguageInterface $source_language, $source_
-      '#title' => $this->t($this->definition->getLabel()),
+      '#title' => SafeMarkup::format('@label <span class="visually-hidden">(@source_language)</span>', array(
+        '@label' => $this->t($this->definition->getLabel()),
+        '@source_language' => $source_language->getName(),
+      )),

Just like FormElementBase this should use $this->t() on the outer level, i.e. instead of the SafeMarkup::format().

Hmm... I disagree, but more importantly, as noted above, FormElementBase contains the same code already. In any case, let's not hold this up on such a minor detail, we can always improve, and even if we don't no one should lose sleep over this.

RTBC.

@tstoeckler I think FormElementBase is weird. Translators have enough to do without translating things like '!label <span class="visually-hidden">(!source_language)</span>'... if this needs to be altered then it should be a template.

Committed 77b9dbc and pushed to 8.0.x. Thanks!

if this needs to be altered then it should be a template

As always, you make a very good point. I like that idea! Might open a ticket for that if I get around to it. In any case, thanks @everyone, especially @maxocub: Impressive persistence here!!! yay

Thanks! About the templates, there's already #2499651: Use inline_templates in TranslateEditForm::buildForm() and PluralString::getTranslationElement() that could be modified to include FormElementBase.