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

[Translation] Separate parameters formatting#41136

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

Closed

sylfabre wants to merge1 commit intosymfony:7.1fromsylfabre:translate_date_time_currency

Closed

[Translation] Separate parameters formatting#41136

sylfabre wants to merge1 commit intosymfony:7.1fromsylfabre:translate_date_time_currency

Conversation

Copy link

Contributor

sylfabre commentedMay 8, 2021•
edited
Loading

Q	A
Branch?	5.4
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix#32652
License	MIT
Doc PR	symfony/symfony-docs#...

ICU recommends to first format advanced parameters before translation (https://unicode-org.github.io/icu/userguide/format_parse/messages/#format-the-parameters-separately-recommended).

Thanks to#41858,TranslatableMessage now supports recursiveTranslatableInterfaceas params.

This PR implements a few common use-cases for arguments that must be first formatted to improve DX:

DateTimeParameter to format date and time
MoneyParameter to format monetary values with the support ofhttps://github.com/moneyphp/money
DecimalParameter to format decimal values as per locale specs

The following implementations could be considered in later PRs:

CountryParameter to format the ISO 3166-2 into a localized country name

# Translation file app+intl-icu.en_US.ymlpayment:"{date, date, short} at {date, time, short} - {value, number, currency}"payment2:"{date} at {time} - {value}"# Translation file app+intl-icu.fr_FR.ymlpayment:"{date, date, short} at {date, time, short} - {value, number, currency}"payment2:"{date} at {time} - {value}"

Before this PR

// BASIC USAGE// en_US: 1/25/19 at 7:30 PM - $100.00// fr_FR: 25/01/2019 à 19:30 - 100,00 €$this->translator->trans('payment', ['date' =>new \DateTime('2019-01-25 11:30:00','America/Los_Angeles'),'value' =>100]);// en_US: 1/25/19 at 10:30 AM - $100.00// fr_FR: 25/01/2019 à 10:30 - 100,00 €$this->translator->trans('payment', ['date' =>new \DateTime('2019-01-25 11:30:00','Europe/Paris'),'value' =>100]);// ADVANCED USAGE// en_US: 1/25/19 at 11:30 AM - $100.00// fr_FR: 25/01/2019 à 11:30 - 100,00 $$userCurrency ='USD';$datetime =new \DateTime('2019-01-25 11:30:00','Europe/Paris');$dateFormatter =newIntlDateFormatter($this->translator->getLocale(), IntlDateFormatter::SHORT, IntlDateFormatter::NONE,$datetime->getTimezone());$timeFormatter =newIntlDateFormatter($this->translator->getLocale(), IntlDateFormatter::NONE, IntlDateFormatter::SHORT,$datetime->getTimezone());$numberFormatter =newNumberFormatter($this->translator->getLocale(), NumberFormatter::CURRENCY);$this->translator->trans('payment2', ['date' =>$dateFormatter->format($datetime),'time' =>$timeFormatter->format($datetime),'value' =>$numberFormatter->formatCurrency(100,$userCurrency)]);

After this PR

// BASIC USAGE// No change// ADVANCED USAGE// en_US: 1/25/19 at 11:30 AM - $100.00// fr_FR: 25/01/2019 à 11:30 - 100,00 $$datetime =new \DateTime('2019-01-25 11:30:00','Europe/Paris');$this->translator->trans('payment2', ['date' => DateTimeTranslatable::date($datetime),'time' => DateTimeTranslatable::time($datetime),'value' =>newMoneyTranslatable(100,'USD'),'value' => MoneyTranslatable::fromMoney(Money::USD(10000)),// Alternative with money-php]);// Or with TranslatableMessage for TwignewTranslatableMessage('payment2', ['date' => DateTimeTranslatable::date($datetime),'time' => DateTimeTranslatable::time($datetime),'value' =>newMoneyTranslatable(100,'USD'),'value' => MoneyTranslatable::fromMoney(Money::USD(10000)),// Alternative with money-php]);

TODO

Fix tests
AddDecimalParameter
Documentation PR

carsonbot added Status: Needs Work Deprecation Feature labels

May 8, 2021

sylfabre mentioned this pull request

May 8, 2021

[Translator] Why does the currency depend on the locale?#32652

Closed

derrabus added Translation and removed Deprecation labels

May 8, 2021

carsonbot changed the title~~[wip]~~[Translation] [wip]

May 8, 2021

derrabus added this to the5.x milestone

May 8, 2021

Copy link

Member

derrabus commentedMay 8, 2021

Please give your PR a meaningful title.

sylfabre changed the title~~[Translation] [wip]~~[Translation] [wip] Advanced localization with custom currency and timezone

May 8, 2021

sylfabre force-pushed thetranslate_date_time_currency branch from08f1df0 to4cde975Compare

May 19, 2021 16:09

This comment was marked as resolved.

ro0NL reviewed

May 28, 2021

View reviewed changes

src/Symfony/Component/Translation/Translatable/TranslatableCurrency.php OutdatedShow resolvedHide resolved

sylfabre force-pushed thetranslate_date_time_currency branch 2 times, most recently from60985be tocdc168fCompare

May 30, 2021 19:17

sylfabre changed the title~~[Translation] [wip] Advanced localization with custom currency and timezone~~[Translation] [wip] Separate parameters formatting

May 30, 2021

sylfabre force-pushed thetranslate_date_time_currency branch 8 times, most recently from539ce31 tof274351Compare

May 30, 2021 20:41

Copy link

ContributorAuthor

sylfabre commentedMay 30, 2021

@ro0NL I've actually created another interface:ParameterInterface as my work resolves around parameters formatting instead of translation. Feel free to have a look :)

sylfabre changed the title~~[Translation] [wip] Separate parameters formatting~~[Translation] Separate parameters formatting

May 30, 2021

sylfabre force-pushed thetranslate_date_time_currency branch 4 times, most recently from9b1506d toed8eae3Compare

June 1, 2021 08:04

sylfabre force-pushed thetranslate_date_time_currency branch 3 times, most recently from0ce48cf tocc02f6aCompare

December 6, 2021 08:45

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Should we use constructor promotion for all features targeting 6.1?

Copy link

Member

Choose a reason for hiding this comment

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

yes (especially now that we target 7.1)

ro0NL mentioned this pull request

Dec 14, 2021

LessThanOrEqual is not formatting compared value in error#44619

Closed

sylfabre mentioned this pull request

Dec 16, 2021

[Translation] Translatable parameters#44672

Merged

fabpot added a commit that referenced this pull request

Dec 17, 2021

feature#44672[Translation] Translatable parameters (sylfabre)

e5ea631

This PR was merged into the 6.1 branch.Discussion----------[Translation] Translatable parameters| Q             | A| ------------- | ---| Branch?       | 6.1| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       | N/A| License       | MIT| Doc PR        | TBD✅  The TwigExtension now supports messages implementing `TranslatableInterface` (https://symfony.com/blog/new-in-symfony-5-2-translatable-objects).✅ Thanks to#41858, `TranslatableMessage` now supports recursive `TranslatableInterface` as params.❌ But using `TranslatableInterface` as params with regular messages is not supported yet.💡 This PR addresses this issue and makes it possible to create dedicated `TranslatableInterface` implementations like the one from#41136_Note: I would like also to add `TranslatableInterface` to the `trans(?string $id)` signature method like with the one from the [TwigExtension](https://github.com/symfony/symfony/blob/6.1/src/Symfony/Bridge/Twig/Extension/TranslationExtension.php#L111) but I don't know how to do it without breaking BC. Any advice is welcome!_Commits-------5beaee8 [Translation] Translatable parameters

symfony-splitter pushed a commit to symfony/translation that referenced this pull request

Dec 17, 2021

feature #44672 [Translation] Translatable parameters (sylfabre)

72e9ca1

This PR was merged into the 6.1 branch.Discussion----------[Translation] Translatable parameters| Q             | A| ------------- | ---| Branch?       | 6.1| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       | N/A| License       | MIT| Doc PR        | TBD✅  The TwigExtension now supports messages implementing `TranslatableInterface` (https://symfony.com/blog/new-in-symfony-5-2-translatable-objects).✅ Thanks to #41858, `TranslatableMessage` now supports recursive `TranslatableInterface` as params.❌ But using `TranslatableInterface` as params with regular messages is not supported yet.💡 This PR addresses this issue and makes it possible to create dedicated `TranslatableInterface` implementations like the one fromsymfony/symfony#41136_Note: I would like also to add `TranslatableInterface` to the `trans(?string $id)` signature method like with the one from the [TwigExtension](https://github.com/symfony/symfony/blob/6.1/src/Symfony/Bridge/Twig/Extension/TranslationExtension.php#L111) but I don't know how to do it without breaking BC. Any advice is welcome!_Commits-------5beaee85f9 [Translation] Translatable parameters

fabpot modified the milestones:6.1,6.2

May 20, 2022

SerhiyMytrovtsiy added a commit to SerhiyMytrovtsiy/translation that referenced this pull request

Oct 19, 2022

feature #44672 [Translation] Translatable parameters (sylfabre)

538d824

This PR was merged into the 6.1 branch.Discussion----------[Translation] Translatable parameters| Q             | A| ------------- | ---| Branch?       | 6.1| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       | N/A| License       | MIT| Doc PR        | TBD✅  The TwigExtension now supports messages implementing `TranslatableInterface` (https://symfony.com/blog/new-in-symfony-5-2-translatable-objects).✅ Thanks to #41858, `TranslatableMessage` now supports recursive `TranslatableInterface` as params.❌ But using `TranslatableInterface` as params with regular messages is not supported yet.💡 This PR addresses this issue and makes it possible to create dedicated `TranslatableInterface` implementations like the one fromsymfony/symfony#41136_Note: I would like also to add `TranslatableInterface` to the `trans(?string $id)` signature method like with the one from the [TwigExtension](https://github.com/symfony/symfony/blob/6.1/src/Symfony/Bridge/Twig/Extension/TranslationExtension.php#L111) but I don't know how to do it without breaking BC. Any advice is welcome!_Commits-------5beaee85f9 [Translation] Translatable parameters

nicolas-grekas modified the milestones:6.2,6.3

Nov 5, 2022

nicolas-grekas modified the milestones:6.3,6.4

May 23, 2023

welcoMattic reviewed

Sep 20, 2023

View reviewed changes

src/Symfony/Component/Translation/CHANGELOG.md

Comment on lines +7 to +8

		* Add new implementations of`TranslatableInterface` to format parameters
		separately as recommended per the ICU for DateTime, money, and decimal values.

Copy link

Member

Choose a reason for hiding this comment

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

After rebase, this should be moved to 6.4 section

Copy link

Member

welcoMattic commentedSep 20, 2023

@sylfabre what is the status of this PR? There are some not addressed comments and the branch needs to be rebased, but I think we are close to be able to merge it.

Copy link

Contributor

OskarStark commentedOct 24, 2023

friendly ping@sylfabre 👋

Copy link

ContributorAuthor

sylfabre commentedOct 24, 2023

@welcoMattic @OskarStark Sorry for the late answer: the back-to-school period was challenging on my side 🙈
I need to find some time to get back to work on this one. 😊

Before that:@stof @nicolas-grekas does this suggestion/PR make sense to you? Is working on it worth it?

Copy link

Member

nicolas-grekas commentedOct 24, 2023

No objections on my side, let's resume the PR for 7.1.
I guess this can be useful in twig templates also, isn't it?

stof reviewed

Oct 24, 2023

View reviewed changes

src/Symfony/Component/Translation/Translatable/DateTimeTranslatable.php

		$key =implode('.', [$locale,$this->dateType,$this->timeType,$timezone->getName()]);
		if (!isset(self::$formatters[$key])) {
		self::$formatters[$key] =new \IntlDateFormatter(
		$locale ??$translator->getLocale(),

Copy link

Member

Choose a reason for hiding this comment

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

the coalesce is dead code as the default is already applied in the variable a few lines above

src/Symfony/Component/Translation/Translatable/DateTimeTranslatable.php


		privatestaticarray$formatters = [];

		publicfunction__construct(

Copy link

Member

Choose a reason for hiding this comment

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

I suggest adding@param \IntlDateFormatter::* $dateType (and same for the other parameters in this constructor and static factory methods) so that static analysis tool can report invalid values

src/Symfony/Component/Translation/Translatable/DateTimeTranslatable.php

		$this->dateTime =$dateTime;
		$this->dateType =$dateType;
		$this->timeType =$timeType;
		}

Copy link

Member

Choose a reason for hiding this comment

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

yes (especially now that we target 7.1)

src/Symfony/Component/Translation/composer.json

		"symfony/yaml":"",
		"psr/log-implementation":"To use logging capability in translator"
		"psr/log-implementation":"To use logging capability in translator",
		"moneyphp/money":"To use money capability in translator"

Copy link

Member

Choose a reason for hiding this comment

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

We stopped usingsuggest in the composer packages.

Thus, this suggestion is useless. If you don't already use this money library, there is no reason to suggest you to use it to use the MoneyTranslatable object (you would use its constructor directly). And if you already use the library, composer won't display the suggestion as it is already part of the project.