Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Translation] allow using the ICU message format using domains with the "+intl-icu" suffix#28952

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedOct 22, 2018•
edited
Loading

QA
Branch?4.2
Bug fix?no
New feature?yes
BC breaks?no (unless merged after 4.2)
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR-

This PR replaces theFallbackFormatter logic by explicit opt-in.
This allows triggering accurate exceptions when one wants to use the ICU message format but the extension (or the polyfill) is missing.
The way to opt-in is to put messages in a domain with the+intl-icu suffixat loading/declaration time.
E.g. translations inmessages+intl-icu.en.yaml will be read as part of themessages domain , but will be parsed by intl'sMessageFormatter instead of the default Symfony one.
To make it seamless to adopt the ICU format,%s are trimmed from parameter keys (idea borrowed fromhttps://github.com/webfactory/WebfactoryIcuTranslationBundle)

This is for 4.2 as it changes a feature that is not released yet.
ping@Nyholm - we drafted this together.

ro0NL, jvasseur, and yceruto reacted with thumbs up emojisstok reacted with heart emoji
@nicolas-grekasnicolas-grekas added this to the4.2 milestoneOct 22, 2018
@jvasseur
Copy link
Contributor

There is one downside to this compared to theFallbackFormatter, you won't be able to use ICU formatted messages with the validator component since the domain is configured for the whole component. But maybe this is something that should be fixed in the Validator component.

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedOct 22, 2018•
edited
Loading

@jvasseur we should work on this separately I suppose when preparing 4.3. see resolution below

@javiereguiluz
Copy link
Member

The current proposal (adding+intl-icu suffix) looks the best one to me ... but I wonder if this could break things (for naming files likemessages+intl-icu.en.xlf instead ofmessages.en.xlf). But I can't think of concrete examples where things will break. It's just a thought.

@nicolas-grekas
Copy link
MemberAuthor

I wonder if this could break things (for naming files like messages+intl-icu.en.xlf instead of messages.en.xlf)

nothing I can think of: a+ is a legal char in a filename, and also in a domain name, so all good.

@stof
Copy link
Member

This requires updating all places using the translator to use an explicit domain though, making the migrating painful.
Could we rely on some catalogue metadata instead of relying on the domain name instead ? Adding some metadata about which keys are Intl ones would require cooperation from loaders (to fill the info based on some opt-in for the resource), but that's doable as we implement them (and custom loaders would keep registering their keys as non-ICU until they support this opt-in, but I think that's fine as there are much less projects writing custom loaders than projects using the translator).
This way, we would not need to allow changing the translation domain in many places (to be able to switch between the normal one and the+intl-icu one).

jvasseur reacted with thumbs up emoji

@stof
Copy link
Member

the question of course would be: what is the way for loaders to decide whether the resource they load is using the Intl-ICU format or the Symfony one ?
For domain-based opt-in, the domain name is the opt-in.

@stof
Copy link
Member

hmm, actually, looking at the implementation, the+intl-icu suffix is handled internally in the translator, by checking the ICU catalogue first and then the normal one. So there is no change in placesusing the translator (and so this fixes the concerns of@jvasseur as well).

javiereguiluz and nicolas-grekas reacted with thumbs up emoji

@nicolas-grekas
Copy link
MemberAuthor

looking at the implementation, the +intl-icu suffix is handled internally in the translator

Yes! Sorry if that wasn't clear. I updated the PR description.

thrownewLogicException('Cannot parse message translation: please install the "intl" PHP extension or the "symfony/polyfill-intl-messageformatter" package.');
}
try {
$this->cache[$locale][$message] =$formatter =new \MessageFormatter($locale,$message);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

should we actually memoize all the formatters ? This increases memory usage for each message being formatted. In my experience, a page is much more likely to translate lots of different messages than lots of messages being the same.
What is the benefit in terms of speed when skipping the instantiation the second time (when using the intl extension. I don't care about the performance of the polyfill here, as the way to improve performance in such case would be to install ext-intl). Does it actually justify such memory leak ?

Copy link
MemberAuthor

@nicolas-grekasnicolas-grekasOct 24, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I benched thiswith the extension:
\MessageFormatter::formatMessage('Hello {name}', 'en', array('name' => 'Bob'));
vs
$formatter->format(array('name' => 'Bob'));

the later is 3 times faster than the former and each $formatter object takes 226 bytes.
With that in mind, I think it's worth doing it: for run-once requests we don't care, and for long-running ones, the benefit is worth it IMHO.

foreach ($parametersas$key =>$value) {
if ('%' === ($key[0] ??null)) {
unset($parameters[$key]);
$parameters[trim($key,'%')] =$value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

should we also support the{{ name }} convention, which is used in the Validator component for instance ?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

good idead, updated

@nicolas-grekasnicolas-grekasforce-pushed thetranslation-msg-format branch 2 times, most recently fromf221f4a to326dd08CompareOctober 24, 2018 10:42
$this->assertInstanceof(IntlFormatterInterface::class,$formatter);
$this->assertSame('Hello Fab',$formatter->formatIntl('Hello {name}','en',array('name' =>'Fab')));
$this->assertSame('Hello Fab',$formatter->formatIntl('Hello {name}','en',array('%name%' =>'Fab')));
$this->assertSame('Hello Fab',$formatter->formatIntl('Hello {name}','en',array('{{ name }}' =>'Fab')));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Nice

if ($catalogue->defines($id,$domain)) {
break;
}
if ($cat =$catalogue->getFallbackCatalogue()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Are we really sure this always will returnnull eventually?

Copy link
MemberAuthor

@nicolas-grekasnicolas-grekasOct 26, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I suppose: this is the exact same logic as in transChoice. Not having it here was a bug actually (with no side effects since trans wasn't locale sensitive before)

jvasseur reacted with thumbs up emoji
@nicolas-grekas
Copy link
MemberAuthor

Ping @symfony/deciders

@fabpot
Copy link
Member

Thank you@nicolas-grekas.

@fabpotfabpot merged commitd95cc4d intosymfony:masterOct 28, 2018
fabpot added a commit that referenced this pull requestOct 28, 2018
… domains with the "+intl-icu" suffix (nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[Translation] allow using the ICU message format using domains with the "+intl-icu" suffix| Q             | A| ------------- | ---| Branch?       | 4.2| Bug fix?      | no| New feature?  | yes| BC breaks?    | no (unless merged after 4.2)| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -This PR replaces the `FallbackFormatter` logic by explicit opt-in.This allows triggering accurate exceptions when one wants to use the ICU message format but the extension (or the polyfill) is missing.The way to opt-in is to put messages in a domain with the `+intl-icu` suffix *at loading/declaration time*.E.g. translations in `messages+intl-icu.en.yaml` will be read as part of the `messages` domain , but will be parsed by intl's `MessageFormatter` instead of the default Symfony one.To make it seamless to adopt the ICU format, `%`s are trimmed from parameter keys (idea borrowed fromhttps://github.com/webfactory/WebfactoryIcuTranslationBundle)This is for 4.2 as it changes a feature that is not released yet.ping@Nyholm - we drafted this together.Commits-------d95cc4d [Translation] allow using the ICU message format using domains with the "+intl-icu" suffix
@nicolas-grekasnicolas-grekas deleted the translation-msg-format branchOctober 29, 2018 11:48
This was referencedNov 3, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@NyholmNyholmNyholm left review comments

@fabpotfabpotfabpot approved these changes

@stofstofstof approved these changes

@chalasrchalasrchalasr approved these changes

+1 more reviewer

@ogizanagiogizanagiogizanagi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

9 participants

@nicolas-grekas@jvasseur@javiereguiluz@stof@fabpot@Nyholm@ogizanagi@chalasr@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp