Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Translation] Allow default parameters#53425
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
[Translation] Allow default parameters#53425
Uh oh!
There was an error while loading.Please reload this page.
Conversation
5f6aaca
to62cb6a3
CompareFriendly ping@94noni 😉 |
94noni left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nice :)
just a small comment regarding if we can add this feature on the default translator or not
obviously in our app we defined a decorator, but here it is core/new feature so I dont see why a new class is needed (or did I miss something?)
(and your example in PR desc part "# twig template" seems wrong for default using global)
wrong
{{'app.version'|trans }} # Displays "1.2.3"{{'app.version'|trans({'{version}':'2.3.4' }) }} # Displays "2.3.4"
correct
{{'app.version'|trans }} # Displays "Application version: 1.2.3"{{'app.version'|trans({'{version}':'2.3.4' }) }} # Displays "Application version: 2.3.4"
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
lgtm, lets wait other reviews :)
Uh oh!
There was an error while loading.Please reload this page.
aab1d74
toc449989
Comparesrc/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Translation/DefaultParametersTranslator.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Translation/Tests/DefaultParametersTranslatorTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Could it be considered as a BC break since we can use a public method from the |
luxferre commentedMar 2, 2024
Can a global parameter value reference translation key? |
For now, in this PR, a global parameter can be used with a fixed value, a parameter or an environment variable. |
575ce2b
to9410fa4
Comparesrc/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
welcoMattic left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Minor comments, otherwise it's ✅ for me!
@Jean-Beru could you squash your commits please?
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/translation.html.twig OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
64f38ad
to7ca354b
Compare
Thanks@welcoMattic ! PR is fixed, squashed and rebased. |
7ca354b
to9d201f3
Comparesrc/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
1013418
toa59e38a
CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Here is my review as a patch.
diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.phpindex 44e9816e18..eba4ee4534 100644--- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php+++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php@@ -180,6 +180,7 @@ use Symfony\Component\Translation\Command\XliffLintCommand as BaseXliffLintComma use Symfony\Component\Translation\Extractor\PhpAstExtractor; use Symfony\Component\Translation\LocaleSwitcher; use Symfony\Component\Translation\PseudoLocalizationTranslator;+use Symfony\Component\Translation\TranslatableMessage; use Symfony\Component\Translation\Translator; use Symfony\Component\TypeInfo\Type; use Symfony\Component\TypeInfo\TypeResolver\PhpDocAwareReflectionTypeResolver;@@ -1612,11 +1613,7 @@ class FrameworkExtension extends Extension } foreach ($config['globals'] as $name => $global) {- if (isset($global['value'])) {- $translator->addMethodCall('addGlobalParameter', [$name, $global['value']]);- } else {- $translator->addMethodCall('addGlobalTranslatableParameter', [$name, $global['message'], $global['parameters'] ?? [], $global['domain'] ?? null]);- }+ $translator->addMethodCall('addGlobalParameter', [$name, $global['value'] ?? new Definition(TranslatableMessage::class, [$global['message'], $global['parameters'] ?? [], $global['domain'] ?? null])]); } if ($config['pseudo_localization']['enabled']) {diff --git a/src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/translation.html.twig b/src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/translation.html.twigindex 5babc0d893..18319e4993 100644--- a/src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/translation.html.twig+++ b/src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/translation.html.twig@@ -172,18 +172,9 @@ <tr> <td></td> <td>{{ id }}</td>- <td>{{ value }}</td>+ <td>{{ profiler_dump(value) }}</td> </tr> {% endfor %}- {% for locale, values in collector.globalTranslatedParameters %}- {% for id, value in values %}- <tr>- <td>{{ locale }}</td>- <td>{{ id }}</td>- <td>{{ value }}</td>- </tr>- {% endfor %}- {% endfor %} </tbody> </table> {% endif %}diff --git a/src/Symfony/Component/Translation/CHANGELOG.md b/src/Symfony/Component/Translation/CHANGELOG.mdindex 64502a89fd..365c5cf1cd 100644--- a/src/Symfony/Component/Translation/CHANGELOG.md+++ b/src/Symfony/Component/Translation/CHANGELOG.md@@ -4,7 +4,7 @@ CHANGELOG 7.3 ---- * Add `addGlobalParameter` and `addGlobalTranslatableParameter` to allow defining global translation parameters+ * Add `Translator::addGlobalParameter()` to allow defining global translation parameters 7.2 ---diff --git a/src/Symfony/Component/Translation/DataCollector/TranslationDataCollector.php b/src/Symfony/Component/Translation/DataCollector/TranslationDataCollector.phpindex bb7dc6b399..e2e597a705 100644--- a/src/Symfony/Component/Translation/DataCollector/TranslationDataCollector.php+++ b/src/Symfony/Component/Translation/DataCollector/TranslationDataCollector.php@@ -45,7 +45,6 @@ class TranslationDataCollector extends DataCollector implements LateDataCollecto $this->data['locale'] = $this->translator->getLocale(); $this->data['fallback_locales'] = $this->translator->getFallbackLocales(); $this->data['global_parameters'] = $this->translator->getGlobalParameters();- $this->data['global_translated_parameters'] = $this->translator->getGlobalTranslatedParameters(); } public function reset(): void@@ -94,14 +93,6 @@ class TranslationDataCollector extends DataCollector implements LateDataCollecto return $this->data['global_parameters'] ?? []; }- /**- * @internal- */- public function getGlobalTranslatedParameters(): Data|array- {- return $this->data['global_translated_parameters'] ?? [];- }- public function getName(): string { return 'translation';diff --git a/src/Symfony/Component/Translation/Translator.php b/src/Symfony/Component/Translation/Translator.phpindex 6994cb9b6f..d57ec9e2c9 100644--- a/src/Symfony/Component/Translation/Translator.php+++ b/src/Symfony/Component/Translation/Translator.php@@ -61,15 +61,10 @@ class Translator implements TranslatorInterface, TranslatorBagInterface, LocaleA private bool $hasIntlFormatter; /**- * @var array<string, string|int|float>+ * @var array<string, string|int|float|TranslatableInterface> */ private array $globalParameters = [];- /**- * @var array<string, TranslatableInterface>- */- private array $globalTranslatableParameters = [];- /** * @var array<string, string|int|float> */@@ -170,16 +165,11 @@ class Translator implements TranslatorInterface, TranslatorBagInterface, LocaleA return $this->fallbackLocales; }- public function addGlobalParameter(string $id, string|int|float $value): void+ public function addGlobalParameter(string $id, string|int|float|TranslatableInterface $value): void { $this->globalParameters[$id] = $value; }- public function addGlobalTranslatableParameter(string $id, string $message, array $parameters = [], ?string $domain = null): void- {- $this->globalTranslatableParameters[$id] = new TranslatableMessage($message, $parameters, $domain);- }- /** * @internal */@@ -188,14 +178,6 @@ class Translator implements TranslatorInterface, TranslatorBagInterface, LocaleA return $this->globalParameters; }- /**- * @internal- */- public function getGlobalTranslatedParameters(): array- {- return $this->globalTranslatedParameters;- }- public function trans(?string $id, array $parameters = [], ?string $domain = null, ?string $locale = null): string { if (null === $id || '' === $id) {@@ -215,17 +197,24 @@ class Translator implements TranslatorInterface, TranslatorBagInterface, LocaleA } }- if ($this->globalTranslatableParameters && !isset($this->globalTranslatedParameters[$locale])) {- $this->globalTranslatedParameters[$locale] = []; // Avoid infinite loops- $this->globalTranslatedParameters[$locale] = array_map(fn ($parameter) => $parameter->trans($this, $locale), $this->globalTranslatableParameters);+ foreach ($parameters as $key => $value) {+ if ($value instanceof TranslatableInterface) {+ $parameters[$key] = $value->trans($this, $locale);+ } }- if ($this->globalParameters) {- $parameters += $this->globalParameters;++ if (null === $globalParameters =& $this->globalTranslatedParameters[$locale]) {+ $globalParameters = $this->globalParameters;+ foreach ($globalParameters as $key => $value) {+ if ($value instanceof TranslatableInterface) {+ $globalParameters[$key] = $value->trans($this, $locale);+ }+ } }- if (isset($this->globalTranslatedParameters[$locale])) {- $parameters += $this->globalTranslatedParameters[$locale];++ if ($globalParameters) {+ $parameters += $globalParameters; }- $parameters = array_map(fn ($parameter) => $parameter instanceof TranslatableInterface ? $parameter->trans($this, $locale) : $parameter, $parameters); $len = \strlen(MessageCatalogue::INTL_DOMAIN_SUFFIX); if ($this->hasIntlFormatter
The TL;DR is that I don't think that we need nor should have two methods for this.
Also, the PR is missing some cross-components checks I think (fwb / profiler are using feats that might not be available depending on the version of translator that's installed.)
a59e38a
todaea7d8
Compare
Thanks for the review ❤️
I updated the profiler part view but what about the bundle? Should we use |
The cleanest solution for FrameworkBundle is to bump the min supported version of |
Thanks for the tip! |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/translation.html.twig OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
121ce11
to652c658
CompareThank you@Jean-Beru. |
36092eb
intosymfony:7.3Uh oh!
There was an error while loading.Please reload this page.
…ation (94noni)This PR was merged into the 7.3 branch.Discussion----------[Translation] document global variables feature configurationdocumentsymfony/symfony#53425close#20754> [!NOTE]> I am very unsure of the php config via object config (not inline php array)> all example have been taken from the merged PRCommits-------97de6eb [Translator] document global variables feature configuration
Uh oh!
There was an error while loading.Please reload this page.
This PRs adds a
GlobalsTranslator
which decorates theTranslator
when global parameters are given.Usage:
Profiler view:
