Postponed on #2454859: Not possible to format plural an already translated string.

Problem/Motivation

The views NumericField formatter and following #2449597: Number formatters: Make it possible to configure format_plural on the formatter level the numeric field formatters also support a singular/plural variant configuration for number display.

1. Storing them as separate singular/plural does not allow for translating them especially if there are multiple plural variants needed.
2. NumericField calls formatPlural() on them which translates them even though they already came translated from config translation. #2454859: Not possible to format plural an already translated string provides an API to use for us.
3. Even if they would be stored as one string, config translation UI lacks support for translating them. That will be solved in #2454829: Configuration translation UI does not support plural sources/targets, NOT HERE.

Proposed resolution

1. Modify the NumericField formatter in views to store singular and plural in one string separated by LOCALE_PLURAL_DELIMITER.
2. Change the assumption in Views that it is editing an English original configuration (allow more than a singular and plural).
3. Use the API provided by #2454859: Not possible to format plural an already translated string to format plurals from the already translated string.
4. Add tests.

Remaining tasks

Review.

User interface changes

The views numeric formatter will have a varied number of input fields depending on the number required by the language.

API changes

API change in terms of how singular/plural strings are stored in views configuration. No API changes in PHP.

Files: 
CommentFileSizeAuthor
#33 interdiff.txt1.91 KBGábor Hojtsy
#33 2453761-33.patch29.61 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,572 pass(es). View
#31 interdiff.txt8.09 KBGábor Hojtsy
#31 2453761-31.patch29.6 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,589 pass(es), 0 fail(s), and 72 exception(s). View
#28 view-plural.png246.44 KBrteijeiro
#28 view-config.png140.55 KBrteijeiro
#26 interdiff.txt5.33 KBGábor Hojtsy
#26 2453761-26.patch26.71 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,567 pass(es). View
#19 2453761-19.patch25.7 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,663 pass(es). View
#18 2453761-18.patch14.09 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,583 pass(es). View
#15 interdiff.txt4.92 KBGábor Hojtsy
#15 2453761-15.patch31.38 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,261 pass(es). View
#14 2453761-14.patch31.94 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,260 pass(es). View
#14 interdiff.txt14.26 KBGábor Hojtsy
#14 2453761-14.patch31.94 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,267 pass(es). View
#13 2453761-13.patch18.68 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,194 pass(es). View
#13 interdiff.txt1.75 KBGábor Hojtsy
#9 interdiff.txt6.24 KBGábor Hojtsy
#9 2453761-9.patch18.65 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,179 pass(es), 5 fail(s), and 0 exception(s). View
#7 2453761-7.patch16.62 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,188 pass(es), 7 fail(s), and 8 exception(s). View
#7 interdiff.txt11.55 KBGábor Hojtsy
#4 drupal] 2015-03-17 12-12-46.png97.23 KBGábor Hojtsy
#3 interdiff.txt1.46 KBGábor Hojtsy
#3 2453761-2.patch6.08 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,192 pass(es). View
#1 2453761-1.patch4.62 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,201 pass(es). View

Comments

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
4.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,201 pass(es). View

Starting with the formatPlural() refactor.

vijaycs85’s picture

1.

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
@@ -183,7 +190,13 @@ public function formatPlural($count, $singular, $plural, array $args = array(),
-    return SafeMarkup::set($return);
+
+    if (empty($args)) {
+      return SafeMarkup::set($return);
+    }
+    else {
+      return String::format($return, $args);
+    }

Not sure why it's necessary as it is already done at

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
@@ -157,8 +157,15 @@ public function formatPlural($count, $singular, $plural, array $args = array(),
     $translated_strings = $this->translate($translatable_string, $args, $options);

and AFAICS, this the only change that's going to change the behaviour. If we have to, a comment would be great.

2. Do we have any other use case where we call formatPluralTranslated() directly? Can we add that change as well part of this issue.

Gábor Hojtsy’s picture

FileSize
6.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,192 pass(es). View
1.46 KB

Sat down to modify the config translation forms but this needs some serious thinking and some rearchitecture there. :/ Adding some @todo items at least to help someone who may work on this.

Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
97.23 KB

@vijaycs85: so the use case of this issue is configuration translation. That will translate the source string in config translation, but still needs support for argument replacement, so that is the reason for 1. The use case I know now is Numeric in views, which will need storage adjustments to store the singular/plural value as one string. Now its broken because it stores the values seaparately, they are translatable separately in config translation and then it still passes it into format_plural() where they are translated again. For one, config sourced data should not be passed into format_plural() like its English. It may or may not be. Second, stored as separate singular and plural, its impossible to translate to languages that have more plural forms. And then the harder part is the config translation UI see #3.

Gábor Hojtsy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Issue summary: View changes

Heh, that is also a problem that Views assumes that the config being edited is of a singular/plural combination not multiple plurals. You cannot create a native Polish view that has correct values there.

Gábor Hojtsy’s picture

FileSize
11.55 KB
16.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,188 pass(es), 7 fail(s), and 8 exception(s). View

This update fixes several things:

1. Exposes the formatPluralTranslated() method on the string translation trait, so Numeric an use it.
2. Adds a new getNumberOfPlurals() method on TranslationManager (and tunneled via string translation trait) so modules like views can display the correct UI for input of singular/plural values.
3. Copied the singular/plural UI logic from TranslateEditForm to Numeric, so that it allows entering multi-plurals now. Added a proxy value element for putting the data to at the end.
4. Added an override of submitOptionsForm() to Numeric, so that it can mangle the form options data before it is passed on. This way we can join the data again here and avoid the individual fields ending up in config.
5. Modified render() so that it now properly uses the formatPluralTranslated(). Even before this patch, it was already dealing with translated data.
6. Modified config schema and all existing views to use the new single string storage. Hope the encoding for the \03 char I found will work with our YAML parser.

This should in itself allow Polish views being created with the right number of plurals and that being rendered. It also allows for locale integration with support for translation via locale. It does not yet solve the problem of config translation integration (translate from any language to any language).

Also added a @todo to the patched code because we need the language of the view, not the interface language to be correct.

Status: Needs review » Needs work

The last submitted patch, 7: 2453761-7.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
18.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,179 pass(es), 5 fail(s), and 0 exception(s). View
6.24 KB
73 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Fails were due to the following problems:

- As per http://symfony.com/doc/current/components/yaml/yaml_format.html strings that use escape sequences like \x03 should use double quotes.
- That same page does not document allowed unicode escapes, so using \x03 now.
- @count was not added in the translated format plural, it should be added there so it is replaced in plural variants.
- The TranslationManager was attempting to use an injected state that was not defined. Injecting that but making it optional so we don't need to build up a key-value store and a state service in the installer just to simulate an empty state for this class.

We can also break out the config translation UI support for this into another issue if we want to make this land sooner.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

The last submitted patch, 9: 2453761-9.patch, failed testing.

The last submitted patch, 9: 2453761-9.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
18.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,194 pass(es). View

The formatPluralTranslated() did not do @count replacement in the translation anymore for single value strings, if it contained @count (or any of the other placeholders). This was previously replaced in formatPlural() through the translate() method itself. We can do that replacement directly now there for the singular case. Also, the empty($args) is a total nonsense, we always add @count, so it can never be empty ever (if it could be, we would need to complicate the line I'm fixing here too).

This should be green again.

Gábor Hojtsy’s picture

FileSize
31.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,267 pass(es). View
14.26 KB
31.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,260 pass(es). View

Updates with tests and issues found while testing:

1. The logic in plural index selection was wrong. We cannot assume that $count == 1 should use the "singular" item, languages like Slovenian would use the first "plural" item for 1. We can do that logic fallback if the index we wanted did not exist. (This came up due to the test using Slovenian rules).

2. Added testing of formatPluralTranslated() itself to LocalePluralFormatTest.

3. Added the new test as NumericFormatPluralTest in views using a test view.

Gábor Hojtsy’s picture

Title: Configuration translation does not support singular/plural translation » Views numeric formatter's plural formatting setting incompatible with many languages
FileSize
31.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,261 pass(es). View
4.92 KB

Wanted to cover the Slovenian rules in LocaleFormatPluralTest because it required logic change in formatPlural(), BUT then I went to localize.drupal.org / l10n_pconfig to cross-check and looks like we use slightly different index meanings (my source for the prior patch was http://localization-guide.readthedocs.org/en/latest/l10n/pluralforms.html). So defaulting to index 0 for $count 1 is correct with our rules. No wonder this was never a problem :) So fixing the Slovenian rules in the new test and rolling back the formatPlural() logic changes.

Also opened a followup for the config translation UI problems at #2454829: Configuration translation UI does not support plural sources/targets and referring that now, so we don't need to solve it in this scope.

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs review » Postponed
Issue tags: +Configuration schema

So I gave this issue a new title in #15 above. I did not manage to decide on a good title because this issue covered two things. So instead of trying to cram it in, I opened #2454859: Not possible to format plural an already translated string and posted the scope that covers that (which has testing and all). Let's get that to RTBC ASAP and then we can get this in.

jhodgdon’s picture

Status: Postponed » Active

Time to un-postpone!

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
14.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,583 pass(es). View

Rerolled. The patch was failing due to PHP 7 related changes, optional config moved and #2454859: Not possible to format plural an already translated string. All those are adjusted now. Note that the patch is much smaller due to #2454859: Not possible to format plural an already translated string :)

Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
25.7 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,663 pass(es). View

The newly added test files were missing from the reroll. Ups.

Status: Needs review » Needs work

The last submitted patch, 19: 2453761-19.patch, failed testing.

Status: Needs work » Needs review

jhodgdon queued 19: 2453761-19.patch for re-testing.

jhodgdon’s picture

Nitpick:

+++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
@@ -64,6 +64,17 @@ protected function formatPluralTranslated($count, $translated, array $args = arr
   }
 
   /**
+   * Returns number of plurals supported by a given language.
+   *
+   * See the
+   * \Drupal\Core\StringTranslation\TranslationInterface::getNumberOfPlurals()
+   * documentation for details.
+   */
+  protected function getNumberOfPlurals($langcode = NULL) {

First line shoudl be "Returns *the* number of plurals. Same in core/lib/Drupal/Core/StringTranslation/TranslationInterface.php ... where there is another nitpick:
,
+ * will be used.
+ * @return int
+ * Number of plural variants supported by the given language.
+ */
+ public function getNumberOfPlurals($langcode = NULL);

Needs blank line before @return.

Guess there are still some @todos in core/modules/config_translation/src/FormElement/FormElementBase.php that are pointing to this issue so I stopped reviewing...

Gábor Hojtsy’s picture

@jhodgdon: the @todos in FormElementBase point to #2454829: Configuration translation UI does not support plural sources/targets which is not this issue?! There are two @todos that need to be resolved in this issue, but I don't know how, we need some views know-how there... How does a handler know its view?

jhodgdon’s picture

Ah, sorry, read the wrong issue number there.

So on the actual @todo lines... One of them is in core/modules/views/src/Plugin/views/field/NumericField.php and it says "@todo Figure out how to pass in the language of the view.". What do you mean here by "language of the view"? There are quite a few languages floating around in views *output*, but I'm not aware of the "view" having a language itself... oh I suppose you mean the language the config for the view is stored in, so that you can translate it? Hm.

Anyway there's a public property PluginBase::view on any handler that will get you the view.

dawehner’s picture

In general it looks great!

  1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -53,14 +54,24 @@ class TranslationManager implements TranslationInterface, TranslatorInterface {
    +  public function __construct(LanguageManagerInterface $language_manager, StateInterface $state = NULL) {
    
    @@ -229,4 +240,20 @@ public function reset() {
    +    if (isset($this->state)) {
    

    I'm curious, why do allow state to be optional? Do you want to not break other code?

  2. +++ b/core/modules/views/src/Tests/Plugin/NumericFormatPluralTest.php
    @@ -0,0 +1,158 @@
    +class NumericFormatPluralTest extends ViewTestBase {
    ...
    +  protected function setUp() {
    +    parent::setUp();
    +
    +    $this->web_user = $this->drupalCreateUser(array('administer views', 'administer languages'));
    +    $this->drupalLogin($this->web_user);
    +  }
    

    You can save a couple of lines by using \Drupal\views_ui\Tests\UITestBase

Gábor Hojtsy’s picture

FileSize
26.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,567 pass(es). View
5.33 KB

Thanks @jhodgdon and @dawehner on IRC, I now know how to take the langcode of the view. So taking that and modifying in the test rather than requesting it in a specific language to edit. Also venturing into adding a config translation and verifying that works too.

@dawehner re #25/1: the installer uses the translation manager too and there is no state service in the installer:

  $container
    ->register('string_translation', 'Drupal\Core\StringTranslation\TranslationManager')
    ->addArgument(new Reference('language_manager'));

If we want to make sure there is a state service at that point, then we need to ensure that a keyvalue service exists, because state depends on keyvalue. Then keyvalue injects the container itself and a service factory, and so on. It looks like a pretty scary domino effect that we'd only need to deal with so that we have some kind of mocked support for this in the installer where we'll not likely to be using it at all...

Gábor Hojtsy’s picture

@dawehner: re #25/2 it does not seem to help with the administer language permission, so I would need to somehow work around that, so would not be saving much there I guess.

rteijeiro’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
140.55 KB
246.44 KB

Code looks good and it seems to work.

View with Number Field and Format Plural Enabled

View Config Export

Gábor Hojtsy’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/views/src/Plugin/views/field/NumericField.php
    @@ -93,28 +92,54 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +    if ($plurals > 2) {
    ...
    +    else {
    

    Is this special casing actually worth it?

  2. +++ b/core/modules/views/src/Plugin/views/field/NumericField.php
    @@ -93,28 +92,54 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +      // Fallback for unknown number of plurals.
    

    This comment seems incomplete - a language might just have have 2 forms.

  3. +++ b/core/modules/views/src/Tests/Plugin/NumericFormatPluralTest.php
    @@ -0,0 +1,172 @@
    +use Drupal\Component\Utility\Unicode;
    +use Drupal\language\Entity\ConfigurableLanguage;
    

    Not used.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
29.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,589 pass(es), 0 fail(s), and 72 exception(s). View
8.09 KB

@alexpott: the special casing was so that the most common singular / plural case, you would not get confusing UI text like 'First plural form'. We can also generate the form and *then* modify the UI text accordingly. Then we should do the same in the place where I took this pattern from (TranslateEditForm) which has the same problem. They should use the same pattern for others to follow. This obsoleted your second review point. Also addressed your third.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 31: 2453761-31.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
29.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,572 pass(es). View
1.91 KB

Helps to use the right keys.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Ok thanks @Gábor Hojtsy. This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 71ebe23 and pushed to 8.0.x. Thanks!

  • alexpott committed 71ebe23 on 8.0.x
    Issue #2453761 by Gábor Hojtsy: Views numeric formatter's plural...
Gábor Hojtsy’s picture

Issue tags: -sprint

Status: Fixed » Closed (fixed)

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