Problem/Motivation

Multi-plural translations in Drupal 7 looks like '@count[2] ...', '@count[3] ...', etc. In Drupal 8, these [number] postfix displays as is, and it's ugly. But separate translations for D7 and D8 is not good for synchronization support.

The problem comes from a different way of handling plural translations in Drupal 8 compared to previous versions. Up to Drupal 7 plurals above the first plural (the second, third, etc.) translate '@count' as '@count[2]', '@count[3]', etc.
Example:

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

As result you see on ugly [2] postfix in Drupal 8:
plural ru
plural ru 2

Because in Drupal 8 '@count' does not need this special treatment and just translates to '@count' in any of the plural forms. But in localize.drupal.org (l10n_server) translations are shared across Drupal versions and therefore in both Drupal 7 and Drupal 8 translations the '@count[index]' format is used.

Example:
Russian localization (3 plural forms) for d7 have many strings for @count[2] and other strings with third plural forms.
For russian language: "Plural-Forms: nplurals=3; plural=((((n%10)==1)&&((n%100)!=11))?(0):(((((n%10)>=2)&&((n%10)<=4))&&(((n%100)<10)||((n%100)>=20)))?(1):2));\n"

At the moment the problem is present in different locations where plural>2, like:

Proposed resolution

Remove unnecessary postfixes from the translations before caching. This will fix the problem without requiring changes to localize.drupal.org or an upgrade path to fix the data - only need a cache clear. See from #23.

Remaining tasks

Workaround

  1. Download the translation, or open localy .po file (example /sites/default/files/translations/drupal-8.4.0.ru.po)
  2. Replace /@count[2]/@count
  3. Go to import page /admin/config/regional/translate/import
  4. Upload the corrected file with flag "Overwrite non-customized translations".

or fix all places by manual.

User interface changes

None

API changes

None

Data model changes

The cached translation strings don't have the postfixes.

CommentFileSizeAuthor
#57 interdiff-53-57.txt1.19 KBvaplas
#57 2139467-57.patch6.82 KBvaplas
#53 interdiff-50-53.txt1.79 KBvaplas
#53 2139467-53.patch6.76 KBvaplas
#50 interdiff-test-42-50.txt2.72 KBvaplas
#50 interdiff-47-50.txt2.17 KBvaplas
#50 2139467-50.patch6.67 KBvaplas
#50 2139467-50-test-only.patch5.17 KBvaplas
#47 interdiff-42-47.txt5.77 KBvaplas
#47 2139467-47.patch5.08 KBvaplas
#42 interdiff-38-42.txt511 bytesvaplas
#42 2139467-42.patch3.19 KBvaplas
#38 interdiff-36-38.txt590 bytesvaplas
#38 2139467-38.patch2.69 KBvaplas
#36 2139467-36.patch2.61 KBvaplas
#36 2139467-36-test-only.patch2.03 KBvaplas
#32 interdiff-21-32.txt4.15 KBvaplas
#32 2139467-32-with-update-path.patch10.33 KBvaplas
#32 2139467-32.patch5.04 KBvaplas
#32 2139467-32-test-only.patch3.77 KBvaplas
#21 interdiff-20-21.txt2.44 KBvaplas
#21 2139467-21.patch7.71 KBvaplas
#20 interdiff-17-20.txt6.71 KBvaplas
#20 2139467-20.patch6.2 KBvaplas
#17 interdiff-16-17.txt1.87 KBvaplas
#17 2139467-17.patch5.87 KBvaplas
#16 interdiff-14-16.txt5.59 KBvaplas
#16 2139467-16.patch6.06 KBvaplas
#14 2139467-14.patch2.43 KBvaplas
#14 2139467-14-test-only.patch1.74 KBvaplas
#8 2139467-8-admin-page-in-ru.png18.95 KBSutharsan
#6 count-replace.patch705 bytesGábor Hojtsy
#1 plural.png16 KBandypost
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

andypost’s picture

Category: Bug report » Task
Priority: Normal » Major
Issue summary: View changes
FileSize
16 KB

Probably we would need to translate all plural strings again for 8.x so that will require to add special "context" for plurals

klonos’s picture

Gábor Hojtsy’s picture

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 :)

Gábor Hojtsy’s picture

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)

andypost’s picture

@Gabor the main purpose of the issue to find a way to serve this strings for LDO! Probably we need separate issue for localization server that will make D7 and D8 strings separate because D7 translations already have this indexes

Parent issue already converts most of plurals so there's no problem, just need to add removal of the index

Gábor Hojtsy’s picture

Status: Active » Closed (won't fix)
FileSize
705 bytes

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.

andypost’s picture

@Gábor Hojtsy please comment on #2833830-11: Translation for third plural form

Sutharsan’s picture

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

admin page in ru

Text from drupal-8.4.0-rc1.ru.po

msgid_plural "@count seconds"
msgstr[0] "1 секунда"
msgstr[1] "@count секунды"
msgstr[2] "@count[2] секунд"
Gábor Hojtsy’s picture

Status: Closed (won't fix) » Needs work

@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...

Gábor Hojtsy’s picture

Version: 8.0.x-dev » 8.4.x-dev
Gábor Hojtsy’s picture

Issue tags: -language-base +language-ui
Sutharsan’s picture

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.

andypost’s picture

Issue tags: +Needs tests

I think Gabor's idea to convert data on "read" is best choice
- it made only once
- it makes d8 translation better
- it does not affect backward compatibility

@Sutharsan Export from each system (before and after d8) should not affected to keep BC

SO only tests needed here and consensus

vaplas’s picture

Status: Needs work » Needs review
FileSize
1.74 KB
2.43 KB

Woot! Here is the #6 (bit change with regexp) + test. Maybe also hook_update?

The last submitted patch, 14: 2139467-14-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

vaplas’s picture

IMO, it will be great if this problem disappears after the update. At first I was thinking about a simple action, like:
$database->query("UPDATE {locales_target} SET translation = replace(translation, '@count[2]', '@count' )", [], []);

or improved version

$languages = \Drupal::service('language_manager')->getLanguages();
$max_plural = 2;
foreach ($languages as $language) {
  $plural = \Drupal::service('locale.plural.formula')->getNumberOfPlurals($language->getId());
  $max_plural = max($max_plural, $plural);
}
for ($number = 2; $number < $max_plural; $number++) {
  $database->("UPDATE {locales_target} SET translation = replace(translation, '@count[$number]', '@count[$number]' )", [], []);
}

But theoretically, the site can have many languages, modules, and custom translations with old plural syntax. So maybe it makes sense to use batch.

Do we need an UpdatePathTest?

vaplas’s picture

The last submitted patch, 16: 2139467-16.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 17: 2139467-17.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

vaplas’s picture

Status: Needs work » Needs review
FileSize
6.2 KB
6.71 KB

Wow, 28 fails just because I put the code in the batch.inc instead of bulk.inc?

vaplas’s picture

And here is the test for the update path.

dillix’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Title: format_plural() does not add plural form after @count » format_plural() does not handle D7 translations with a plural form after @count
Category: Task » Bug report
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

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.

alexpott’s picture

+++ 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’s picture

Issue summary: View changes

Thanks for review, @alexpott!

I tried to update IS based on #2833830: Translation for third plural form (@VVS) + #13 (@andypost) + #8 (@Sutharsan) + #2833830-11 (@Sutharsan) + #2833830-4 (@vaplas).

If update function is reason for postponed to 8.5.x, let's remove it from this patch to separate issue (or contrib module). Because if we can fix the old-plural style now, all new sites will not suffer this, which is already pretty cool.

alexpott’s picture

@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’s picture

Issue summary: View changes

#22: thanks for testing @dillix!

#26: This idea super works for me. But most of my works use Russian language, so I can not be unbiased in this regard. Because in fact, the percentage of sites that this problem affects a very small. And percentage of strings with @count also very small (near 60/8563). But an additional check on the rendering will affect the performance of all sites and all strings.

andypost’s picture

@alexpott the reason to do that only once instead of additional overhead on each operation

Maybe instead of hook_update_N() better to use hook_post_update_NAME() so the fix could be compatible with 8.4 & 8.5?

alexpott’s picture

@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.

Gábor Hojtsy’s picture

@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’s picture

Sounds cool, thanks! I will do it + test now.

vaplas’s picture

interdiff-21-32.txt is interdiff between #21 and 2139467-32-with-update-path.patch.
Also path update (and test for it) separate out of the main patch. Perhaps we can also fix this issue without correction via import?

vaplas’s picture

Status: Needs work » Needs review

The last submitted patch, 32: 2139467-32-test-only.patch, failed testing. View results

dillix’s picture

@vaplas which one should we test?

vaplas’s picture

@dillix, i don't know and I'm also waiting for clues))

  • 2139467-32.patch contains fix for import and render phase.
  • 2139467-32-with-update-path.patch contains additional fix for update phase.

Now i'm attached patch, that fixs strings only on render phase (2139467-32.patch without import). It does not change any data, which is a serious plus. Perhaps that it will be commited (at least as part 1).

The last submitted patch, 36: 2139467-36-test-only.patch, failed testing. View results

vaplas’s picture

Correction of a logical flaw. Because when $value has no translation in the required language, it can be replaced from another language (which can contains the old style too). For some reason, I overlooked this moment

vaplas’s picture

I forgot to say, that after applying #38 patch we need to clear the cache, otherwise the changes will not take effect.

vaplas’s picture

Issue summary: View changes

Updated map of patches in IS.

andypost’s picture

vaplas’s picture

@andypost, thanks for hint! I also read that it is preferable to use post_update hook instead of empty hook_update_N() for more human-readable reasons. So, done through them.

vaplas’s picture

andypost’s picture

Assigned: andypost » Unassigned
Status: Needs review » Needs work

Patch looks ready, only issue summary needs update to describe chosen solution as set in #23

vaplas’s picture

Issue summary: View changes
Status: Needs work » Needs review

@andypost, thank you! I tried to do this.

We can also add more conditions to fix only multiple translations. Example:

- if (is_string($value)) {
+ if (is_string($value) && strpos($value, PluralTranslatableMarkup::DELIMITER) !== FALSE) {

If that makes sense, i will update test too.

alexpott’s picture

@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.

vaplas’s picture

@alexpott thanks for review and hint! Done + checking that only plural forms are affected, and fix works for translations from candidates language too.

vaplas’s picture

Did I do something wrong in the last patch? Since no data actually changes, can we fix this for 8.4.x too?

Is it possible to add credit to all participants of this topic (since each greatly helped in the reviews, testing and suggestions) +

Similarly, how the #2791405: When installing a site in a language besides English, the site name is not saved and reverts to "Drupal" has a negative impact on the first Drupal look, the problem with plural[2] has a big negative impact too. But removing it manually is much harder than site name, and many users are just afraid of something to change.

alexpott’s picture

+++ 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.

vaplas’s picture

@alexpott, thanks for the good directions as always!

In #47 an additional check has been added, and now processing only translations with plural delimiter sign. So, I bit changed the #42 test for check both cases.

alexpott’s picture

+++ 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.

The last submitted patch, 50: 2139467-50-test-only.patch, failed testing. View results

vaplas’s picture

Thanks again!

Yes, clear cache is necessary, because translation->save() does not invalidate cache. We can use Cache::invalidateTags(['rendered', 'locale']); or _locale_refresh_translations for necessary event.

Also small clean up.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
+++ 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.

andypost’s picture

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
  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.
vaplas’s picture

Status: Needs work » Needs review
FileSize
6.82 KB
1.19 KB

#56: @Gábor Hojtsy, thanks for review!

56.1: Done;

56.2: Proof. This approach is also widely used in other *.post_update.php files, like system.post_update.php. But an interesting fact, without any updates, we also call clear cache (proof). But maybe exist a situation, where we have other updates and the cache is not clear. Therefore, for reliability, hook locale_post_update not removed from patch.

56.3: Done. Fair point, of course. 'Old style' my bad, i did not want to put it non-tactically and meant "previous style" :) Just c/p from
$this->assertEqual('%1 !1', (string) $output, "Ensure that old style placeholders aren't replaced");
where this phrase is obviously more suitable.

56.4: I tried to do it in #25 (edit: + #45). Unfortunately, current IS is my maximum. If anyone knows what needs to be done yet, or wants to hint - it will be great.

alexpott’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

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().

vaplas’s picture

#58: looks nice! The double section Current proposed resolution/Proposed resolution was really a bit confusing. Other updates also have sense! RTBC +1.

  • Gábor Hojtsy committed ac7a34d on 8.4.x
    Issue #2139467 by vaplas, Gábor Hojtsy, andypost, Sutharsan, alexpott,...

  • Gábor Hojtsy committed d7af82a on 8.4.x
    Revert "Issue #2139467 by vaplas, Gábor Hojtsy, andypost, Sutharsan,...

  • Gábor Hojtsy committed 3f49bda on 8.5.x
    Issue #2139467 by vaplas, Gábor Hojtsy, andypost, Sutharsan, alexpott,...

  • Gábor Hojtsy committed 07a71f5 on 8.4.x
    Issue #2139467 by vaplas, Gábor Hojtsy, andypost, Sutharsan, alexpott,...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

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

vaplas’s picture

🎉🎉🎉

@Gábor Hojtsy, thanks so much!

Also many thanks to all who helped to overcome this problem! Drupal 8.4.3 rocks! 🚀

VVS’s picture

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

vaplas’s picture

@VVS, absolutely! Thank you for #2833830: Translation for third plural form and positive feedback here!

Status: Fixed » Closed (fixed)

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