Skip to content
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

ICU - Main topic #399

Closed
odolbeau opened this issue Jan 28, 2020 · 5 comments
Closed

ICU - Main topic #399

odolbeau opened this issue Jan 28, 2020 · 5 comments

Comments

@odolbeau
Copy link
Member

odolbeau commented Jan 28, 2020

The ICU problem

For now, we don't support ICU at all in this bundle...

There is a specific issue for the extract command created by @Guite (#300) but it's not specific to this one, download & sync commands don't support ICU neither (see bug encountered by @tristanbes #300 (comment)).

When symfony 4.4 & 5.0 were released, all translations were ICU enabled by default (catalogues suffixed with +intl-icu). It was great for us as we didn't have anything to do to support ICU.
But this behavior was an unexpected BC break and have been removed in symfony/symfony#35351

That's why we need to think about how we want to support ICU.

How Symfony deal with ICU

The current symfony behavior is quite simple:

  • translations stored in messages.en.xlf don't support ICU
  • translations stored in messages+intl-icu.en.xlf support it

That's the ideal situation: developers using a non-compatible ICU format (containing % in parameters for example, see symfony/symfony#35328) are able to use the ICU format for their new translations & to migrate old ones later if they wish.

How we can deal with ICU

Same behavior

In our case, I think its quite hard to keep this behavior. It means we should be able to know if a translation is "ICU ready" or not when we retrieve it from it's adapter.
TBH I don't know how to achieve this easily / without impacting the end user.

ICU only

We could force the usage of the ICU format which is now the standard for new symfony application but this will have some huge impacts:

  • we'll need to release a new major version to stop supporting non ICU translations (as some of them are not compatible with the ICU format)
  • we won't be able to support symfony 3.x anymore as it's not shipped with the ICU support.

Try to guess if ICU ready or not

We could also try to look at the translations to know if they are ICU ready (for example, translations containing parameters surrounding by % are not compatible) but I guess we'll always miss some edge cases & it could introduce an unwanted overhead as we'll have to take a look at each translations parameters.

How should we address this problem?

@php-translation/deciders

@rvanlaak
Copy link
Member

rvanlaak commented Jan 28, 2020

Forcing ICU would be a BC break, so I'd then be more in favor of adding ICU support and letting userland decide whether to enable it or not. Imho guessing would be too error prone.

Would Symfony deprecate their own format (using %) in favor of {? If so, that could be the path to follow as a bundle.

@odolbeau
Copy link
Member Author

I don't know exactly when symfony will remove support for its non ICU format. It could be in 6.0 or it could be never as there is near anything to maintain (the % character was used in the documentation until they switch to ICU format but there is nothing specific to it in the code AFAIK).

@ker0x
Copy link
Contributor

ker0x commented Feb 1, 2020

Wouldn't it be possible to add a new format parameter with 2 possible values, normal (default) and icu? If the format is icu, message domains will be automatically suffixed with +icu-intl

@welcoMattic
Copy link
Member

Symfony deals with ICU like this: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Command/TranslationUpdateCommand.php#L220
It's a behaviour which is copied in TranslationUpdateCommand, MergeOperation and TargetOperation.

In the PR that brings Remote Storages in Symfony, I had to use the trick in another place, so I factorized it https://github.com/symfony/symfony/pull/37462/files#diff-c61b18609ea2781de96eb79e51e895f4R155

May be we could use this in the bundle in order to support ICU format, until the merge and the stabilization of the feature in Symfony? WDYT @php-translation/core

@bocharsky-bw
Copy link
Member

I think this can be closed by #452

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

No branches or pull requests

5 participants