8000 Unable to dump multi format translations for same domain · Issue #35264 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Unable to dump multi format translations for same domain #35264

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
deguif opened this issue Jan 8, 2020 · 19 comments
Closed

Unable to dump multi format translations for same domain #35264

deguif opened this issue Jan 8, 2020 · 19 comments
Labels
Translation

Comments

@deguif
Copy link
Contributor
deguif commented Jan 8, 2020

Symfony version(s) affected: 4.4.2

Description

We have a translation process where translations are downloaded from an external service then dumped to translation files.
We have two translation formats (Symfony format and icu) for thousands of translations (legacy were using Symfony format, and new ones are using icu).
These translations once retrieved were dumped with XliffFileDumper to two files messages.en.xlf and messages+intl-icu.en.xlf (each one supporting a different format).

Since #34797 was merged, it is not possible anymore to dump two files for same translation domain but with two different formats (icu and Symfony format). All translations are dumped to the same file messages+intl-icu.en.xlf as class \MessageFormatter exists.
@nicolas-grekas
Copy link
Member

/cc @yceruto

@yceruto
Copy link
Member
yceruto commented Jan 8, 2020 •
edited
Loading

Since #34797 was merged, it is not possible anymore to dump two files for same translation domain but with two different formats (icu and Symfony format).

Hi @deguif, the previous behavior to #34797 is faulty because new translations are dumped into messages.en.xlf always, even if you've the messages+intl-icu.en.xlf file and \MessageFormatter exists.

AFAIK, dumping new translations content into both files (at the same time) were never supported. A manual copy/paste/rename actions were required anyway, isn't it?

@deguif
Copy link
Contributor Author
deguif commented Jan 8, 2020 
$catalogue = new MessageCatalogue($locale);
$dumper = new XliffFileDumper();

foreach ($translations as $key => $value) {
    $catalogue->set($key, $value, $domain.$domainSuffix);
}

$dumper->dump($catalogue, [
    'path' => $directory,
    'xliff_version' => '2.0',
]);

Here our simplified code which was dumping the two files (no manual copy/paste/rename) just by varying domains with messages, messages+intl-icu (by the use of $domain and $domainSuffix that can change based on some metatada retrieved from the external service).

For more precisions $domainSuffix can be an empty string '' or the constant MessageCatalogue::INTL_DOMAIN_SUFFIX

This code was dumping two files (one for messages.en.xlf and messages+intl-icu.en.xlf) in the same execution.

@deguif
Copy link
Contributor Author
deguif commented Jan 9, 2020 •
edited
Loading

@yceruto to solve this issue shouldn't the computation (\MessageFormatter class existence and files existence) for the translation domain be moved to the update command in order to add in the catalog the right domain (with icu or no icu suffix) instead of doing the logic in the FileDumper?

That would keep the old behaviour working and allows to write multiple formats for same domain with the file dumper as before.

@yceruto
Copy link
Member
yceruto commented Jan 9, 2020

Hi @deguif, PR welcome as long as this issue #34713 is solved and the behavior achieved on #34797 is preserved.

I don't have much time this week.

@ruudk
Copy link
Contributor
ruudk commented Jan 10, 2020

We're experiencing a similar issue. We have messages.en.yml that we want to dump to messages.en.php. But since MessageFormatter exists, they are getting dumped into messages+intl-icu.en.php instead of messages.en.php.

@yceruto
Copy link
Member
yceruto commented Jan 10, 2020 •
edited
Loading

@ruudk which would only happen if you also have messages+intl-icu.en.php file with other translations, otherwise it should keep all translations inside messages.en.php. If it's not the case, please creates a reproducer to look in.

I don't see the need to keep both files for one project if MessageFormatter exists and working with messages+intl-icu.en.php you'll face better performance.

@ruudk
Copy link
Contributor
ruudk commented Jan 10, 2020

@yceruto That's not the case. I'm converting YAML to PHP, I guess that the PHP dumper only looks for it's own type.

I created a POC that demonstrates the bug. This is my code: ruudk/symfony-translation-bug-poc@86c05a6

And this is what happens when I run the command: ruudk/symfony-translation-bug-poc@84290da

Hope this helps.

@yceruto
Copy link
Member
yceruto commented Jan 10, 2020

@ruudk that is expected if don't have the messages.en.php file. The dumper will do the best for your by creating the messages+intl-icu.en.php if your system has the icu extension enabled.

@yceruto
Copy link
Member
yceruto commented Jan 10, 2020

What I still don't understand in this issue is why you want to generate the messages.en.php file if your system is ready to work better with messages+intl-icu.en.php.

@ruudk
Copy link
Contributor
ruudk commented Jan 10, 2020 •
edited
Loading

Because today we broke production because %parameter% replacement stopped working. This does not work when the file is in +intl-icu modus. The icu modus was enabled because we upgraded to SF 4.4 and the dumper automatically adds it. When I remove the suffix from the file the %parameter% is replaced properly.

@yceruto
Copy link
Member
yceruto commented Jan 10, 2020 •
edited
Loading

Because today we broke production because %parameter% replacement stopped working. This does not work when the file is in +intl-icu modus. The icu modus was enabled because we upgraded to SF 4.4 and the dumper automatically adds it. When I remove the suffix from the file the %parameter% is replaced properly.

I didn't know about that issue with +intl-icu... is it already reported yet?

@ruudk
Copy link
Contributor
ruudk commented Jan 10, 2020

I thought the issue was with the dumper but maybe it’s with the loader not replacing percent placeholders? But on the other hand, it makes sense, because icu has message formatter and that uses {}, right?

So maybe this is a weird circumstance that causes this bug to happen?

@yceruto
Copy link
Member
yceruto commented Jan 10, 2020 •
edited
Loading

@ruudk thanks for your input, I think you found a bug that has not been reported yet. Could you please create a reproducer for that? It would be better create also a separate issue. Thanks again :)

@yceruto
Copy link
Member
yceruto commented Jan 10, 2020

If I understand it correctly, the original issue here is a modern project that generates translation files for modern and legacy projects by using the standalone Translation component. So the dumper is unable to generate the legacy ones when the icu is enabled.

We should re-think this implementations...

@ruudk
Copy link
Contributor
ruudk commented Jan 10, 2020

I will create a separate issue.

@ruudk ruudk mentioned this issue Jan 13, 2020
Closed
@ruudk
Copy link
Contributor
ruudk commented Jan 13, 2020

@yceruto Created issue, do you think this is clear enough? #35328

@yceruto yceruto mentioned this issue Jan 15, 2020
Merged
@yceruto
Copy link
Member
yceruto commented Jan 15, 2020

Reverting #34797 in #35351

8000

@yceruto
Copy link
Member
yceruto commented Jan 15, 2020

see also #35328 (comment)

nicolas-grekas added a commit that referenced this issue Jan 21, 2020
@nicolas-grekas
bug #35351 Revert #34797 "Fixed translations file dumper behavior" and …
039feed 
…fix #34713 (yceruto)

This PR was squashed before being merged into the 4.4 branch.

Discussion
----------

Revert #34797 "Fixed translations file dumper behavior" and fix #34713

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #35264
| License       | MIT
| Doc PR        | -

Revert #34797

See also #35328

It's very likely that the new way will be completely different from this one that is being reverted. That's why I'm reverting rather than fixing it.

Commits
-------

9ca8720 Fixed #34713 Move new messages to intl domain when possible
56e79fe Revert "Fixed translations file dumper behavior"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Translation
Projects
None yet
3BDB
Milestone
No milestone
Development

No branches or pull requests
5 participants
@ruudk @nicolas-grekas @deguif @xabbuh @yceruto
0