String version of plural formula is not available, exported .po files contain an incorrect default

I think we could store the plural formula in core for later use like this even if core does not actively use it. That would not solve existing sites/imports, but when a localization update happens, that should fix/update the plural info. (I think we should make sure that happens).

I agree that storing the formula during the import is the most sensible to do. Forcing an import is one way to get the formula string stored. But alternatively and temporary Potx module can store the formula directly from user entered form input. But perhaps a (readme) instruction is the simplest to implement.

A first stab at the code. Not sure about whether to add the formula string as a parameter to the existing ::setPluralFormula() or use a new ::setPluralFormulaString().

Needs tests

I think the two new methods need to be on a different interface to keep backwards compatibility proper. Otherwise anybody implementing the interface will get a white screen due to the missing two methods when updating. We can implement the new interface as well in our own class though, so its the same from there.

Created an new interface \Drupal\locale\PluralFormulaStringInterface that defines the new methods.
Class PluralFormula additionally implements the new interface.

Continuing with test code...

The above code stores the formula string it in a state variable, like the formulae array is currently stored in. I think it is better store it in the language (config) entity.

Further I discovered that the header of an exported translation file does not contain the actual formula, but the default ("nplurals=2; plural=(n > 1);") that is set in \Drupal\Component\Gettext\PoHeader::__construct(). This should go into a separate issue.

Using the Language entity to store the plural_forms
- Requires a change of language.schema.yml (language.entity.*)
- Non-standard plural_forms values can be defined in \Drupal\Core\Language\LanguageManager::getStandardLanguageList / ::getUnitedNationsLanguageList()
- Adding ConfigurableLanguage::getPluralFormulaString requires an extra interface

I can not oversee the backwards compatibility consequences of the schema change.

Let's first decide where to store the plural formula string value.

I would store the string formula at the same place as the rest of the data about plurals for the sake of encapsulation.

IIRR initially there was intent for separation of locales and languages

From BC POV I'd better added kinda "locale" entity with relation to language and next minors changes API to use it
But this requires split this issue

+++ b/core/modules/locale/src/PluralFormula.php
@@ -90,6 +109,14 @@ public function getFormula($langcode) {
+  public function getFormulaString($langcode) {

maybe __toString() better?

So we agree that the formula string and the formula array should be stored together and separated from the Language entity.

maybe __toString() better?

That will only work if the plural formula is an object. But @anypost, that was your point. Architectural wise, it makes sense. But it would require a fair amount of reworking above and beyond this issue. I agree that this would deserve a separate issue.

Added unit test.

Added a small patch as proof that patch #15 works on exporting PO files.

Functional test:

1. Existing site with Polish as language, without patch #15 : "Plural-Forms: nplurals=2; plural=(n > 1);\n" "
2. Same setup, with patch #15 and attached patch : "Plural-Forms: \n"
3. After language updates : "Plural-Forms: nplurals=3; plural=((n==1)?(0):(((((n%10)>=2)&&((n%10)<=4))&&(((n%100)<10)||((n%100)>=20)))?(1):2));\n"

Should there be a functional test in core/modules/locale/tests/src/Functional/LocaleImportFunctionalTest.php to check if an empty $formulaStrings[$langcode] isn't empty after an import?

In this issue a test should be added that PoDatabaseWriter::setHeader() sets the plural formula string.
Back to needs work to add this test.

Using the formula string in the Gettext component is a different issue and its tests should be added in that issue. Note that the Gettext component should not depend of a Drupal core service. But that is a different issue altogether.

Don't forget to check if the nplurals is valid or you'll get a nasty bug in the User interface translation UI.

Added the requested test with plurals for Arabic.

I'm not superhappy with the approach, as we are keeping the responsibility to the caller for keeping them consistent (e.g. I could call setPluralFormulaString without calling setPluralFormula).
But I don't think we can do this in a BC way without changing the interface per #7

Tagging for tracking Global2020 related issues.

Fixed errors and coding standards.

Code is all clean and readable, test coverage looks good, and tests are passing.

I see no outstanding issues after reading the IS and comments, so I'm going to say this passes community review.

I noticed this needs a reroll and I don't see a fail patch. Adding both now.

Attached is the reroll and a diff of that. I made one additional change and that was to a comment. Changing 'de' to 'Arabic'.

Also, I made a fail patch which only contains the test files. I didn't make an interdiff between the patch and the fail patch.

back to rtbc

+++ b/core/modules/locale/src/PluralFormula.php
@@ -40,6 +40,13 @@ class PluralFormula implements PluralFormulaInterface {
+  /**
+   * The string representation of the plural formula, keyed by langcode.
+   *
+   * @var string[]
+   */
+  protected $formulaStrings;

+++ b/core/modules/locale/src/PluralFormulaStringInterface.php
@@ -0,0 +1,34 @@
+  /**
+   * Gets the plural formula string for a langcode.
+   *
+   * @param string $langcode
+   *   The language code to get the formula for.
+   *
+   * @return string
+   *   Plural formula.
+   */
+  public function getFormulaString($langcode);

+++ b/core/modules/locale/src/PoDatabaseWriter.php
@@ -178,7 +178,9 @@ public function setHeader(PoHeader $header) {
-        \Drupal::service('locale.plural.formula')->setPluralFormula($langcode, $nplurals, $formula);
+        \Drupal::service('locale.plural.formula')
+          ->setPluralFormula($langcode, $nplurals, $formula)
+          ->setPluralFormulaString($langcode, $plural);

I don't get why we need to store the plural forms as a string. We're already reading it and storing it as an array after calling parsePluralForms.

I think storing both the string and array is a recipe for things getting out of sync. If need to be able to retrieve the plural form as a string again after loading can't we convert the array back to a string in getFormulaString()?

Or if we can't convert the array form back into a string form can we change setPluralFormula() to...

  /**
   * {@inheritdoc}
   */
  public function setPluralFormula($langcode, $plural_count, array $formula, string $string_formula = '') {
    // @todo add deprecation if called with only 3 args.
    // Ensure that the formulae are loaded.
    $this->loadFormulae();

    $this->formulae[$langcode] = [
      'plurals' => $plural_count,
      'formula' => $formula,
      'string_formula' => $string_formula,
    ];
    $this->state->set('locale.translation.formulae', $this->formulae);
    return $this;
  }

This way the string and array representations are stored together.

The array is optimised for lookup. Not sure if we can optimise it back for the string format again without it being too unwieldy honestly. It would be great if someone comes up with a magic solution to that.

IIRC there was issue to use compiled/executable formula as performance improvement (maybe chx could recall) but I can't find it.
The idea was to speed-up evaluation of formula but then it was considered having not viable effect.

So maybe compromise here is to save both?

Looking at http://docs.translatehouse.org/projects/localization-guide/en/latest/l10... there are only a finite and a very small finite number of possible plural strings, run each of them and compare to the array, you will get a match quick. Especially so if you pick them by npurals.

++ I like the idea of having plural formulae (string representation) like there's CountryManager

OTOH it could be part of standard language list shipped in core or even in core/lib/Drupal/Component/Gettext

I think the idea from #42 to take the finite (short) list of plural formulas and compare the arrays to what comes out of those is a good one. That would even allow potx to implement this without a core change. However, even though the issue summary does not mention core impact, core also supports exporting a .po file with translations in it? How does it do that and how is this not a problem there?

@Gábor Hojtsy it looks like core currently provides a default - $this->pluralForms = 'nplurals=2; plural=(n > 1);'; - and allows you to set from an existing file - see \Drupal\Component\Gettext\PoStreamReader::readHeader() - but if you export a new file it looks like it is up to you to fix it after the fact. It looks like you've identified a bug.

Picking this up today as part of helping with porting localize.drupal.org to Drupal 9.

@alexpott, I would return to this:

I don't get why we need to store the plural forms as a string. We're already reading it and storing it as an array after calling parsePluralForms.

I think storing both the string and array is a recipe for things getting out of sync. If need to be able to retrieve the plural form as a string again after loading can't we convert the array back to a string in getFormulaString()?

Rethinking this, I think the suggestiont to include a mapping table of plural formats to arrays in core so we can look up the right string for the array we store is interesting. However it locks us into a finite set up plural forms and lock this part of the API in. While there may seem to be a finite set of plural forms used, it would be great to have a way to keep this open instead of making it locked in. I know its currently even more locked in with this bug, but yeah.

As for the .po file export not using the right formula, that was supposed to be #2882667: [PP-1] Interface translations export contains incorrect plural formula which was closed down in favour of #2918489: Plurals are not exported correctly when exporting source translations which only deals with the number of plural variants exported now, not the formula. I think it would be more within the scope of this issue to resolve the export of the formula as well, after all that is the ONLY place where this bug shows in core, otherwise itsjust a nice to have issue.

Retitling based on the problem rather than the solution.

Recap from DrupalCon Lille 23:

• The original String-Version of the Formular should be saved in State, not in Config.
• Configuration is not aware of Plural.
• State becomes the primary source of truth.
• This dosn't disrupt common translation-deploy-flows, which currently can only use the import of PO-Files.
• We need to write an update hook, which guesses the Plural-Formular from the current Plural-Array.
• We can generate and and then compare arrays for these plural-forms. This should lead to a correct guess in most of the cases.
• If we don't find a correct guess (for example for a custom or broken language), we need a reasonable default. This default may change depending on the number of plurals in the array.

Reroll the latest patch:
I reroll the latest patch. A merge conflict (changes in an adjacent line) has been solved.