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

nicolas-grekas · 2018-08-16T13:57:15Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#15714, #18930
License	MIT
Doc PR	-

Let's decouple Validator from Translation component \o/!

TODO:

add TranslatorInterface, deprecate it from Translation
add TranslatorTrait, deprecating MessageSelector, Internal and PluralizationRules
deprecate ValidatorBuilderInterface(LegacyTranslatorInterface)
inject a new identity_translator into translator.formatter.default, deprecate translator.selector
copy tests in the Contracts namespace to ensure the TranslatorTrait behaves properly
figure out a way to keep throwing InvalidArgumentException from the component
update UPGRADING and CHANGELOG files
polish the deprecation layer (ensure all needed runtime deprecations are here)

Reviews welcome already.

stof

This is removing an extension point, as your translator is now performing replacements of parameters instead of separating the choice of the pluralization variant from the formatting of parameters (which was the reason to create MessageFormatter and MessageSelector)

stof · 2018-08-16T15:53:23Z

src/Symfony/Component/Validator/ValidatorBuilder.php

@@ -295,7 +300,7 @@ public function getLoaders()
    /**
     * {@inheritdoc}
     */
-    public function getValidator()
+    public function getValidator(/*TranslatorInterface $translator = null*/)


that looks wrong to me. This class is a builder, so passing one of the (optional) dependencies to the final building method makes this one very special compared to the other settings.

I get that. That's the best I could think of for now. Any better suggestion?

IMO, it should stay on the setter, like today

I'd like to. How could we deal with BC then?

Well, too bad that we haven't made this builder a @final class but an interface with an extendable implementation instead. That does not make sense. ValidatorBuilder is not an extension point (the interface is useless as it is not injected anywhere, and would forbid adding any method).
So for now, it would require using a new method as we cannot remove the typehint. to accept both.

We can deprecate the interface and make the class final. That should do it.
In order to pass the type hint, we would need to make the new TranslatorInterface extend the old one when the component is found. Would that work for you?

new TranslatorInterface extend the old one

done, and made ValidatorBuilder final + deprecated its interface

stof · 2018-08-16T16:00:25Z

However, note that I'm not sure it is the right extension point anyway, as an implementation based on the ICU message format would not be able to split this (it would not have transChoice at all btw)

nicolas-grekas · 2018-08-16T16:18:08Z

This is removing an extension point, as your translator is now performing replacements of parameters instead of separating the choice of the pluralization variant from the formatting of parameters (which was the reason to create MessageFormatter and MessageSelector)

MessageFormatter will remain as extensible as it is currently: you can pass it an identity translator of your own to resolve the plural rules.
You're correct about IdentityTranslator: it would make no sense to make its selector logic extensible when creating a custom identity translator is precisely the way to define a custom selector logic now.

Note that if we really want an interoperable interface, the parsing of choices needs to be in Contracts. That's why TranslatorTrait exists in the first place since its logic is far from being trivial to implement.

There is another extension point that this plans to remove: PluralizationRules::set(). Honestly, I don't think we need to make this extensible (and I don't know how to make it extensible if we really need to BTW. Could be figured out later IMHO, if someone comes with a use case.)

stof · 2018-08-16T16:26:41Z

There is another extension point that this plans to remove: PluralizationRules::set(). Honestly, I don't think we need to make this extensible (and I don't know how to make it extensible if we really need to BTW. Could be figured out later IMHO, if someone comes with a use case.)

I think we're fine with deprecating this. If the rules for an existing locale are wrong, people should contribute a fix in Symfony instead of fixing it in their own project. And custom locales would be weird.

nicolas-grekas · 2018-08-16T20:37:03Z

TODO list completed and tests green. Ready for final review.

xabbuh · 2018-08-17T07:10:32Z

src/Symfony/Component/Validator/composer.json

@@ -33,6 +32,7 @@
        "symfony/expression-language": "~3.4|~4.0",
        "symfony/cache": "~3.4|~4.0",
        "symfony/property-access": "~3.4|~4.0",
+        "symfony/translation": "~3.4|~4.0",


This looks wrong to me. Older version of the component do not implement the new interface. And don't we also need a conflict rule for older component versions?

xabbuh · 2018-08-17T07:12:02Z

UPGRADE-4.2.md

+Translation
+-----------
+
+ * The `TranslationInterface` has been deprecated in favor of `Symfony\Contracts\Translation\TranslationInterface`


TranslatorInterface (same below)

stof · 2018-08-17T08:07:53Z

should we actually use class_alias for the BC layer ? This makes it impossible to throw a deprecation warning for people using the old name (and so we cannot really remove it)

nicolas-grekas · 2018-08-17T08:27:54Z

Older version of the component do not implement the new interface

It works because this uses class_alias(), but see below.

should we actually use class_alias for the BC layer ? This makes it impossible to throw a deprecation warning for people using the old name (and so we cannot really remove it)

Whatever the deprecation layer, we cannot throw a deprecation, because our own implementations would trigger it. We've already done like that before I think. The deprecation will be triggered by DebugClassLoader actually so the forward path is fine.

What worries me more is the class_alias in Contracts: we won't ever be able to remove it.
All the complexity resolves around finding a BC path to preserve the type-hint of ValidatorBuilder::setTranslator().
But in the end, I'm wondering if we shouldn't just break BC here: nobody reasonably implements this interface nor extends this class. Don't you think?

stof · 2018-08-17T08:31:08Z

But in the end, I'm wondering if we shouldn't just break BC here: nobody reasonably implements this interface nor extends this class. Don't you think?

yeah, we might end up with this option (and deprecating the interface and marking the class as @final maybe too ?)

nicolas-grekas · 2018-08-17T09:35:45Z

I found https://github.com/tomazahlin/symfony-core-bundle/blob/master/src/Ahlin/Bundle/CoreBundle/Tests/Mock/ValidatorBuilderMock.php and https://github.com/oro-subtree/EntityExtendBundle/blob/master/Validator/ValidatorBuilder.php that implement ValidatorBuilderInterface.
I think both could extend ValidatorBuilder instead.

Which means we should maybe not make the class final. Let's just deprecate the interface (it's really not a type we need) and do the BC break on the type hint. The listed use cases will need to extend ValidatorBuilder instead and remove the custom implementation for setTranslator at least. That would provide them with a way to keep compatibility with all versions of the Translation component. OK for you?

nicolas-grekas · 2018-08-17T12:13:30Z

It took me a while, but I finally figured out a way to move forward without breaking BC.
See LegacyTranslatorProxy and AddValidatorInitializersPass.

PR green.

UPGRADE-4.2.md

stof · 2018-08-17T12:59:00Z

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

    {
-        $this->selector = $selector ?: new MessageSelector();
+        $this->translator = $translator instanceof MessageSelector ? new IdentityTranslator($translator) : $translator ?? new IdentityTranslator();


we should throw an exception if we receive something else than TranslatorInterface|MessageSelector|null, as we do in other places having such BC layer

btw, TypeError might be a good one (now that we are on PHP 7+), as that's the same one than thrown by the native typehint

src/Symfony/Component/Translation/IdentityTranslator.php

src/Symfony/Component/Translation/Interval.php

Koc · 2018-08-24T21:13:46Z

src/Symfony/Contracts/Translation/TranslatorInterface.php

+     *
+     * @throws InvalidArgumentExceptionInterface If the locale contains invalid characters
+     */
+    public function transChoice($id, $number, array $parameters = array(), $domain = null, $locale = null);


Do we need transChoice in contracts? We already have message formatters and unmerged PR #27399 which allow get rid of transChoice.

Good point. I think we should do it one step at a time: first merge this PR, then refactor #27399 on top, maybe removing the method if proved ok. We still have a few months to finish these.

nicolas-grekas · 2018-08-30T13:09:26Z

thanks (just rebased to fix a minor conflict, ready)

fabpot · 2018-08-30T16:31:20Z

src/Symfony/Contracts/Translation/InvalidArgumentExceptionInterface.php

+namespace Symfony\Contracts\Translation;
+
+/**
+ * Exception interface for all exceptions thrown by the component.


Looks wrong to me

But anyway, why do we need this interface in the first place?

I removed InvalidArgumentExceptionInterface and updated TranslationInterface with @throws \InvalidArgumentException instead.

src/Symfony/Component/Validator/ValidatorBuilder.php

nicolas-grekas · 2018-08-31T08:16:56Z

Comments addressed thanks.

stof · 2018-09-03T13:00:19Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php

@@ -48,7 +49,7 @@ public function load(array $configs, ContainerBuilder $container)
            $loader->load('console.xml');
        }

-        if (!interface_exists('Symfony\Component\Translation\TranslatorInterface')) {
+        if (!interface_exists(TranslatorInterface::class)) {


this change is wrong. We still need to detect the translation component itself to keep the extension, not the contracts component (you could have it without having the translation component)

good catch, fixed thanks

…lidator from symfony/translation

fabpot · 2018-09-03T20:20:53Z

Thank you @nicolas-grekas.

…uple symfony/validator from symfony/translation (nicolas-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- [Contracts] Add Translation\TranslatorInterface + decouple symfony/validator from symfony/translation | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #15714, #18930 | License | MIT | Doc PR | - Let's decouple Validator from Translation component \o/! TODO: - [x] add `TranslatorInterface`, deprecate it from Translation - [x] add `TranslatorTrait`, deprecating `MessageSelector`, `Internal` and `PluralizationRules` - [x] deprecate `ValidatorBuilderInterface(LegacyTranslatorInterface)` - [x] inject a new `identity_translator` into `translator.formatter.default`, deprecate `translator.selector` - [x] copy tests in the Contracts namespace to ensure the `TranslatorTrait` behaves properly - [x] figure out a way to keep throwing `InvalidArgumentException` from the component - [x] update UPGRADING and CHANGELOG files - [x] polish the deprecation layer (ensure all needed runtime deprecations are here) Reviews welcome already. Commits ------- 064e369 [Contracts] Add Translation\TranslatorInterface + decouple symfony/validator from symfony/translation

…nt (sroze) This PR was merged into the 4.2-dev branch. Discussion ---------- [Validator] Allow Validator without the translator component | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #28210 | License | MIT | Doc PR | ø Validator should be available without the Translator service. #28210 introduced a regression, it was not the case anymore: ``` You have requested a non-existent service "translator". ``` This fixes it. Commits ------- 2dc92d7 Allow validator without the translator

nicolas-grekas added this to the next milestone Aug 16, 2018

carsonbot added Status: Needs Review Feature Deprecation labels Aug 16, 2018

nicolas-grekas mentioned this pull request Aug 16, 2018

Decouple Validator from Translation component #15714

Closed

nicolas-grekas force-pushed the contract-trans branch from 6d69cf9 to e850ffa Compare August 16, 2018 14:18

stof reviewed Aug 16, 2018

View reviewed changes

nicolas-grekas force-pushed the contract-trans branch 3 times, most recently from 1f4367e to d1063a2 Compare August 16, 2018 20:36

xabbuh reviewed Aug 17, 2018

View reviewed changes

nicolas-grekas force-pushed the contract-trans branch 2 times, most recently from 00ea207 to d1bea0d Compare August 17, 2018 12:00

stof reviewed Aug 17, 2018

View reviewed changes

nicolas-grekas mentioned this pull request Aug 17, 2018

[Validator] There is no way to add custom validator loader since symfony 2.5 #18930

Closed

nicolas-grekas force-pushed the contract-trans branch 5 times, most recently from d4ab6f7 to 6a9762c Compare August 19, 2018 13:01

Koc reviewed Aug 24, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Aug 30, 2018

nicolas-grekas force-pushed the contract-trans branch from 6a9762c to 80ac68c Compare August 30, 2018 13:09

nicolas-grekas force-pushed the contract-trans branch from 80ac68c to 866b6a9 Compare August 30, 2018 14:02

fabpot reviewed Aug 30, 2018

View reviewed changes

nicolas-grekas force-pushed the contract-trans branch from 866b6a9 to c24c59f Compare August 31, 2018 08:13

fabpot approved these changes Sep 2, 2018

View reviewed changes

nicolas-grekas force-pushed the contract-trans branch 3 times, most recently from 118669d to 2f94d74 Compare September 3, 2018 13:01

stof reviewed Sep 3, 2018

View reviewed changes

nicolas-grekas force-pushed the contract-trans branch from 2f94d74 to d72a85f Compare September 3, 2018 13:04

[Contracts] Add Translation\TranslatorInterface + decouple symfony/va…

064e369

…lidator from symfony/translation

nicolas-grekas force-pushed the contract-trans branch from d72a85f to 064e369 Compare September 3, 2018 13:19

stof approved these changes Sep 3, 2018

View reviewed changes

fabpot merged commit 064e369 into symfony:master Sep 3, 2018

nicolas-grekas deleted the contract-trans branch September 4, 2018 06:45

javiereguiluz mentioned this pull request Sep 7, 2018

Update examples to use classes from Symfony\Contracts\ symfony/symfony-docs#10271

Closed

sroze mentioned this pull request Sep 19, 2018

[Validator] Allow Validator without the translator component #28520

Merged

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

javiereguiluz mentioned this pull request Jun 10, 2019

Rendering Forms without Translators and Validators #31968

Closed

wouterj mentioned this pull request Dec 9, 2020

[Translator] Updated TranslationInterface import symfony/symfony-docs#14667

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nicolas-grekas commented Aug 16, 2018 •

edited

Loading

stof left a comment

stof Aug 16, 2018

nicolas-grekas Aug 16, 2018

stof Aug 16, 2018

nicolas-grekas Aug 16, 2018 •

edited

Loading

stof Aug 16, 2018

nicolas-grekas Aug 16, 2018

nicolas-grekas Aug 16, 2018 •

edited

Loading

stof commented Aug 16, 2018

nicolas-grekas commented Aug 16, 2018 •

edited

Loading

stof commented Aug 16, 2018

nicolas-grekas commented Aug 16, 2018

xabbuh Aug 17, 2018

xabbuh Aug 17, 2018

stof commented Aug 17, 2018

nicolas-grekas commented Aug 17, 2018

stof commented Aug 17, 2018

nicolas-grekas commented Aug 17, 2018 •

edited

Loading

nicolas-grekas commented Aug 17, 2018

stof Aug 17, 2018

stof Aug 17, 2018

nicolas-grekas Aug 17, 2018

Koc Aug 24, 2018

nicolas-grekas Aug 24, 2018 •

edited

Loading

nicolas-grekas commented Aug 30, 2018

fabpot Aug 30, 2018

fabpot Aug 30, 2018

nicolas-grekas Aug 31, 2018

nicolas-grekas commented Aug 31, 2018

stof Sep 3, 2018

nicolas-grekas Sep 3, 2018

fabpot commented Sep 3, 2018

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

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

Conversation

nicolas-grekas commented Aug 16, 2018 • edited Loading

stof left a 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

nicolas-grekas Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

stof commented Aug 16, 2018

nicolas-grekas commented Aug 16, 2018 • edited Loading

stof commented Aug 16, 2018

nicolas-grekas commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Aug 17, 2018

nicolas-grekas commented Aug 17, 2018

stof commented Aug 17, 2018

nicolas-grekas commented Aug 17, 2018 • edited Loading

nicolas-grekas commented Aug 17, 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

nicolas-grekas Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Aug 30, 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 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Sep 3, 2018

nicolas-grekas commented Aug 16, 2018 •

edited

Loading

nicolas-grekas Aug 16, 2018 •

edited

Loading

nicolas-grekas Aug 16, 2018 •

edited

Loading

nicolas-grekas commented Aug 16, 2018 •

edited

Loading

nicolas-grekas commented Aug 17, 2018 •

edited

Loading

nicolas-grekas Aug 24, 2018 •

edited

Loading