Problem/Motivation

As reported in #2545730-2: Misuse of formatPlural() in Numeric field prefix/suffix \Drupal\Core\Validation\DrupalTranslator::transChoice() uses \Drupal::translation()->formatPlural() in what looks like an incorrect manner. Here's what it's doing:

    // Violation messages can separated singular and plural versions by "|".
    $ids = explode('|', $id);

    if (!isset($ids[1])) {
      throw new \InvalidArgumentException(sprintf('The message "%s" cannot be pluralized, because it is missing a plural (e.g. "There is one apple|There are @count apples").', $id));
    }
    return \Drupal::translation()->formatPlural($number, $ids[0], $ids[1], $this->processParameters($parameters), $this->getOptions($domain, $locale));
  }

However, if transChoice() is used with messages that are defined on a constraint class, that is already supported by potx, see #1903362: Support for Drupal 8 validation constraint messages. We should document this usage.

Proposed resolution

Document it.

Remaining tasks

Commit.

User interface changes

None.

API changes

None.

Data model changes

None.

Brief beta eval: This is just adding a code comment (documentation). No disruption. Can be committed any time.

CommentFileSizeAuthor
#8 2552867-document-nonstandard-format-plural.patch1.43 KBjhodgdon
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented

pjonckiere created an issue. See original summary.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented
Related issues: +#2545730: Misuse of formatPlural() in Numeric field prefix/suffix
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Issue summary: View changes

Adding details from the other issue to the summary here.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

formatPlural() assumes it can translate the English input by pasting it together with a special string that is NOT |, and look up the translation to the target language. Here, the string that is the source has | as the separator. So, formatPlural() will not be able to find the translation and provide the correct output in other languages.

The code does not actually use that in formatPlural()?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Here's the code:

 // Violation messages can separated singular and plural versions by "|".
 $ids = explode('|', $id);
 return \Drupal::translation()->formatPlural($number, $ids[0], $ids[1], $this->processParameters($parameters), $this->getOptions($domain, $locale));

So the source string is $id and it indeed has singular/plural forms separated by | and is passing those into formatPlural().

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

I see, so the concern is the extractability of the source string first. Let's see, #1903362: Support for Drupal 8 validation constraint messages will extract the | separated string in potx for constraint messages. This seem to be only used for constraint messages, right? Or is it used for other things as well?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Title: Fix DrupalTranslator::transChoice()'s formatPlural() usage » Document DrupalTranslator::transChoice()'s non-standard formatPlural() usage
Status: Active » Needs review
FileSize
2552867-document-nonstandard-format-plural.patch1.43 KB

Ah, so that is very interesting. In that case it looks like DrupalTranslator::transChoice() should be OK to use the | character in its constraint messages and call formatPlural(), because we've specially modified the POTX extractor to make this work.

So for this issue I think we should just add a comment to this code so that people don't try to use it elsewhere.

Here's a patch.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Priority: Major » Normal
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +language-ui, +sprint

Superb. Updated the summary too :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Component: field system » documentation
Issue summary: View changes

Adding beta eval and now this is just documentation (wasn't really field system anyway).

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2552867-document-nonstandard-format-plural.patch, failed testing.

Status: Needs work » Needs review

Gábor Hojtsy queued 8: 2552867-document-nonstandard-format-plural.patch for re-testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed db342cb and pushed to 8.0.x. Thanks!

  • alexpott committed db342cb on 8.0.x 
    Issue #2552867 by jhodgdon, Gábor Hojtsy, pjonckiere: Document...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: -sprint

Thanks!

Status: Fixed » Closed (fixed)

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