Problem/Motivation

Following the introduction to plural sources in #2453761: Views numeric formatter's plural formatting setting incompatible with many languages, config translation needs to support exposing a user interface to display singular/plural sources and plural targets. It does support translating those now so long as users carefully insert EXT (\x03) manually between the variants, which is a no-go in terms of UX.

Proposed resolution

Implement UI similar to TranslateEditForm::buildForm().

Remaining tasks

Review.

User interface changes

Singular/plural sources would show properly, targets would provide multiple input fields appropriate for the language.

Before
After

Multiple sources when translating from Slovenian to Russian:
Slovenian to Russian

Singular only when translating from Vietnamese to Irish:
Vietnamese to Irish

The plural forms labels from this last screenshot should be corrected by #2499639: Use better labels for numeric fields when using a multiple plural forms language.

API changes

1. Added a new config schema type: 'plural_string'
2. Added a new config translation form element: 'PluralString'

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because of bad UX
Issue priority Major because of bad UX
Unfrozen changes Unfrozen because it only changes the user interface of configuration translation of plural strings
Prioritized changes The main goal of this issue is usability
Disruption Not disruptive
Files: 
CommentFileSizeAuthor
#66 interdiff.txt3.33 KBmaxocub
#66 plural_strings_translation-2454829-65.patch15 KBmaxocub
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,355 pass(es). View
#63 interdiff.txt4.78 KBmaxocub
#63 plural_strings_translation-2454829-63.patch15.01 KBmaxocub
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,374 pass(es). View
#63 test_only.patch3.68 KBmaxocub
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,329 pass(es), 7 fail(s), and 0 exception(s). View
#57 interdiff.txt10.21 KBmaxocub
#57 plural_strings_translation-2454829-57.patch12.54 KBmaxocub
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,181 pass(es). View

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Issue tags: +sprint

Put on sprint in hopes that we can get to start work on this. I'll not be able to personally devote time to drive this in the foreseeable future :/ Can help with reviews. Practically this issue needs to resolve these two @todos:

diff --git a/core/modules/config_translation/src/FormElement/FormElementBase.php b/core/modules/config_translation/src/FormElement/FormElementBase.php
index e13cc31..7db01e1 100644
--- a/core/modules/config_translation/src/FormElement/FormElementBase.php
+++ b/core/modules/config_translation/src/FormElement/FormElementBase.php
@@ -91,7 +91,6 @@ public function getTranslationBuild(LanguageInterface $source_language, Language
    *   A render array for the source value.
    */
   protected function getSourceElement(LanguageInterface $source_language, $source_config) {
-    // @todo Should support singular+plurals https://www.drupal.org/node/2454829
     if ($source_config) {
       $value = '<span lang="' . $source_language->getId() . '">' . nl2br($source_config) . '</span>';
     }
@@ -162,7 +161,6 @@ protected function getSourceElement(LanguageInterface $source_language, $source_
    */
   protected function getTranslationElement(LanguageInterface $translation_language, $source_config, $translation_config) {
     // Add basic properties that apply to all form elements.
-    // @todo Should support singular+plurals https://www.drupal.org/node/2454829
     return array(
       '#title' => $this->t('!label <span class="visually-hidden">(!source_language)</span>', array(
         '!label' => $this->t($this->definition['label']),
rvilar’s picture

Assigned: Unassigned » rvilar

I'm working on it

rvilar’s picture

Status: Active » Needs review
FileSize
3.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,431 pass(es), 4 fail(s), and 30 exception(s). View

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?

rvilar’s picture

Issue tags: +drupaldevdays

Added the tag for devdays review

Status: Needs review » Needs work

The last submitted patch, 4: 2454829-4.patch, failed testing.

fran seva’s picture

Assigned: rvilar » Unassigned
Jose Reyero’s picture

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

maxocub’s picture

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.

Gábor Hojtsy’s picture

@Jose Reyero: The prefix and suffix are already translatable on numeric fields, the decimal point and separator are not, that is in itself easy to fix (making them "label"s). Introducing a more high level type to support for translation as a whole is a whole new question not in scope for this issue IMHO. The plural translation solution has beenfits other than the numeric fields in views.

maxocub’s picture

FileSize
2.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,040 pass(es). View

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’s picture

Status: Needs work » Needs review
Jose Reyero’s picture

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

maxocub’s picture

Status: Needs review » Needs work
maxocub’s picture

Status: Needs work » Needs review
FileSize
4.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,407 pass(es). View

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.

maxocub’s picture

FileSize
20.33 KB

Here's what it looks like:slovenian

maxocub’s picture

FileSize
4.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,347 pass(es). View
2.02 KB

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?

eiriksm’s picture

FileSize
6.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,612 pass(es). View
2.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,167 pass(es), 8 fail(s), and 0 exception(s). View

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

maxocub’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 18: configuration-2454829-18-test-only.patch, failed testing.

eiriksm’s picture

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?

Gábor Hojtsy’s picture

@eiriksm: re #21 we are adding support to the UI here, so we should test the UI IMHO.

vijaycs85’s picture

Status: Needs work » Needs review

Looks like it's 'needs review'

maxocub’s picture

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.

eiriksm’s picture

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

maxocub’s picture

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

Status: Needs review » Needs work

The last submitted patch, 18: configuration-2454829-18-test-only.patch, failed testing.

maxocub’s picture

Status: Needs work » Needs review
penyaskito’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs screenshots

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.

maxocub’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -drupaldevdays, -Needs tests, -Needs screenshots
FileSize
10.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,183 pass(es). View
9.28 KB
20.2 KB
30.7 KB

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

maxocub’s picture

Issue summary: View changes
eiriksm’s picture

Status: Needs review » Reviewed & tested by the community

LGTM!

Patch works and screenshots are descriptive.

penyaskito’s picture

Status: Reviewed & tested by the community » Needs work
  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!

maxocub’s picture

FileSize
62.84 KB

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

interface_translation.png

penyaskito’s picture

Issue tags: +Needs beta evaluation

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

maxocub’s picture

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?

penyaskito’s picture

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

maxocub’s picture

Issue summary: View changes

Updated the summary and added the beta evaluation template.

maxocub’s picture

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Usability
  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.

maxocub’s picture

Status: Needs work » Needs review
FileSize
3.13 KB
10.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,394 pass(es). View

Status: Needs review » Needs work

The last submitted patch, 43: plural_strings_translation-2454829-43.patch, failed testing.

Gábor Hojtsy’s picture

Does not seem to be a related fail.

Drupal\views\Tests\Wizard\TaggedWithTest                      78 passes
Drupal\views_ui\Tests\TagTest                                 16 passes
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated
Terminated].
[05:57:38] Encountered error on [review], details:
array (
  '@reason' => 'failed during invocation of run-tests.sh',
)
[05:57:39] Review complete. test_id=1068233 result code=8 details=Array
(
    [@reason] => failed during invocation of run-tests.sh
)

Status: Needs work » Needs review
penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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?

maxocub’s picture

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 better 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’s picture

Status: Needs review » Needs work

@maxocub: well, formatPlural() is supposed to be used with English source strings only. Configuration may or may not be originally in English. It may have from 1 (eg. Vietnamese) to 5 (eg. Irish) or even 6 (eg. Arabic) variants depending on language. So displaying exactly two forms may or may not even work. The source display AFAIS should use the same code to display variants as the form input code does.

maxocub’s picture

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

Gábor Hojtsy’s picture

In TranslateEditForm / locale module / formatPlural(), the source is always English, at least unless someone misuses the API, so the source always uses two plural forms. In configuration, the source may be any language, so there may be 1 to 6 variants (as opposed to a fixed 2) both in the source and the target. See eg. #2453761: Views numeric formatter's plural formatting setting incompatible with many languages which is a form for editing original configuration with a plural setting. That has the varied plural input for the config value too.

maxocub’s picture

Status: Needs work » Needs review
FileSize
10.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,014 pass(es). View
2.38 KB
19.22 KB
15.16 KB
13.78 KB

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:
Slovenian to Russian

And from Vietnamese to Irish:
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 better 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 better labels for numeric fields when using a multiple plural forms language, or postpone this issue and fix #2499639: Use better labels for numeric fields when using a multiple plural forms language first?

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update
  1. +++ b/core/config/schema/core.data_types.schema.yml
    @@ -56,6 +56,12 @@ label:
    +plural_string:
    

    I know @reyero prefered plural_string as the type name, but I indicated that if we consider this a special case of label (which it is), then it should be plural_label. Not all strings that we use the 'label' type for are actual labels, but the label type is translatable, the string type is not. The plural_string type makes it look like this is a derivative of strings, while its more logically a derivative of labels. (Feel free to actually inherit it from label too).

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

    Let's get this on one line if we can. Eg. "Defines form elements for plurals in configuration translation."

  3. +++ b/core/modules/config_translation/src/FormElement/PluralString.php
    @@ -0,0 +1,81 @@
    +    $plurals = $this->getNumberOfPlurals($source_language->getId());
    ...
    +    for ($i = 0; $i < $plurals; $i++) {
    ...
    +          '@label' => $i == 0 ? $this->t('Singular form') : $this->formatPlural($i, 'First plural form', '@count. plural form'),
    ...
    +    $plurals = $this->getNumberOfPlurals($translation_language->getId());
    ...
    +    for ($i = 0; $i < $plurals; $i++) {
    ...
    +          '@label' => $i == 0 ? $this->t('Singular form') : $this->formatPlural($i, 'First plural form', '@count. plural form'),
    

    I think this displays the incorrect plural labels because you are not passing over the language to use for the plural logic. I see TranslateEditForm and NumericField have the same code. So these will not be good :D We can either try to argue around it here and ignore the problem and solve it in #2499639: Use better labels for numeric fields when using a multiple plural forms language or solve it here but then the scope would expand :(

    So I feel like better would be to solve in #2499639: Use better labels for numeric fields when using a multiple plural forms language. However, the solution is not just changing text but also changing logic, because we need to send over the right language to use for the plural variant *selection* but not for translating the item. So it would properly say "3. plural form" even if English (the UI language used to display it) does not support a 3rd plural.

    One way to do that would be to do $this->t('First plural form' . LOCALE_PLURAL_DELIMITER . '@count. plural form') (do not pass in a langcode, so the translation happens to the interface) and then $this->formatPluralTranslated() and pass in the langcode there, so the right logic is picked. That would fall down if the translation replaces "@count plural form" with "Plural variant" or something though...

    Anyway, I see this is an existing problem in the code copied around 3rd time now, so I am inclined to defer to #2499639: Use better labels for numeric fields when using a multiple plural forms language. Let's add a @todo at least then referring to there above the two formatPlural() lines, so we track it.

Finally, the issue summary needs updating with new screenshots.

Gábor Hojtsy’s picture

+++ b/core/modules/views/config/schema/views.field.schema.yml
@@ -117,7 +117,7 @@ views.field.numeric:
       label: 'Singular and one or more plurals'

Oh, this label is also pre-existing, but it looks silly if you look at the UI (this patch is the first fixing this UI, so let's fix it here). If you look at Vietnamese screenshot its evidently wrong, there is only one form, not "singular and one or more plural". So maybe "Plural variants" or something would be better.

maxocub’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
maxocub’s picture

Status: Needs work » Needs review
FileSize
12.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,181 pass(es). View
10.21 KB

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

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

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’s picture

@Maxocub: can you work on this one still?

maxocub’s picture

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

tstoeckler’s picture

  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 &lt;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()

maxocub’s picture

FileSize
3.68 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,329 pass(es), 7 fail(s), and 0 exception(s). View
15.01 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,374 pass(es). View
4.78 KB

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

maxocub’s picture

Status: Needs work » Needs review

The last submitted patch, 63: test_only.patch, failed testing.

maxocub’s picture

FileSize
15 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,355 pass(es). View
3.33 KB

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

tstoeckler’s picture

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

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review

@tstoeckler: not sure about that format/t, there is nothing translatable in the text it is only used for formatting with some HTML. The patch has several of those and @alexpot complained in #42 that it was a t() and said it should be safemarkup format :D

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed 77b9dbc on
    Issue #2454829 by maxocub, eiriksm, rvilar, Gábor Hojtsy, penyaskito,...
Gábor Hojtsy’s picture

tstoeckler’s picture

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

maxocub’s picture

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

Status: Fixed » Closed (fixed)

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