NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Translation] Added intl message formatter.#27399

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

Jump to bottom

Merged

fabpot merged 11 commits intosymfony:masterfromNyholm:intl-formatter

Sep 4, 2018

Merged

[Translation] Added intl message formatter.#27399

fabpot merged 11 commits intosymfony:masterfromNyholm:intl-formatter

Sep 4, 2018

Conversation

Copy link

Member

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	replaces#20007
License	MIT
Doc PR

Choose a reason for hiding this comment

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

When the parsing of this message fails you get a very non-descriptive exception message (Constructor failed). Might be good for DX to rethrow a more descriptive one.

frankdejonge reviewed

May 28, 2018

View reviewed changes

src/Symfony/Component/Translation/Translator.php Outdated

		*/
		privatefunctiongetFormatter(string$domain)
		{
		if (isset($this->formatters[$domain])) {

Copy link

Contributor

Choose a reason for hiding this comment

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

this could be reduced to:

return$this->formatters[$domain] ??$this->formatters['_default'];

Copy link

Contributor

Choose a reason for hiding this comment

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

And we can declare return type also

Koc reviewed

May 29, 2018

View reviewed changes

src/Symfony/Component/Translation/Tests/Formatter/IntlMessageFormatterTest.php Outdated


		publicfunctiontestFormatWithNamedArguments()
		{
		if (PHP_VERSION_ID <50500 \|\|version_compare(INTL_ICU_VERSION,'4.8','<')) {

Copy link

Contributor

Choose a reason for hiding this comment

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

Checking for php version is redundant

symfony/composer.json

Line 19 inf557f94

"php":"^7.1.3",

nicolas-grekas added this to thenext milestone

May 29, 2018

Nyholm force-pushed theintl-formatter branch fromc77b31a toe89ef2fCompare

June 8, 2018 20:26

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

to be removed :)

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Thank you. I added: void as return type.

nicolas-grekas reviewed

Jun 19, 2018

View reviewed changes

src/Symfony/Component/Translation/Translator.php Outdated

		/**
		* @param string $domain
		*
		* @return MessageFormatterInterface

Copy link

Member

Choose a reason for hiding this comment

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

to be removed (and a real return type be used instead)

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Thank you

Nyholm force-pushed theintl-formatter branch from09c3ff6 todbe0ed4Compare

June 21, 2018 16:25

Copy link

MemberAuthor

Nyholm commentedJul 23, 2018

Anything I can do to help review this?

Copy link

Contributor

artursvonda commentedAug 1, 2018

Love the change and allowing to get rid of special case of transChoice (over time).

The original PR mentioned something about storing format with the messages and I actually tend to agree with it. The format IS information that's directly coupled with actual messages (basically, a meta information about contents of the file). This becomes even more important if translations are coming from external source and are not part of source files. There already have been talks about allowing different caching mechanisms and sources (like database) so coupling domain to format in config would not allow for changes in message format without deployment (and would make syncing changes hard anyway).
Catalogues already support metadata so we can add formatter as additional key there and maybe think about making formatter metadata part of actual message which would allow even more granular transition of messages from one format to another.
Architecture wise this would mean a bigger change long term with messages probably being objects (or structs) containing metadata but short term we can just use similar strategy as in this PR but configure domain => formatter relation automatically based on metadata in translation files.

Koc mentioned this pull request

Aug 24, 2018

[Contracts] Add Translation\TranslatorInterface + decouple symfony/validator from symfony/translation#28210

Merged

8 tasks

Copy link

Member

nicolas-grekas commentedAug 24, 2018

Added to project "Contracts" after comment#28210 (comment)

Koc reviewed

Aug 24, 2018

View reviewed changes

src/Symfony/Component/Translation/Formatter/IntlMessageFormatter.php Outdated

		*/
		publicfunctionchoiceFormat($message,$number,$locale,array$parameters =array())
		{
		return$this->format($message,$locale,$parameters);

Copy link

Contributor

KocAug 24, 2018

Choose a reason for hiding this comment

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

Is this kind of implementation right? Argumentnumber is unused.

Copy link

Contributor

jvasseurAug 31, 2018

Choose a reason for hiding this comment

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

I think we shouldn't implement theChoiceMessageFormatterInterface instead, it will disallow using thetransChoice method which is right since we should only use thetrans method for intl-formated message.

Nyholm commented

Aug 30, 2018

View reviewed changes

src/Symfony/Component/Translation/Formatter/IntlMessageFormatter.php Outdated

		}catch (\Throwable$e) {
		thrownew \InvalidArgumentException('Invalid message format.',$e);
		}
		if (null ===$formatter) {

Copy link

MemberAuthor

NyholmAug 30, 2018

Choose a reason for hiding this comment

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

It will never be null here

Copy link

Member

nicolas-grekas commentedAug 30, 2018

I like it, but I would make the new formatter accessible only throughtrans() and not throughtransChoice() which should support only the Symfony syntax.
This would pave the way to deprecating transChoice at a later point in time, followingthis comment from@Koc.
I'd suggest to wait for#28210 to be merged before rebasing this PR and resolving this very comment.

Copy link

Member

nicolas-grekas commentedAug 31, 2018

Some description of the format here:
https://messageformat.github.io/messageformat/page-guide

Nyholmand others added4 commits

September 3, 2018 23:02

Make it a warning

940d440

use error

19e8e69

[Translation] Added intl message formatter.

c2b3dc0

Add support for multiple formatters

b43fe21

Nyholm force-pushed theintl-formatter branch 2 times, most recently from7358b74 todaaceb0Compare

September 3, 2018 22:13

Nyholm added3 commits

September 4, 2018 00:13

Allow config for different domain specific formatters

a325a44

Fixes according to feedback

2aa7181

Use FallbackFormatter instead of support for multiple formatters

597a15d

Nyholm force-pushed theintl-formatter branch fromdaaceb0 to597a15dCompare

September 3, 2018 22:14

Copy link

MemberAuthor

Nyholm commentedSep 3, 2018

I've updated this PR.
I did some major changes. I removed support for multiple formatters. Instead, I added aFallbackFormatter.

With the fallback formatter we can support both "Symfony style plural" and intl messages at the same time. This will make sure we have a zero config solution for adding support for this new feature while all the old formats still works as expected.

Possible update: I could make theFallbackFormatter less generic by using the following constructor signature:FallbackFormatter::__construct(IntlMessageFormatter $first, MessageFormatter $second). The logic will be simpler if I do this change.

fabpot requested changes

Sep 4, 2018

View reviewed changes

src/Symfony/Component/Translation/Tests/Formatter/IntlMessageFormatterTest.php Outdated

		if (!\extension_loaded('intl')) {
		$this->markTestSkipped(
		'The Intl extension is not available.'
		);

Copy link

Member

fabpotSep 4, 2018

Choose a reason for hiding this comment

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

should be on one line

src/Symfony/Component/Translation/Tests/Formatter/IntlMessageFormatterTest.php Outdated


		class IntlMessageFormatterTestextends \PHPUnit\Framework\TestCase
		{
		publicfunctionsetUp()

Copy link

Member

fabpotSep 4, 2018

Choose a reason for hiding this comment

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

should be protected

src/Symfony/Component/Translation/Tests/Formatter/IntlMessageFormatterTest.php Outdated

		);
		}

		privatefunctiongetMessageFormatter()

Copy link

Member

fabpotSep 4, 2018

Choose a reason for hiding this comment

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

I think this can be inlined instead.

src/Symfony/Component/Translation/CHANGELOG.md Outdated

		* Started using ICU parent locales as fallback locales.
		* deprecated`TranslatorInterface` in favor of`Symfony\Contracts\Translation\TranslatorInterface`
		* deprecated`MessageSelector`,`Interval` and`PluralizationRules`; use`IdentityTranslator` instead
		* Added`IntlMessageFormatter` and`FallbackMessageFormatter`

Copy link

Member

fabpotSep 4, 2018

Choose a reason for hiding this comment

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

missing space afterand.

src/Symfony/Component/Translation/Formatter/FallbackFormatter.php Outdated

		* file that was distributed with this source code.
		*/

		declare(strict_types=1);

Copy link

Member

fabpotSep 4, 2018

Choose a reason for hiding this comment

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

AFAIK, we are not using this anywhere else, this should be removed.

src/Symfony/Component/Translation/Formatter/FallbackFormatter.php Outdated

		return$this->secondFormatter->choiceFormat($message,$number,$locale,$parameters);
		}

		thrownewLogicException(sprintf('The no formatter support plural translations.'));

Copy link

Member

fabpotSep 4, 2018

Choose a reason for hiding this comment

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

The message is not so clear to me :) I suppose you meantNo formatters support plural translations.

src/Symfony/Component/Translation/Formatter/IntlMessageFormatter.php Outdated

		try {
		$formatter =new \MessageFormatter($locale,$message);
		}catch (\Throwable$e) {
		thrownewInvalidArgumentException(sprintf('Invalid message format. Reason: %s (error #%d)',intl_get_error_message(),intl_get_error_code()),0,$e);

Copy link

Member

fabpotSep 4, 2018

Choose a reason for hiding this comment

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

As an exception should end with a dot, I suggest to useInvalid message format (%s, error #%d). Same below

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Sep 4, 2018

Updates according to feedback

f88153f

Copy link

MemberAuthor

Nyholm commentedSep 4, 2018

Thank you for the review, Ive updated the PR

Only use the default translator if intl extension is loaded

b1aa004

nicolas-grekas reviewed

Sep 4, 2018

View reviewed changes

src/Symfony/Component/Translation/Formatter/FallbackFormatter.php Outdated

		{
		try {
		$result =$this->firstFormatter->format($message,$locale,$parameters);
		}catch (\Throwable$e) {

Copy link

Member

nicolas-grekasSep 4, 2018

Choose a reason for hiding this comment

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

Can we be more specific here? We should be as much as possible IMHO (same below.)

Copy link

MemberAuthor

NyholmSep 4, 2018

Choose a reason for hiding this comment

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

Do we want to be more specific?
I know the IntlMesaageFormatter only throws InvalidArgument, but isn’t it a good idea to catch everything to give the second formatter a change to run?

Copy link

Member

fabpotSep 4, 2018

Choose a reason for hiding this comment

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

I agree with@nicolas-grekas

Be more specific with what exception we catch

fb30c77

carsonbot removed the Status: Needs Work label

Sep 4, 2018

fabpot approved these changes

Sep 4, 2018

View reviewed changes

Copy link

Member

fabpot left a comment

Choose a reason for hiding this comment

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

With one small comment. In a followup PR, we should deprecate the old way, deprecatetransChoice and probably try to write a script that converts our proprietary format to the intl one. WDYT?

src/Symfony/Component/Translation/Tests/Formatter/FallbackFormatterTest.php Outdated

		useSymfony\Component\Translation\Formatter\FallbackFormatter;
		useSymfony\Component\Translation\Formatter\MessageFormatterInterface;

Copy link

Member

fabpotSep 4, 2018

Choose a reason for hiding this comment

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

should be removed

src/Symfony/Component/Translation/Tests/Formatter/FallbackFormatterTest.php Outdated

		useSymfony\Component\Translation\Formatter\FallbackFormatter;
		useSymfony\Component\Translation\Formatter\MessageFormatterInterface;

Copy link

Member

fabpotSep 4, 2018

Choose a reason for hiding this comment

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

should be reverted

carsonbot added the Status: Reviewed label

Sep 4, 2018

Copy link

Member

nicolas-grekas commentedSep 4, 2018

In a followup PR, we should deprecate the old way, deprecate transChoice and probably try to write a script that converts our proprietary format to the intl one

💯 for both!

cs

2a90931

Copy link

MemberAuthor

Nyholm commentedSep 4, 2018

Thank you. PR updated.

I agree to deprecate transChoice

Copy link

Member

fabpot commentedSep 4, 2018

Thank you@Nyholm.

fabpot merged commit2a90931 intosymfony:master

Sep 4, 2018

fabpot added a commit that referenced this pull request

Sep 4, 2018

feature#27399[Translation] Added intl message formatter. (aitboudad…

baad332

…, Nyholm)This PR was merged into the 4.2-dev branch.Discussion----------[Translation] Added intl message formatter.| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | replaces#20007| License       | MIT| Doc PR        |This PR will replace#20007 and continue the work from the original author.I've have tried to address all comments made in the original PR.Commits-------2a90931 csfb30c77 Be more specific with what exception we catchb1aa004 Only use the default translator if intl extension is loadedf88153f Updates according to feedback597a15d Use FallbackFormatter instead of support for multiple formatters2aa7181 Fixes according to feedbacka325a44 Allow config for different domain specific formattersb43fe21 Add support for multiple formattersc2b3dc0 [Translation] Added intl message formatter.19e8e69 use error940d440 Make it a warning

Nyholm mentioned this pull request

Sep 5, 2018

[Translator] Document intl formatsymfony/symfony-docs#10264

Closed

Nyholm deleted the intl-formatter branch

September 5, 2018 20:21

Nyholm mentioned this pull request

Sep 5, 2018

[Translator] Deprecated transChoice and moved it away from contracts#28375

Merged

Nyholm mentioned this pull request

Sep 17, 2018

[Translation] SimpleIntlMessageFormatter for users with no icu extension#28492

Closed

nicolas-grekas mentioned this pull request

Sep 24, 2018

Gender and number-dependent words in translation#27132

Closed

fabpot added a commit that referenced this pull request

Oct 6, 2018

feature#28375[Translator] Deprecated transChoice and moved it away …

23b0787

…from contracts (Nyholm, nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[Translator] Deprecated transChoice and moved it away from contracts| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets | n/a| License       | MIT| Doc PR        | Issue:symfony/symfony-docs#10264This will address comment made here:#27399 (review)This PR moves the `transChoice()` method away from Contracts and back to the component. I also reverted changes in the `IdentityTranslator`. I will still use the deprecated `MessageSelector` but this is not fine since `transChoice()` is also deprecated.Commits-------dc5f3bf Make trans + %count% parameter resolve pluralsd870a85 [Translator] Deprecated transChoice and moved it away from contracts

nicolas-grekas modified the milestones:next,4.2

Nov 1, 2018

This was referencedNov 3, 2018

Release v4.2.0-BETA1#29072

Closed

Release v4.2.0-BETA1#29073

Merged