[Contracts] Add Translation\TranslatorInterface + decouple symfony/validator from symfony/translation #28210
Conversation
6d69cf9 to
e850ffa
Compare
There was a problem hiding this comment.
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)
| @@ -295,7 +300,7 @@ public function getLoaders() | |||
| /** | |||
| * {@inheritdoc} | |||
| */ | |||
| public function getValidator() | |||
| public function getValidator(/*TranslatorInterface $translator = null*/) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I get that. That's the best I could think of for now. Any better suggestion?
There was a problem hiding this comment.
IMO, it should stay on the setter, like today
There was a problem hiding this comment.
I'd like to. How could we deal with BC then?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
new TranslatorInterface extend the old one
done, and made ValidatorBuilder final + deprecated its interface
|
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 |
Note that if we really want an interoperable interface, the parsing of choices needs to be in Contracts. That's why There is another extension point that this plans to remove: |
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. |
1f4367e to
d1063a2
Compare
|
TODO list completed and tests green. Ready for final review. |
| @@ -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", | |||
There was a problem hiding this comment.
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?
UPGRADE-4.2.md
Outdated
| Translation | ||
| ----------- | ||
|
|
||
| * The `TranslationInterface` has been deprecated in favor of `Symfony\Contracts\Translation\TranslationInterface` |
There was a problem hiding this comment.
TranslatorInterface (same below)
|
should we actually use |
It works because this uses
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 What worries me more is the |
yeah, we might end up with this option (and deprecating the interface and marking the class as |
|
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 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 |
00ea207 to
d1bea0d
Compare
|
It took me a while, but I finally figured out a way to move forward without breaking BC. PR green. |
| { | ||
| $this->selector = $selector ?: new MessageSelector(); | ||
| $this->translator = $translator instanceof MessageSelector ? new IdentityTranslator($translator) : $translator ?? new IdentityTranslator(); |
There was a problem hiding this comment.
we should throw an exception if we receive something else than TranslatorInterface|MessageSelector|null, as we do in other places having such BC layer
There was a problem hiding this comment.
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
d4ab6f7 to
6a9762c
Compare
| * | ||
| * @throws InvalidArgumentExceptionInterface If the locale contains invalid characters | ||
| */ | ||
| public function transChoice($id, $number, array $parameters = array(), $domain = null, $locale = null); |
There was a problem hiding this comment.
Do we need transChoice in contracts? We already have message formatters and unmerged PR #27399 which allow get rid of transChoice.
There was a problem hiding this comment.
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.
6a9762c to
80ac68c
Compare
|
thanks (just rebased to fix a minor conflict, ready) |
80ac68c to
866b6a9
Compare
| namespace Symfony\Contracts\Translation; | ||
|
|
||
| /** | ||
| * Exception interface for all exceptions thrown by the component. |
There was a problem hiding this comment.
But anyway, why do we need this interface in the first place?
There was a problem hiding this comment.
I removed InvalidArgumentExceptionInterface and updated TranslationInterface with @throws \InvalidArgumentException instead.
866b6a9 to
c24c59f
Compare
|
Comments addressed thanks. |
118669d to
2f94d74
Compare
| @@ -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)) { | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
good catch, fixed thanks
2f94d74 to
d72a85f
Compare
…lidator from symfony/translation
d72a85f to
064e369
Compare
|
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
Let's decouple Validator from Translation component \o/!
TODO:
TranslatorInterface, deprecate it from TranslationTranslatorTrait, deprecatingMessageSelector,InternalandPluralizationRulesValidatorBuilderInterface(LegacyTranslatorInterface)identity_translatorintotranslator.formatter.default, deprecatetranslator.selectorTranslatorTraitbehaves properlyInvalidArgumentExceptionfrom the componentReviews welcome already.