Deprecate locale_get_plural

#2766857: Provide a PluralTranslatableMarkup::getPluralVariantIndex() factory method is related.

Here's patch.

Small improvement.

Added the missed service in unit tests container.

Thanks for the patch. I have reviewed it and don't see any obvious issues though there are some comments that should be changed slightly.

1. +++ b/core/core.api.php
   @@ -2236,6 +2236,61 @@ function hook_validation_constraint_alter(array &$definitions) {
   + * implementation should return a valid zero-based positive integer or nothing,
   

   I had not heard "zero-based positive integer" used before. I googled it and it does appear to be used but it might be better to change to something like:

   ...implementation should return zero, a positive integer, or...

2. +++ b/core/core.api.php
   @@ -2236,6 +2236,61 @@ function hook_validation_constraint_alter(array &$definitions) {
   + * if he wants to pass the decision to core or other modules. Returning anything
   

   The "he" should be changed to be gender neutral. In other places "you" is used. Or, the sentence could be restructured something like:

   The implementation should return zero, a positive integer, or, to not impact the results, nothing.

3. +++ b/core/core.api.php
   @@ -2236,6 +2236,61 @@ function hook_validation_constraint_alter(array &$definitions) {
   + * can provide your own implementation, in your custom module, when you don't
   

   Typo:

   local_get_plural => locale_get_plural

4. +++ b/core/core.api.php
   @@ -2236,6 +2236,61 @@ function hook_validation_constraint_alter(array &$definitions) {
   + *   example the Local module implementation locale_plural_variant_index() is
   

   Typo:

   Local => Locale

I'll test the patch shortly.

I've tested this as follows:

1. Enabled Language and Interface Translation
2. Added Romanian
3. Found some text that uses plurals: Module %name has been enabled.
4. Changed the plurals for these for Romanian
5. Enabled some modules (1 module, 2 modules, and 3 modules)
6. The messages were as expected (see screenshots below) (though I'm not sure what the 2. plural is for in Romanian)

Assigning to myself to make the comment changes. I will try to finish this today.

Still plan on working on this but the day got away from me. Crossing fingers for tomorrow. If I don't finish by Friday EOD, then I'll take my name off.

I reworded some of the comments. Here's the patch and interdiff.

@Kristen Pol, thank you for your work and review. Sorry for my poor English. I think the patch looks good. I'm not very sure yet about the solution: if invoking a hook is the way to go. Also I don't find it bad. Probably in 99.99% of the cases nobody will implement such a hook and we'll live with the only implementation which is locale_plural_variant_index(). Normally we should offer also an "alter hook" but for me that sounds a little bit over-engineering.

Nit:

+++ b/core/core.api.php
@@ -2247,6 +2247,63 @@ function hook_validation_constraint_alter(array &$definitions) {
+ * Compute the plural variant index.

Compute > Computes

This might be related to #2273889: Don't use one language's plural index formula with another language's string in the case of untranslated strings using format_plural()., in the sense that computing the plural index requires more information than just the target language -- we also need to know the string itself: if no translation exists for a string, we should not be using the target language plural index, but rather -1, it seems. Perhaps the string itself could be passed to hook_plural_variant_index, so that the locale_plural_variant_index could then check if the string is, in fact, translated; if it is not then the plural variant index would always be -1.

We can also deprecate locale_get_plural() in the scope of this issue as in #23, the procedural function became only a wrapper of the service method call.

Also this approach help us also to remove usages of drupal_static() and drupal_static_reset() from locale_get_plural() and this makes this part of the #1577902: [META] Remove all usages of drupal_static() & drupal_static_reset() effort.

Reroll, looks tests do a lot of mocks for services so instead of strings translatable object returns

Debugging test \Drupal\Tests\Core\Datetime\DateTest::testFormatInterval() shows that count passed wrong somehow

8) Drupal\Tests\Core\Datetime\DateTest::testFormatInterval with data set #15 (7344000, 3, '2 months 3 weeks 4 days')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'2 months 3 weeks 4 days'
+'1 month 1 week 1 day'

Should fix some tests

Decorated service should implement all public methods

reroll

Reroll patch for 9.1.x.

Fixing the error in patch #38.

Thanks for the patch. I tried testing it twice (two different installs) but keep getting an error after adding the language:

LogicException: The database connection is not serializable. This probably means you are serializing an object that has an indirect reference to the database connection. Adjust your code so that is not necessary. Alternatively, look at DependencySerializationTrait as a temporary solution. in Drupal\Core\Database\Connection->__sleep() (line 1725 of /var/lib/tugboat/stm/web/core/lib/Drupal/Core/Database/Connection.php).

When the patch is updated, here are a few nitpicks that could be addressed:

1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
   @@ -109,4 +109,22 @@ public function translateString(TranslatableMarkup $translated_string);
   +   *   Optional language code to translate to a language other than
   +   *   what is used to display the page.
   

   Nitpick: Character wrapping (move some words up).

2. +++ b/core/modules/locale/locale.services.yml
   @@ -37,3 +37,9 @@ services:
   +    # pass the old service as an argument
   

   Nitpick: Comment should be a sentence with starting capital and ending period.

3. +++ b/core/modules/locale/src/LocaleTranslationManager.php
   @@ -0,0 +1,142 @@
   +      // If there is a plural formula for the language, evaluate it for the given
   

   Nitpick: More than 80 chars.

4. +++ b/core/modules/locale/src/LocaleTranslationManager.php
   @@ -0,0 +1,142 @@
   +
   +
   +  /**
   

   Nitpick: Extra empty line.

Updated patch as per the changes suggested in #40, please review.

Thanks for the update. #44 fixes the nitpicks noted in #40.

Tagging for manual testing. I had issues but maybe someone else can try. If you want test steps, see #11.

We have followed the steps given in #11 and after adding the patch mentioned #44 (https://www.drupal.org/pift-ci-job/1751390). While enabling singular/plural modules , its showing 'Site encountered error'. When we refresh the modules page then it should that the module is enable. Please refer the attached screenshots below.

Ok, since @tanubansal also had an issue, moving this back to needs work.

Tests failing because of

Exception: TypeError: Argument 8 passed to Drupal\help_topics\Plugin\HelpSection\HelpTopicSection::__construct() must be an instance of Drupal\Core\StringTranslation\TranslationManager, instance of Drupal\locale\LocaleTranslationManager given, called in /var/www/html/core/modules/help_topics/src/Plugin/HelpSection/HelpTopicSection.php on line 128
Drupal\help_topics\Plugin\HelpSection\HelpTopicSection->__construct()() (Line: 107)

Attempt to fix #50

Patch fails to apply to 9.2.x setting to NW for that.

The Issue summary needs to have a proposed resolution, adding tag.

Added reroll of patch #51 on Drupal 9.2.x.

Fixed custom command fails.

New method needs CR also it could help to deprecate locale code a bit

Fixing test cases.
All the test cases failing in #56 are passing on local now.

InstallerExistingConfigTest test is something that is passing without any change. So not sure if past failure was due to other reasons.

Pulling out the failure info:

There was 1 failure:
1) Drupal\Tests\locale\Functional\LocalePluralFormatTest::testGetPluralFormat
Plural translation of 1 hours / @count hours for count 0 in fr is 0 heure
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'0 heure'
+'0 heures'

Related code:

./core/modules/locale/tests/src/Functional/LocalePluralFormatTest.php:        $this->assertSame($expected_plural_string, \Drupal::translation()->formatPlural($count, '1 hour', '@count hours', [], ['langcode' => $langcode])->render(), 'Plural translation of 1 hours / @count hours for count ' . $count . ' in ' . $langcode . ' is ' . $expected_plural_string);

Added reroll of patch #59 on Drupal 9.5.x.

Lets see what will fail now

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

For the IS update and change record

Did not test or review yet.

#3577671: Modernize locale file handling
#3037031: Convert locale.compare.inc to a service
#3037156: Modernize locale history functions

I think we combine a few of the remaining functions in locale.module as part of the .module elimination initiative.

There are still 8 issues remaining even if we combine these few.

• locale_get_plural
• locale_system_set_config_langcodes
• locale_system_update
• locale_system_remove

Should have done the file audit first, locale_get_plural is complex enough, let's just do that one here.

Removing postponed status, I think this is fairly conflict-free with other issues (maybe except services.yml, but I've moved the change up and that's easy to rebase) and can be worked on. Created an MR with that. At some point changes to TranslateEditForm snuck in, those seem unrelated and I ignored them.

I think I want to explore adding this method to a new interface + service in core, that should be much easier to overwrite. I'm not found of decorators generally and this also comes with changes to a pretty fundamental interface.

Working on a new service. That requires updates to a bunch of unit tests, but seems to work well and feels much cleaner.

\Drupal\Core\StringTranslation\StringTranslationTrait::getNumberOfPlurals() is somewhat related to this. That also wraps a locale service. I only identified 3 calls to that. one in locale, one in config_translation and one in views. We could consider to just deprecate that, only the views call doesn't depend on locale anyway. Alternatively we could expand the same of the new interface and add it to that as well.

I'm even wondering if we could somehow reuse the existing \Drupal\locale\PluralFormula service implement the new interface instead of a new one that calls out to it. Would nee to alias and deprecate the current service. This is already complex enough I think.

FWIW, the static/memory cache here feels overkill. what exactly is this even caching? a php calculation?

I think this is ready for review.

Took a quick look and had some minor questions, I'll have a clear look in a bit.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

This is where formula caching been added so not sure it's a good idea to remove #1273968: remove eval from locale.module

That's not true, locale_get_plural() was already using drupal_static() before that, those lines weren't touched in that commit. and it had been using static caching even before drupal_static(), it had been using the static keyword since that function was added in a commit in 2004 by Dries before there were issues and issue numbers.

Raw PHP execution performance changed *a tiny bit* since 2004, it doesn't look like anyone every questioned whether static caching here is worth it or not. Might not even have been back in 2004.

I did a script that called the function 100k times, it barely registered with microtime() and it made no difference whether or not the static caching is there. I verified how often it's called on the frontpage of umami with disabled render caches: zero times, on the article add form, once.

Rebased and removed it.

Ok I did a bunch of digging, had a couple of discussions in slack and read a bunch of references.
I think we should find a way to document this here in the comments for future people.

Some of this may be obvious to other people on this thread, but I want to type it out to help ensure I understand it properly.

Different languages have different formulas for plurals: https://docs.translatehouse.org/projects/localization-guide/en/latest/l1...

English for example: nplurals=2; plural=(n != 1);
There are two plural forms and the rule for plurals is if there is 1 say the singular, if there are more or less than 1 then use the plural.
E.g 1 horse, 2 horses, 50 horses, 0 horses, -10 horses.

Other languages have other rules: https://www.gnu.org/software/gettext/manual/html_node/Plural-forms.html
Polish: nplurals=3; plural=(n==1 ? 0 : n%10>=2 && n%10<=4 && (n%100<10 || n%100>=20) ? 1 : 2);

1 plik
2,3,4 pliki
5-21 pliko'w
22-24 pliki
25-31 pliko'w

There is some documentation on d.o here: https://www.drupal.org/community/contributor-guide/reference-information...

My understanding is that the formula will be at the top of the .po files.
We then precalculate the plural formula in a compressed way shown here: https://www.drupal.org/project/drupal/issues/1273968#comment-7559379

A translation will look like this:

#: modules/user/views_handler_filter_user_name.inc:112
msgid "Unable to find user: @users"
msgid_plural "Unable to find users: @users"
msgstr[0] "Benutzer konnte nicht gefunden werden: @users"
msgstr[1] "Benutzer konnten nicht gefunden werden: @users"

The 0 or 1 is the plural index.

I think we can fill out and link to this reference on PluralFormula.
And maybe on LocalePluralIndex