Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

sylfabre
Copy link
Contributor

@sylfabre sylfabre commented May 8, 2021

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 recursive TranslatableInterfaceas 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 of https://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.yml
payment: "{date, date, short} at {date, time, short} - {value, number, currency}"
payment2: "{date} at {time} - {value}"

# Translation file app+intl-icu.fr_FR.yml
payment: "{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 = new IntlDateFormatter($this->translator->getLocale(), IntlDateFormatter::SHORT, IntlDateFormatter::NONE, $datetime->getTimezone());
$timeFormatter = new IntlDateFormatter($this->translator->getLocale(), IntlDateFormatter::NONE, IntlDateFormatter::SHORT, $datetime->getTimezone());
$numberFormatter = new NumberFormatter($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' => new MoneyTranslatable(100, 'USD'),
    'value' => MoneyTranslatable::fromMoney(Money::USD(10000)), // Alternative with money-php
]);

// Or with TranslatableMessage for Twig
new TranslatableMessage('payment2', [
    'date' => DateTimeTranslatable::date($datetime),
    'time' => DateTimeTranslatable::time($datetime),
    'value' => new MoneyTranslatable(100, 'USD'),
    'value' => MoneyTranslatable::fromMoney(Money::USD(10000)), // Alternative with money-php
]);

TODO

  • Fix tests
  • Add DecimalParameter
  • Documentation PR

@derrabus
Copy link
Member

derrabus commented May 8, 2021

Please give your PR a meaningful title.

@sylfabre sylfabre changed the title [Translation] [wip] [Translation] [wip] Advanced localization with custom currency and timezone May 8, 2021
@sylfabre sylfabre force-pushed the translate_date_time_currency branch from 08f1df0 to 4cde975 Compare May 19, 2021 16:09
ro0NL

This comment was marked as resolved.

@sylfabre sylfabre force-pushed the translate_date_time_currency branch 2 times, most recently from 60985be to cdc168f Compare May 30, 2021 19:17
@sylfabre sylfabre changed the title [Translation] [wip] Advanced localization with custom currency and timezone [Translation] [wip] Separate parameters formatting May 30, 2021
@sylfabre sylfabre force-pushed the translate_date_time_currency branch 8 times, most recently from 539ce31 to f274351 Compare May 30, 2021 20:41
@sylfabre
Copy link
Contributor Author

@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 sylfabre changed the title [Translation] [wip] Separate parameters formatting [Translation] Separate parameters formatting May 30, 2021
@sylfabre sylfabre force-pushed the translate_date_time_currency branch 4 times, most recently from 9b1506d to ed8eae3 Compare June 1, 2021 08:04
@sylfabre sylfabre force-pushed the translate_date_time_currency branch 3 times, most recently from 0ce48cf to cc02f6a Compare December 6, 2021 08:45
@sylfabre
Copy link
Contributor Author

sylfabre commented Dec 6, 2021

Thank you @stof for the review, I've fixed my PR

@sylfabre sylfabre force-pushed the translate_date_time_currency branch 2 times, most recently from 0f0101c to b642d18 Compare December 6, 2021 09:20
@sylfabre sylfabre force-pushed the translate_date_time_currency branch from b642d18 to dab3f2a Compare December 6, 2021 09:35
$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.

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)

fabpot added a commit that referenced this pull request Dec 17, 2021
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
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 symfony/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 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
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 symfony/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 nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
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

@welcoMattic
Copy link
Member

@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.

@OskarStark
Copy link
Contributor

friendly ping @sylfabre 👋

@sylfabre
Copy link
Contributor Author

@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?

@nicolas-grekas
Copy link
Member

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?

$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


private static array $formatters = [];

public function __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

$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)

@@ -47,7 +47,8 @@
"suggest": {
"symfony/config": "",
"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 using suggest 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.

Copy link
Member

Choose a reason for hiding this comment

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

thus, your PR does not even contain the fromMoney static method described in the PR description.

@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@fabpot
Copy link
Member

fabpot commented Feb 4, 2024

Closing as there is no more activity. Feel to reopen when you have time.

@fabpot fabpot closed this Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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