format_plural() does not handle D7 translations with a plural form after @count

So the new system added in #532512: Plural string storage is broken, editing UI is missing does not require any special number markers on those counts, because the way we import/store them, we implicitly know the index of them already. No need to add the ugly indexes. As for translations from your Drupal 7 site, a conversion may be possible in the migration process, but based on the old vs. new data structure in #532512: Plural string storage is broken, editing UI is missing I'm not sure if a migration path for these strings is even possible. The old data structure did not have connections between the singular/plural strings for us to identify AFAIR. Please correct me if I'm wrong :)

In short, I think this issue itself is a won't fix and we'll need a migration path issue to remove those numbers and somehow figure out how to merge the old strings to the new strings (if we can at all). Or repurpose this issue for the IMP project (https://groups.drupal.org/imp)

So I was working on code to fix this in the import of .po files, but then looked up the .po files and found no indexes. That was added on import (but not anymore in Drupal 8 right?). From http://ftp.drupal.org/files/translations/5.x/drupal/drupal-5.23.ru.po (forgive the bugos chars).

msgid "1 hour"
msgid_plural "@count hours"
msgstr[0] "@count час"
msgstr[1] "@count часа"
msgstr[2] "@count часов"
msgid "1 day"
msgid_plural "@count days"
msgstr[0] "1 день"
msgstr[1] "@count дня"
msgstr[2] "@count дней"

So nothing to fix in core. I attached my patch anyway, but as demonstrated, it is not needed.

The problem still exists in 8.4.x. I've installed Drupal in Russian and visited the admin page.
Downloaded the current ru translation straight from l.d.o

Text from drupal-8.4.0-rc1.ru.po

msgid_plural "@count seconds"
msgstr[0] "1 секунда"
msgstr[1] "@count секунды"
msgstr[2] "@count[2] секунд"

@Sutharsan: ok that looks very strange and does not match my prior experience from #6 but that is because in #6 I looked at Drupal 5 translations. Why would I do that? Duh. Should reopen for Drupal 8 then...

One thing to consider is what to do with export of translations. Do we want these translations to be compatible with D7? When exported configuration gets imported into l.d.o, l10n_server or a Drupal 7 site, the >=2 plurals will fail on D7 strings.
I consider this as a minor use case, and it can be solved later if we want to.

I tested #21 on 2 sites and it works as expected.

Given that users who download the Russian translation are seeing incorrect translations is this not a bug?

Also I think the issue summary could do with an update to explain what is going on - ie. making D8 translation system compatible with D7 translations that have a plural form after a count. I've made a start but someone more knowledgeable about the issue might like to complete it.

+++ b/core/modules/locale/locale.bulk.inc
@@ -639,3 +639,69 @@ function locale_config_batch_finished($success, array $results) {
+ *   concluded.

+++ b/core/modules/locale/locale.install
@@ -303,3 +303,20 @@ function locale_update_8300() {
+function locale_update_8400() {

+++ b/core/modules/locale/tests/src/Functional/Update/OldPluralUpdateTest.php
@@ -0,0 +1,54 @@
+  /**

Normally update functions only go in the next version - so this is going to have to be 8500.

Just wondering is there anyway we can make the system just ignore the ugly indexes - and therefore fix the bug in a way that does not mean having to update all the translations?

@vaplas what I was wondering is if instead of doing manipulation of the strings on reading the PO files we could somehow change the lookup of the translated string to look for @count[2] (for example) if there was a miss. That way we wouldn't need to manipulate data coming from l.d.o and we wouldn't need an update function.

I guess what I'm asking was why aren't doing

Filter the '[index]' when rendering the plural form

As @Sutharsan suggested - what are the downsides of doing that?

@vaplas but can't we remove the [0-9] prior to caching? So there's not really any performance issue once the locale strings have been cached?

@andypost good idea re hook_post_update_NAME() that makes sense here - no need to use hook_update_N() for this - if we go for manipulating the data on input.

@alexpott yeah I guess the performance penalty would be pre-cache; if we are willing to take that, then we can fix this for all sites and all of them will incur the small penalties in the future because we'll not "fix" the data itself -- that penalty is probably small and infrequent though

@vaplas which one should we test?

@vaplas thanks for working on this - I think the new direction makes things really simple for everyone - including existing sites.

+++ b/core/modules/locale/tests/src/Functional/LocaleLocaleLookupTest.php
@@ -55,4 +56,38 @@ public function testLanguageFallbackDefaults() {
+    // Assert that @count[2] was fixed with caching.
+    drupal_flush_all_caches();
+    $this->drupalGet('');
+    $this->assertSession()->pageTextContains('@count old-plural-test');
+
+    // Assert that @count[2] wasn't fixed after caching.
+    drupal_flush_all_caches();
+    $collector = new CacheCollectorHelper('locale:fr::authenticated', $this->container->get('cache.default'), $this->container->get('lock'));
+    $collector->set('Member for', '@count[2] old-plural back');
+    $collector->destruct();
+    $this->drupalGet('');
+    $this->assertSession()->pageTextContains('@count[2] old-plural back');
+

Writing directly to the cache in the test seems really brittle and over testing. We only really need to test that for a given input LocaleLookup behaves as we expect.

+++ b/core/modules/locale/tests/src/Functional/LocaleLocaleLookupTest.php
@@ -4,6 +4,7 @@
+use Drupal\Tests\Core\Cache\CacheCollectorHelper;

This looks unnecessary now.

+1 for the unit test test but it is nice also to have the integration test without the mock so maybe we can bring back the functional test from #42 too but without the futzing with the cache.

+++ b/core/modules/locale/tests/src/Functional/LocaleLocaleLookupTest.php
@@ -55,4 +56,46 @@ public function testLanguageFallbackDefaults() {
+    drupal_flush_all_caches();

Let's just clear the specific cache here (if that is actually even necessary). drupal_flush_all_caches() is super heavy.

+++ b/core/modules/locale/tests/src/Functional/LocaleLocaleLookupTest.php
@@ -55,4 +56,45 @@ public function testLanguageFallbackDefaults() {
+    _locale_refresh_translations(['fr'], [$lid]);

This seems a fine compromise. In an ideal world saving the translation would fire this event and not \Drupal\locale\Form\TranslateEditForm::submitForm() - but it is hardly a new problem that the locale module needs a major API overhaul.

1. +++ b/core/modules/locale/locale.post_update.php
   @@ -0,0 +1,13 @@
   + * Clear cache to ensure translations with old style are clear.
   

   Are clear of what? I think this meant to say "Clear cache to ensure plural translations are removed from it." or somesuch?

2. +++ b/core/modules/locale/locale.post_update.php
   @@ -0,0 +1,13 @@
   +function locale_post_update_clear_cache_for_old_translations() {
   +  // Remove cache of translations, like '@count[2] words'.
   +}
   

   Merely defining a post update function will clear the cache? I looked in the hook docs and found no mention of this.

3. +++ b/core/modules/locale/src/LocaleLookup.php
   @@ -172,6 +173,11 @@ protected function resolveCacheMiss($offset) {
   +    if (is_string($value) && strpos($value, PluralTranslatableMarkup::DELIMITER) !== FALSE) {
   +      // Fix old style plural problem, when value contains @count[number].
   +      $value = preg_replace('!@count\[\d+\]!', '@count', $value);
   +    }
   

   Are these "old style"? As per the issue discussion localize.drupal.org is still distributing translations like that, so in some ways its the current style also? It would be nice to be more specific here. Eg. "Community translations imported from localize.drupal.org as well as migrated translations may contain @count[number]".

4. "Needs issue summary update" still seems to be true.

I've updated the issue summary to only have pertinent information as to the agree fix. The issue comments contain the history.

@Gábor Hojtsy empty post update hooks are the way we can ensure a cache clear happens on update. There's no point clearing a specific cache because if there is a single update function as the end of the update process we call drupal_flush_all_caches().

Thanks all, looks good now! I committed the fix and merged to 8.4.x (on second try ;)

Together make Drupal awesome again and again! \m/
Thanks!