[Translation] translation:push command diff using intl-icu #47986
Conversation
|
About the code, it seems fair, but can you add or modify tests to cover the bug you fix? Thank you |
|
@welcoMattic the 6.0 branch is a maintained one (as its support has been extended by 6 months due to the bump of the min PHP version in 6.1): https://symfony.com/releases However, if the 6.0 branch is affected, the 5.4 branch will also be affected, and it should indeed be the target of the fix. |
|
@stof you are right, I forgot about this exception, sorry. |
|
I'm trying to fix tests. But sorry, I don't understand how the push/pull should work.
I don't understand if we shoud use a domain like "messages+intl-icu" or a "messages" domain with the |
|
Hello, we are also having this bug in Symfony 5.4, pushing to Loco overwrite every translations due to bug in diff with intl-icu. @Broutard for the pull command i've done some test to understand how it works, and i would say "a "messages" domain with the --intl-icu option " Because, the
(here for exemple my config)
and then if we come back on our so when u'll be at the line 159
So if u don't put a name in
you probably put in your config domain name as |
|
@Broutard After some researches on tests fail, the fix seems to be the problem.
Doing this fix the push command well because Catalogue has same array key with domain name but this function is also use for pull command creating a new bug.
As you can see above we'll generate domain with +intl-icu and put into $intlMessages the translations catalogue if it found datas but with $domain into second if does the diff between all key for domain (intl-icu or not) with $intlMessages who's keeping intl-icu keys in order to divide in two files if u've a domain So with this we create only one
We've to take problem from another angle, LocoProvider havent information about intl-icu domain so we cant add into catalogue translations For me only way is to modify as you does TranslationTrait, used by pull and push command. We need to divide pull and push call.
|
|
@a-vallet indeed, sorry for the inconvenience. I will try to find another way to fix it. |
|
Fixed, by merging intl-icu domains only for the push command. |
|
Hi everyone, If it can helps you, I had many issues with pushing ICU and non-ICU messages from the same domains, this is how I fixed it, as a one-shot, for Crowdin (in symfony 5.4): --- vendor/symfony/crowdin-translation-provider/CrowdinProvider.php.old 2022-11-03 22:15:27.000000000 +0100
+++ vendor/symfony/crowdin-translation-provider/CrowdinProvider.php 2022-11-04 11:47:08.000000000 +0100
@@ -15,6 +15,8 @@
use Symfony\Component\Translation\Dumper\XliffFileDumper;
use Symfony\Component\Translation\Exception\ProviderException;
use Symfony\Component\Translation\Loader\LoaderInterface;
+use Symfony\Component\Translation\MessageCatalogue;
+use Symfony\Component\Translation\MessageCatalogueInterface;
use Symfony\Component\Translation\Provider\ProviderInterface;
use Symfony\Component\Translation\TranslatorBag;
use Symfony\Component\Translation\TranslatorBagInterface;
@@ -63,33 +65,48 @@
$responses = [];
foreach ($translatorBag->getCatalogues() as $catalogue) {
- foreach ($catalogue->getDomains() as $domain) {
- if (0 === \count($catalogue->all($domain))) {
- continue;
- }
+ foreach ($catalogue->getDomains() as $domainWithoutSuffix) {
+ foreach (['', MessageCatalogueInterface::INTL_DOMAIN_SUFFIX] as $suffix) {
+ $domain = $domainWithoutSuffix.$suffix;
- $content = $this->xliffFileDumper->formatCatalogue($catalogue, $domain, ['default_locale' => $this->defaultLocale]);
- $fileId = $this->getFileIdByDomain($fileList, $domain);
+ // Remove ICU messages from the non-ICU catalogue, as MessageCatalogue::all() merges them :/
+ $fixedCatalogue = $catalogue;
- if ($catalogue->getLocale() === $this->defaultLocale) {
- if (!$fileId) {
- $file = $this->addFile($domain, $content);
- } else {
- $file = $this->updateFile($fileId, $domain, $content);
- }
+ if ($domainWithoutSuffix === $domain) {
+ $messages = \Closure::bind(fn() => $this->messages, $catalogue, MessageCatalogue::class)();
+ unset($messages[$domainWithoutSuffix.MessageCatalogueInterface::INTL_DOMAIN_SUFFIX]);
- if (!$file) {
- continue;
+ $fixedCatalogue = new MessageCatalogue($catalogue->getLocale(), $messages);
}
- $fileList[$file['name']] = $file['id'];
- } else {
- if (!$fileId) {
+ if (0 === \count($fixedCatalogue->all($domain))) {
continue;
}
- $responses[] = $this->uploadTranslations($fileId, $domain, $content, $catalogue->getLocale());
+ $content = $this->xliffFileDumper->formatCatalogue($fixedCatalogue, $domain, ['default_locale' => $this->defaultLocale]);
+
+ $fileId = $this->getFileIdByDomain($fileList, $domain);
+
+ if ($fixedCatalogue->getLocale() === $this->defaultLocale) {
+ if (!$fileId) {
+ $file = $this->addFile($domain, $content);
+ } else {
+ $file = $this->updateFile($fileId, $domain, $content);
+ }
+
+ if (!$file) {
+ continue;
+ }
+
+ $fileList[$file['name']] = $file['id'];
+ } else {
+ if (!$fileId) {
+ continue;
+ }
+
+ $responses[] = $this->uploadTranslations($fileId, $domain, $content, $fixedCatalogue->getLocale());
+ }
}
}
}and for Lokalise (the most recent version): --- vendor/symfony/lokalise-translation-provider/LokaliseProvider.php 2022-11-14 19:43:50
+++ vendor/symfony/lokalise-translation-provider/LokaliseProvider.php 2022-11-14 19:51:51
@@ -82,16 +82,18 @@
foreach ($existingKeysByDomain as $domain => $existingKeys) {
$allKeysForDomain = array_keys($defaultCatalogue->all($domain));
+ $allKeysForDomainIcu = array_keys($defaultCatalogue->all($domain.MessageCatalogueInterface::INTL_DOMAIN_SUFFIX));
+
foreach (array_keys($existingKeys) as $keyName) {
unset($allKeysForDomain[$keyName]);
}
- $keysToCreate[$domain] = $allKeysForDomain;
+ $keysToCreate[$domain.MessageCatalogueInterface::INTL_DOMAIN_SUFFIX] = $allKeysForDomainIcu;
+ $keysToCreate[$domain] = array_values(array_diff($allKeysForDomain, $allKeysForDomainIcu));
}
foreach ($keysToCreate as $domain => $keys) {
$createdKeysByDomain[$domain] = $this->createKeys($keys, $domain);
}
-
$this->updateTranslations(array_merge_recursive($createdKeysByDomain, $existingKeysByDomain), $translatorBag);
}I'm not 100% sure that it 100% works, but maybe it can help you in a certain way. |
|
@welcoMattic is my fix is okay for you ? Thanks |
|
@Broutard If I understand correctly this fix, it will merge ICU translations with non-ICU translation in the same Catalogue? If yes, I'm not sure that it will be the right way to fix the issue. If we take a step back, in theory, it's not recommended to work with ICU and non-ICU translations. However in real life, I understand that it could happen. But the fact is Symfony will not differentiate @Kocal worked on some fixes for Crowdin and Lokalise Providers. Moreover, we discussed privately, and he's digging into all these issues to correctly understand what is wrong and how to fix it permanently. @Kocal can you let us know when you open issues/PRs? |
|
@welcoMattic I'm currently donating my blood 😛, and after that I have to work. I will try to open the issues and PRs this evening. |
|
@Kocal no rush 😉 |
|
@welcoMattic Was not really to merge ICU and not ICU, personnaly i've only ICU translations but when I want to push new keys to localise.biz (Loco) This fix is to when you push to Loco merge on to same domain as Loco do with tags and differentiate working with pull command to get back ICU as before. |
|
@a-vallet Ok, so what we have to fix here is to not have both |
|
In fact, when we use By merging ICU translations into the base domain (without |
|
But it's a fix for Loco Provider in a Translations part used by others. Question i've now is : Does this fix impact other providers as Crowdin or Lokalize ? Those was maybe working well. |
|
@a-vallet I did not succeed with Lokalise, I have systematically an error with it : |
|
Closing in favor of #49833 which fix the bug at a lower level |
…gue domains instead of operation domains (welcoMattic) This PR was merged into the 5.4 branch. Discussion ---------- [Translation] TranslatorBag::diff now iterates over catalogue domains instead of operation domains | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | Fix #47886 <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead --> | License | MIT | Doc PR | For Loco Provider (at least, but I'm pretty sure it applies for Lokalise and Crowdin), the `--force` option on `translation:push` command has no effect. The main cause is located in the `TranslatorBag::diff` method which was iterating over `$operation->getDomains()` to add new messages to the diff catalogue. But, the fact is `$operation->getDomains()` returns all domains *AND* their `+intl-icu` variants! Which is certainly not what we want here. See the test that covers the case ```php public function testDiffWithIntlDomain() { $catalogueA = new MessageCatalogue('en', [ 'domain1+intl-icu' => ['foo' => 'foo', 'bar' => 'bar'], 'domain2' => ['baz' => 'baz', 'qux' => 'qux'], ]); $bagA = new TranslatorBag(); $bagA->addCatalogue($catalogueA); $catalogueB = new MessageCatalogue('en', [ 'domain1' => ['foo' => 'foo'], 'domain2' => ['baz' => 'baz', 'corge' => 'corge'], ]); $bagB = new TranslatorBag(); $bagB->addCatalogue($catalogueB); $bagResult = $bagA->diff($bagB); $this->assertEquals([ 'en' => [ 'domain1' => ['bar' => 'bar'], 'domain2' => ['qux' => 'qux'], ], ], $this->getAllMessagesFromTranslatorBag($bagResult)); } ``` Without the change, `'foo' => 'foo'` was in the result bag. Replaces #47986 Commits ------- 178abbf TranslatorBag::diff now iterates over catalogue domains instead of operation domains
Provider translations do not come back with "+intl-icu" in
$provider->read($domains, $locales);, so local translations should not include it for the diff.