[Translation] Added intl message formatter. #27399

Nyholm · 2018-05-28T20:42:46Z

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.

frankdejonge · 2018-05-28T20:51:32Z

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

+     */
+    public function format($message, $locale, array $parameters = array())
+    {
+        $formatter = new \MessageFormatter($locale, $message);


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 · 2018-05-28T20:55:24Z

src/Symfony/Component/Translation/Translator.php

+     */
+    private function getFormatter(string $domain)
+    {
+        if (isset($this->formatters[$domain])) {


this could be reduced to:

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

And we can declare return type also

Koc · 2018-05-29T10:56:14Z

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

+
+    public function testFormatWithNamedArguments()
+    {
+        if (PHP_VERSION_ID < 50500 || version_compare(INTL_ICU_VERSION, '4.8', '<')) {


Checking for php version is redundant

symfony/composer.json

Line 19 in f557f94

"php": "^7.1.3",

Nyholm · 2018-06-08T20:26:39Z

I've rebased this PR and updated according to feedback.

nicolas-grekas · 2018-06-19T13:23:01Z

src/Symfony/Component/Translation/Translator.php

@@ -132,6 +132,15 @@ public function addResource($format, $resource, $locale, $domain = null)
        }
    }

+    /**
+     * @param string                    $domain
+     * @param MessageFormatterInterface $formatter


to be removed :)

Thank you. I added : void as return type.

nicolas-grekas · 2018-06-19T13:23:27Z

src/Symfony/Component/Translation/Translator.php

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


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

Nyholm · 2018-07-23T18:03:52Z

Anything I can do to help review this?

artursvonda · 2018-08-01T14:33:48Z

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.

nicolas-grekas · 2018-08-24T21:27:13Z

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

Koc · 2018-08-24T22:45:58Z

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

+     */
+    public function choiceFormat($message, $number, $locale, array $parameters = array())
+    {
+        return $this->format($message, $locale, $parameters);


Is this kind of implementation right? Argument number is unused.

I think we shouldn't implement the ChoiceMessageFormatterInterface instead, it will disallow using the transChoice method which is right since we should only use the trans method for intl-formated message.

Nyholm · 2018-08-30T13:25:56Z

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

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


It will never be null here

nicolas-grekas · 2018-08-30T14:15:16Z

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

nicolas-grekas · 2018-08-31T15:18:24Z

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

Nyholm · 2018-09-03T22:17:55Z

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

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 the FallbackFormatter 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 · 2018-09-04T05:13:51Z

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

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


should be on one line

fabpot · 2018-09-04T05:14:07Z

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

+
+class IntlMessageFormatterTest extends \PHPUnit\Framework\TestCase
+{
+    public function setUp()


should be protected

fabpot · 2018-09-04T05:15:13Z

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

+        );
+    }
+
+    private function getMessageFormatter()


I think this can be inlined instead.

fabpot · 2018-09-04T05:15:32Z

src/Symfony/Component/Translation/CHANGELOG.md

@@ -7,6 +7,7 @@ CHANGELOG
 * 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`


missing space after and.

fabpot · 2018-09-04T05:16:46Z

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

+ * file that was distributed with this source code.
+ */
+
+declare(strict_types=1);


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

fabpot · 2018-09-04T05:17:19Z

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

+            return $this->secondFormatter->choiceFormat($message, $number, $locale, $parameters);
+        }
+
+        throw new LogicException(sprintf('The no formatter support plural translations.'));


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

fabpot · 2018-09-04T05:30:59Z

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

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


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

Nyholm · 2018-09-04T05:36:51Z

Thank you for the review, Ive updated the PR

nicolas-grekas · 2018-09-04T05:52:45Z

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

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


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

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?

I agree with @nicolas-grekas

fabpot

With one small comment. 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. WDYT?

fabpot · 2018-09-04T06:45:41Z

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

@@ -17,6 +17,7 @@
 use Symfony\Component\Translation\Formatter\FallbackFormatter;
 use Symfony\Component\Translation\Formatter\MessageFormatterInterface;

+


should be removed

fabpot · 2018-09-04T06:46:12Z

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

+use Symfony\Component\Translation\Formatter\FallbackFormatter;
+use Symfony\Component\Translation\Formatter\MessageFormatterInterface;
+
+


should be reverted

nicolas-grekas · 2018-09-04T06:48:54Z

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!

Nyholm · 2018-09-04T06:59:39Z

Thank you. PR updated.

I agree to deprecate transChoice

fabpot · 2018-09-04T07:03:45Z

Thank you @Nyholm.

…, 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 cs fb30c77 Be more specific with what exception we catch b1aa004 Only use the default translator if intl extension is loaded f88153f Updates according to feedback 597a15d Use FallbackFormatter instead of support for multiple formatters 2aa7181 Fixes according to feedback a325a44 Allow config for different domain specific formatters b43fe21 Add support for multiple formatters c2b3dc0 [Translation] Added intl message formatter. 19e8e69 use error 940d440 Make it a warning

…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#10264 This 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 plurals d870a85 [Translator] Deprecated transChoice and moved it away from contracts

carsonbot added Status: Needs Review Translation Feature labels May 28, 2018

frankdejonge reviewed May 28, 2018

View reviewed changes

Koc reviewed May 29, 2018

View reviewed changes

nicolas-grekas added this to the next milestone May 29, 2018

Nyholm force-pushed the intl-formatter branch from c77b31a to e89ef2f Compare June 8, 2018 20:26

fabpot mentioned this pull request Jun 10, 2018

[Translation] Added intl message formatter. #20007

Closed

nicolas-grekas reviewed Jun 19, 2018

View reviewed changes

Nyholm force-pushed the intl-formatter branch from 09c3ff6 to dbe0ed4 Compare June 21, 2018 16:25

Koc mentioned this pull request Aug 24, 2018

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

Merged

8 tasks

nicolas-grekas added this to WIP in Contracts via automation Aug 24, 2018

Koc reviewed Aug 24, 2018

View reviewed changes

Nyholm commented Aug 30, 2018

View reviewed changes

Nyholm and others added 4 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 the intl-formatter branch 2 times, most recently from 7358b74 to daaceb0 Compare September 3, 2018 22:13

Nyholm added 2 commits September 4, 2018 00:13

Allow config for different domain specific formatters

a325a44

Fixes according to feedback

2aa7181

Nyholm force-pushed the intl-formatter branch from daaceb0 to 597a15d Compare September 3, 2018 22:14

fabpot requested changes Sep 4, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Sep 4, 2018

Updates according to feedback

f88153f

Only use the default translator if intl extension is loaded

b1aa004

nicolas-grekas reviewed Sep 4, 2018

View reviewed changes

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

carsonbot added the Status: Reviewed label Sep 4, 2018

cs

2a90931

fabpot merged commit 2a90931 into symfony:master Sep 4, 2018

Contracts automation moved this from WIP to DONE Sep 4, 2018

Nyholm mentioned this pull request Sep 5, 2018

[Translator] Document intl format symfony/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

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

		@@ -17,6 +17,7 @@
		use Symfony\Component\Translation\Formatter\FallbackFormatter;
		use Symfony\Component\Translation\Formatter\MessageFormatterInterface;

[Translation] Added intl message formatter. #27399

[Translation] Added intl message formatter. #27399

Conversation

Nyholm commented May 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Jul 23, 2018

artursvonda commented Aug 1, 2018

nicolas-grekas commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Aug 30, 2018

nicolas-grekas commented Aug 31, 2018

Nyholm commented Sep 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Sep 4, 2018

Nyholm commented Sep 4, 2018

fabpot commented Sep 4, 2018