Problem/Motivation

In #2453761: Views numeric formatter's plural formatting setting incompatible with many languages we have seen that Views is using configuration to store singular/plural string settings. This configuration may not be English, it may be Polish or Slovenian. It also may be translated in config translation. So using format_plural() on it is not possible because that would consider the singular/plural string as English source strings and translate them t(). We introduced String::format() to deal with formatting strings with placeholders without translation, but format_plural() does not have that equivalent.

Proposed resolution

Introduce a TranslationManager::formatPluralTranslated() and a StringTranslationTrait::formatPluralTranslated(). (While renaming formatPlural() to translatePlural() and introducing a formatPlural() that does not do translation would be in line with how String::format() is done, that would be too much of an API change now).

Add tests.

Remaining tasks

Review.

User interface changes

None.

API changes

None, just API additions.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

jhodgdon’s picture

Ah, I was just reviewing that other issue's patch. I think my review belongs here instead.

---

Mostly looking good!

I took a look through the patch, with (of course) a focus on the API docs. A few nitpicks:

a) There's a reference "See format_string()" in the formatPluralTranslated() docs on the interface. But that function has very little documentation. The docs are on \Drupal\Component\Utility\String::format(), so I think that would be a better "see".

b) On that same method:

+   * @param array $options
+   *   An associative array of additional options. t() defines all allowed keys.
+   *   The 'context' key is not supported because the passed string is already
+   *   translated.

I'm very confused about why $options is needed on this function? t() says that the only options supported are 'langcode' and 'context'. But this function does not translate, so why would it even care about t() options such as these, and if 'langcode' is the only option that would be supported, how would that be used in a function that is not doing translation?

c) And in that same interface:

+  /**
+   * Returns number of plurals supported by a given language.
+   *
+   * @param null $langcode
+   *   (optional) The language code. If not provided, the current language
+   *   will be used.
+   * @return int
+   *   Number of plural variants supported by the given language.
+   */
+  public function getNumberOfPlurals($langcode = NULL);

- Should say "Returns **the** number...". [Also true for the Trait method of the same name]
- I think the $langcode is a string|null?
- Needs blank line between @param and @return

d) A question... So why do we even have TranslationInterface::formatPlural() taking $singular and $plural arguments, if this is totally the wrong way to do translations??!? (and the corresponding TranslationManager method) It seems like anyone using this method is doing it wrong? A grep through the Drupal Core code base for formatPlural finds 139 hits... ?? I don't get it. It looks like Drupal 7 did the same thing. ... Oh, I see. So the English-based code can use format_plural() or formatPlural() to define the singular/plural forms for English. Behind the scenes, the PO extractor must be joining these together into a single translation string (???), and then the method formatPlural() is looking up this combined string and translating that, then passing into the new formatPluralTranslated() method to actually format the string.

OK, I get it. Leaving this here in case anyone else is confused too. ;)

Note that Drupal 7 does *not* work the same way quite. It seems as though it stored the singular form in one translatable string, and the plural in another.

Gábor Hojtsy’s picture

FileSize
7.4 KB
1.53 KB

@dawehner suggested code comments to discourage people from doing the (normally silly) thing that we do in the test. We just do it because its simpler to assert the return values that way.

Gábor Hojtsy’s picture

FileSize
7.56 KB
1.56 KB

@jhodgdon:

a) Sure, that required fixing the other method, did that too.

b) The plural logic depends on the language, so even if we don't do translation, we need to use the right language.

c) That applies to code in #2453761: Views numeric formatter's plural formatting setting incompatible with many languages, not here. Will address there, when we get back there.

d) Yeah format_plural() assumes an English source, which makes it possible to define them as a singular and a plural string. Sources in other languages may have any number of variants. The existing fomat_plural()/formatPlural() logic is good, things coming from PHP code should be English. We actually need this new API for things not coming from PHP code, such as config, which may be in any other language.

jhodgdon’s picture

Status: Needs review » Needs work

Better!

Taking a look at this patch:

a) In TranslationInterface::formatPlural() docs:

    *   themed. See format_string(). Note that you do not need to include @count
-   *   in this array; this replacement is done automatically for the plural case.
+   *   in this array; this replacement is done automatically for the plural
+   *   cases.

Can we please not say "see format_string()" here? See #2-a.

b) Also the same in the formatPluralTranslated() docs:

+   *   Based on the first character of the key, the value is escaped and/or
+   *   themed. See format_string(). Note that you do not need to include @count

c) Same method:

+   * @param array $options
+   *   An associative array of additional options. t() defines all allowed keys.
+   *   The 'context' key is not supported because the passed string is already
+   *   translated. Use the 'langcode' key to ensure the proper plural logic is
+   *   used.

It seems like the only key we are interested in here is 'langcode'. Rather than having to go to t() and then read about 'langcode' and 'context', and then discard what it says about 'langcode', we'd be better off just documenting that you only need langcode. Or better yet just put in a 'langcode' argument instead of $options? Seems much clearer. I checked and all that is being used in this new method from $options is explicitly $options['langcode'], which is being passed to locale_get_plural().

Other than these details of the interface docs, and correspondingly maybe needing a small code change, I think this is looking good!

mrjmd’s picture

Status: Needs work » Needs review
FileSize
7.74 KB
2.38 KB

Here's an attempt at cleaning up the documentation. I did not switch out the argument in formatPluralTranslated().

My two cents is that the $options array is already used all over the place, so even if we don't need anything but the 'langcode' key it seems more consistent to just leave it be. But I'll leave that decision to people who know the code better than I do.

Gábor Hojtsy’s picture

Thanks @mrjmd! @jhodgdon: as @mrjmd said, we use the same $options array on t(), TranslationManager::translate(), StringTranslationTrait::t(), format_plural(), TranslationManager::formatPlural() and StringTranslationTrait::formatPlural() at least. I'd love to avoid getting into a haystack/needle hole (as in PHP's string functions take haystack and needle in different order and different types and everybody hates that). Because every other function in this family uses the same $options array, even though we only support one option here, I think it makes sense to keep it for consistency.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Fair enough on $options! The new docs look good to me. Thanks @rmjmd for the patch on that!

This seems to be RTBC to me. The docs are good, there's a test, and the code changes are straightforward. Plus, it's blocking another issue. Let's do it!

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
@@ -152,16 +152,20 @@ public function translate($string, array $args = array(), array $options = array
+    return $this->formatPluralTranslated($count, $translated_strings, $args, $options);

I don't think it is necessary to pass $args here. Doesn't this add a cost of doing more strtr() because of the additional calls to String::format() - but it won't have to do anything because the replacements have been done?

Gábor Hojtsy’s picture

@alexpott: it will formulate a single item $args in formatPluralTranslated() at least with a @count index. The 90% (and probably more) use case I've seen is that there is no other replacement item in $args. So we can pass an empty array here but its likely not saving us much in most cases. I'll post an update soon.

Gábor Hojtsy’s picture

FileSize
9.69 KB
3.54 KB

Discussed the patch with @alexpott. He found it problematic that we call String::format() multiple times and also that we store the translated full string as a safe string even though we are never going to use it. So in this update:

1. Introducing a private method to do the common parts of the translation and non-translation based method.
2. Add a fake ! argument, so the concatenated string will not be stored as safe.
3. In the new private method compute whether the original replacements should make us assume the string is safe or not (because at this point the replacements are already made, but the resulting string may still be unsafe). That required us to keep passing in $args even though we don't use the value of that.

alexpott’s picture

FileSize
2.47 KB
9.33 KB

I think there might be an easier way to remove the SafeMarkup::set() and the fact that we make the string containing LOCALE_PLURAL_DELIMITER safe. How about this... interdiff to #6

Gábor Hojtsy’s picture

This modifies the t() case too to invoke one more method but that already is way deep down in the method call chain, so I don't think this is significant for it. Looks fine for me.

alexpott’s picture

Issue tags: +Security improvements

This patch is a security improvement since it ensures that not everything passed through formatPlural is marked safe.

Gábor Hojtsy’s picture

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

I think the new patch is good. It looks like Gabor thinks so too, and Alex wrote it so I assume he does too. That's probably enough. :)

Gábor Hojtsy’s picture

catch’s picture

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

Given #2454447: Split Utility\String class to support PHP 7 (String is a reserved word) I don't think the API change here would have been too bad.

On the other hand not changing the API is also fine.

Doesn't apply though.

mrjmd’s picture

Working on a reroll now.

mrjmd’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
9.33 KB
alexpott’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
@@ -70,19 +70,55 @@ public function translate($string, array $args = array(), array $options = array
    * @see \Drupal\Component\Utility\String
...
+   * @see \Drupal\Component\Utility\String::format()
...
+   * @see \Drupal\Component\Utility\String::format()

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
@@ -129,6 +129,30 @@ public function getStringTranslation($langcode, $string, $context) {
+      return String::format($string, $args);

@@ -137,30 +161,27 @@ public function translate($string, array $args = array(), array $options = array
+      return String::format($translated_array[0], $args);

@@ -182,7 +203,8 @@ public function formatPlural($count, $singular, $plural, array $args = array(),
+    return String::format($return, $args);

Let's update all of these now that String::format() is deprecated and replaced by SafeMarkup::format()

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned

@mrjmd: thanks for the help, feel free to continue :)

The last submitted patch, 20: 2454859-20.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.38 KB
9.35 KB

Fixing patch.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Straightforward fixes. Thanks!

alexpott’s picture

I've done some profiling. On doing 1000 calls to t() this change costs us 15ms. See https://blackfire.io/profiles/compare/4cd2a39a-de60-4c64-a501-b8bb3927ce....

Should we move the doTranslate() method to be inline?

Wim Leers’s picture

1000 calls doesn't seem impossible for a single page load. How many t() calls do we have on a typical page load?

15 ms seems like an enormous amount of time for 1000 calls.

alexpott’s picture

@catch pointed out that this is probably blackfireio overhead. @catch thinks it's about

1ms for 1000 function calls.

  • catch committed 1c9ddc4 on 8.0.x
    Issue #2454859 by Gábor Hojtsy, alexpott, mrjmd: Not possible to format...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks! If we see problems later we could still inline this, but I really think blackfire.io is not helping here.

Gábor Hojtsy’s picture

Issue tags: -sprint

Yay! Thanks all! Thanks mrjmd!

Status: Fixed » Closed (fixed)

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