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:
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
- Download the translation, or open localy .po file (example /sites/default/files/translations/drupal-8.4.0.ru.po)
- Replace /@count[2]/@count
- Go to import page /admin/config/regional/translate/import
- 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.
Comment | File | Size | Author |
---|---|---|---|
#57 | interdiff-53-57.txt | 1.19 KB | Anonymous (not verified) |
#57 | 2139467-57.patch | 6.82 KB | Anonymous (not verified) |
Comments
Comment #1
andypostProbably we would need to translate all plural strings again for 8.x so that will require to add special "context" for plurals
Comment #2
klonosComment #3
Gábor HojtsySo 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 :)
Comment #4
Gábor HojtsyIn 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)
Comment #5
andypost@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
Comment #6
Gábor HojtsySo 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).
So nothing to fix in core. I attached my patch anyway, but as demonstrated, it is not needed.
Comment #7
andypost@Gábor Hojtsy please comment on #2833830-11: Translation for third plural form
Comment #8
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedThe 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
Comment #9
Gábor Hojtsy@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...
Comment #10
Gábor HojtsyComment #11
Gábor HojtsyComment #12
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedOne 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.
Comment #13
andypostI 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
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedWoot! Here is the #6 (bit change with regexp) + test. Maybe also hook_update?
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedIMO, 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
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?
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedWow, 28 fails just because I put the code in the batch.inc instead of bulk.inc?
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedAnd here is the test for the update path.
Comment #22
dillix CreditAttribution: dillix commentedI tested #21 on 2 sites and it works as expected.
Comment #23
alexpottGiven 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.
Comment #24
alexpottNormally 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?
Comment #25
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks 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.
Comment #26
alexpott@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
As @Sutharsan suggested - what are the downsides of doing that?
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commented#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.
Comment #28
andypost@alexpott the reason to do that only once instead of additional overhead on each operation
Maybe instead of
hook_update_N()
better to usehook_post_update_NAME()
so the fix could be compatible with 8.4 & 8.5?Comment #29
alexpott@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.
Comment #30
Gábor Hojtsy@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
Comment #31
Anonymous (not verified) CreditAttribution: Anonymous commentedSounds cool, thanks! I will do it + test now.
Comment #32
Anonymous (not verified) CreditAttribution: Anonymous commentedinterdiff-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?
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #35
dillix CreditAttribution: dillix commented@vaplas which one should we test?
Comment #36
Anonymous (not verified) CreditAttribution: Anonymous commented@dillix, i don't know and I'm also waiting for clues))
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).
Comment #38
Anonymous (not verified) CreditAttribution: Anonymous commentedCorrection 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 momentComment #39
Anonymous (not verified) CreditAttribution: Anonymous commentedI forgot to say, that after applying #38 patch we need to clear the cache, otherwise the changes will not take effect.
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous commentedUpdated map of patches in IS.
Comment #41
andypost@vaplas that means you need to add empty
hook_update_N()
to locale moduleComment #42
Anonymous (not verified) CreditAttribution: Anonymous commented@andypost, thanks for hint! I also read that it is preferable to use
post_update
hook instead of emptyhook_update_N()
for more human-readable reasons. So, done through them.Comment #43
Anonymous (not verified) CreditAttribution: Anonymous commented#2921103: Adds [2] to numeric values in Admin menus and pages in Russian version closed like duplicate. @Max_Z. also tested patch #38 and gave a positive feedback.
Comment #44
andypostPatch looks ready, only issue summary needs update to describe chosen solution as set in #23
Comment #45
Anonymous (not verified) CreditAttribution: Anonymous commented@andypost, thank you! I tried to do this.
We can also add more conditions to fix only multiple translations. Example:
If that makes sense, i will update test too.
Comment #46
alexpott@vaplas thanks for working on this - I think the new direction makes things really simple for everyone - including existing sites.
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.
Comment #47
Anonymous (not verified) CreditAttribution: Anonymous commented@alexpott thanks for review and hint! Done + checking that only plural forms are affected, and fix works for translations from candidates language too.
Comment #48
Anonymous (not verified) CreditAttribution: Anonymous commentedDid 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.
Comment #49
alexpottThis 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.
Comment #50
Anonymous (not verified) CreditAttribution: Anonymous commented@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.
Comment #51
alexpottLet's just clear the specific cache here (if that is actually even necessary). drupal_flush_all_caches() is super heavy.
Comment #53
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks again!
Yes, clear cache is necessary, because
translation->save()
does not invalidate cache. We can useCache::invalidateTags(['rendered', 'locale']);
or_locale_refresh_translations
for necessary event.Also small clean up.
Comment #54
alexpottThis 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.
Comment #55
andypostRTBC +1
String translation cache invalidation - I filed follow-up #2927222: Move cache invalidation to event out of TranslateEditForm::submitForm()
Comment #56
Gábor HojtsyAre clear of what? I think this meant to say "Clear cache to ensure plural translations are removed from it." or somesuch?
Merely defining a post update function will clear the cache? I looked in the hook docs and found no mention of this.
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]".
Comment #57
Anonymous (not verified) CreditAttribution: Anonymous commented#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.
Comment #58
alexpottI'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().
Comment #59
Anonymous (not verified) CreditAttribution: Anonymous commented#58: looks nice! The double section
Current proposed resolution/Proposed resolution
was really a bit confusing. Other updates also have sense! RTBC +1.Comment #64
Gábor HojtsyThanks all, looks good now! I committed the fix and merged to 8.4.x (on second try ;)
Comment #65
Anonymous (not verified) CreditAttribution: Anonymous commented🎉🎉🎉
@Gábor Hojtsy, thanks so much!
Also many thanks to all who helped to overcome this problem! Drupal 8.4.3 rocks! 🚀
Comment #66
VVS CreditAttribution: VVS commentedTogether make Drupal awesome again and again! \m/
Thanks!
Comment #67
Anonymous (not verified) CreditAttribution: Anonymous commented@VVS, absolutely! Thank you for #2833830: Translation for third plural form and positive feedback here!