Skip to content

The translation:update command should be aware of the +intl-icu variant #34713

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.

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

Closed
fabpot opened this issue Nov 29, 2019 · 9 comments
Closed

The translation:update command should be aware of the +intl-icu variant #34713

fabpot opened this issue Nov 29, 2019 · 9 comments

Comments

@fabpot
Copy link
Member

fabpot commented Nov 29, 2019

If I'm using a file named translations/messages+intl-icu.fr.xlf for translations, the translation:update command will create a translations/messages.fr.xlf file instead of updating the existing one.

/cc @yceruto

@fabpot
Copy link
Member Author

fabpot commented Nov 29, 2019

The translation:update command should also probably generate +intl-icu files by default, no?

@yceruto
Copy link
Member

yceruto commented Dec 2, 2019

#28952
The way to opt-in is to put messages in a domain with the +intl-icu suffix at loading/declaration time.

would that mean you should also update the suffix in the code to make it work properly?

{% trans_default_domain 'messages+intl-icu' %}

It works, but @nicolas-grekas (as author of this code) can confirm it better.

@nicolas-grekas
Copy link
Member

Nope, there should be no need to update the code/template.

@stof
Copy link
Member

stof commented Dec 2, 2019

@yceruto trans_default_domain is part of the usage time, not of the loading/declaration time. Your are translating things there, not defining your translations.

@yceruto
Copy link
Member

yceruto commented Dec 2, 2019

Okay, so it's a buggy behavior related to the FileDumper when there are new translation keys.

The translation:update command should also probably generate +intl-icu files by default, no?

I think it will depend:

Intl extension (or polyfill) installed default domain
no messages
yes messages+intl-icu

but also if we are updating an existing (single) file, we must keep the domain attached to it, regardless whether Intl ext is installed or not. And if both files exist (messages.en.xlf and messages+intl-icu.en.xlf) the new keys should be generated into the default guessed earlier.

Can you confirm that's what we want before make the changes?

@nicolas-grekas
Copy link
Member

@yceruto LGTM!

@yceruto
Copy link
Member

yceruto commented Dec 4, 2019

ready for review #34797

fabpot added a commit that referenced this issue Dec 4, 2019
This PR was merged into the 4.4 branch.

Discussion
----------

[Translation] Fix FileDumper behavior

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

Execute `bin/console translation:update --force en` command:

## Before
See related issue for details #34713

## After
The default translation file name will depend on whether the intl (or polyfill) extension is installed or not.

For exmaple:

| Intl extension (or polyfill) installed | translation file created |
| --- | --- |
| no | messages.en.xlf |
| yes | messages+intl-icu.en.xlf |

However, if you are currently updating a single file, that file name will be used regardless of whether the Intl extension is installed, i.e. if you have this translation file: `messages.en.xlf`, new translation keys will be stored in it, even if you have installed the intl extension.

Last, if both translation files (`messages.es.xlf` and `messages+intl-icu.en.xlf`) coexist in the same path, rare but possible, we will use the default filename guessed earlier to store all current messages and the another file will be emptied.

Commits
-------

1c41ae7 Fixed translations file dumper behavior
@fabpot fabpot closed this as completed Dec 4, 2019
yceruto added a commit to yceruto/symfony that referenced this issue Jan 20, 2020
nicolas-grekas added a commit that referenced this issue Jan 21, 2020
…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"
nicolas-grekas added a commit that referenced this issue Jan 21, 2020
* 4.4:
  [DI] Fix EnvVar not loaded when Loader requires an env var
  Fixed #34713 Move new messages to intl domain when possible
  [FrameworkBundle] Fix small typo in output comment
  chown and chgrp should also accept int as owner and group
  Revert "Fixed translations file dumper behavior"
  Fix RememberMe with null password
  [Validator] Fix plurals for sr_Latn (Serbian language written in latin script) validation messages
  Set booted flag to false when test kernel is unset
  [FrameworkBundle] remove messenger cache if not enabled
  [PhpUnitBridge][SymfonyTestsListenerTrait] Remove some unneeded code
  [HttpClient] Fix strict parsing of response status codes
  fix PHP const mapping keys using the inline notation
  [SecurityBundle] Drop duplicated code
  [FrameworkBundle] Make sure one can use fragments.hinclude_default_template
  Fix that no-cache requires positive validation with the origin, even for fresh responses
  Improve upgrading instructions for deprecated router options
  [DI] Suggest typed argument when binding fails with untyped argument
nicolas-grekas added a commit that referenced this issue Jan 21, 2020
* 5.0:
  [Filesystem] chown and chgrp should also accept int as owner and group
  [DI] Fix EnvVar not loaded when Loader requires an env var
  Fixed #34713 Move new messages to intl domain when possible
  [FrameworkBundle] Fix small typo in output comment
  chown and chgrp should also accept int as owner and group
  Revert "Fixed translations file dumper behavior"
  Fix RememberMe with null password
  [Validator] Fix plurals for sr_Latn (Serbian language written in latin script) validation messages
  Set booted flag to false when test kernel is unset
  [FrameworkBundle] remove messenger cache if not enabled
  [PhpUnitBridge][SymfonyTestsListenerTrait] Remove some unneeded code
  [HttpClient] Fix strict parsing of response status codes
  fix PHP const mapping keys using the inline notation
  [SecurityBundle] Drop duplicated code
  [FrameworkBundle] Make sure one can use fragments.hinclude_default_template
  Fix that no-cache requires positive validation with the origin, even for fresh responses
  Improve upgrading instructions for deprecated router options
  [DI] Suggest typed argument when binding fails with untyped argument
This was referenced Jan 21, 2020
@timosmit
Copy link

timosmit commented Apr 4, 2020

I would like to reopen this issue as I have been experiencing similar issues with the translation:update command after upgrading from 5.0.2 to 5.0.3. Even though I have the PHP intl extension installed, the command refuses to output the messages in the ICU format.

The issue also exists in the latest bugfix release (5.0.7 at the moment of writing).

Here's what I'm currently using:

  • Windows 10
  • PHP 7.4.2
  • symfony/intl 5.0.7
  • symfony/translation 5.0.7

@fabpot
Copy link
Member Author

fabpot commented Apr 15, 2020

Please, open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants