[FrameworkBundle][Translator] Make the Translator works with any PSR-11 container #22010
Conversation
e8fc714 to
ff9b631
Compare
1f0acaa to
645cf66
Compare
| @@ -54,8 +55,21 @@ class Translator extends BaseTranslator implements WarmableInterface | |||
| * | |||
| * @throws InvalidArgumentException | |||
| */ | |||
| public function __construct(ContainerInterface $container, MessageSelector $selector, $loaderIds = array(), array $options = array()) | |||
| public function __construct(ContainerInterface $container, MessageSelector $selector, $defaultLocale = null, $loaderIds = array(), array $options = array()) | |||
There was a problem hiding this comment.
missing "array" type hint for $loaderIds
| $loaderIds = $defaultLocale; | ||
|
|
||
| if (!$container instanceof SymfonyContainerInterface) { | ||
| throw new \InvalidArgumentException(sprintf('A default locale must be passed as 3rd argument of %s() since version 3.3, the argument will be mandatory in 4.0.', __METHOD__)); |
There was a problem hiding this comment.
I'd suggest moving this before the trigger_error, and making the message simpler:
Missing third $defaultLocale argument.
5bec128 to
bd666f8
Compare
|
@nicolas-grekas comments addressed |
7d881cc to
94073df
Compare
| <argument type="service" id="translator.selector" /> | ||
| <argument type="collection" /> <!-- translation loaders --> | ||
| <argument>%kernel.default_locale%</argument> |
There was a problem hiding this comment.
Why not pass the default locale as an option instead?
There was a problem hiding this comment.
The argument exists in the parent constructor with getter/setter (unlike other options), I find consistent to have it here. An option would be fine too
7faf677 to
c5d9781
Compare
|
|
|
Not sure about the change: the option is not optional (it's required ;) ), which means the 4th arg is now mandatory also, but the signature tells the contrary. WDYT @fabpot ? Was that what you had in mind? |
|
My point was just that we already have an |
a8f4c75 to
ef8c191
Compare
|
Given it would be the only mandatory option and we would have to provide an equivalent BC layer in both cases, I changed it back to an argument. |
240639a to
58e3db1
Compare
|
rebased |
58e3db1 to
fc2d3b8
Compare
| ; | ||
|
|
||
| $translator = new Translator($container, new MessageSelector()); | ||
| $container = $this->getMockBuilder('Psr\Container\ContainerInterface')->getMock(); |
There was a problem hiding this comment.
Psr\Container\ContainerInterface::class ?
aead0cd to
f9f7f3f
Compare
f9f7f3f to
85177a6
Compare
|
Tests green, ready. |
|
Thank you @chalasr. |
…s with any PSR-11 container (chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [FrameworkBundle][Translator] Make the Translator works with any PSR-11 container | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Uses a service-locator for collected translation loaders and replace the single call of `getParameter()` by an optional constructor argument. Commits ------- 85177a6 [FrameworkBundle] Make Translator works with any PSR-11 container
…sses (chalasr) This PR was merged into the 4.0-dev branch. Discussion ---------- Remove deprecated container injections and compiler passes | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~~#21451 #21625 #21284 #22010~~ #22805 | License | MIT | Doc PR | n/a Commits ------- 16a2fcf Remove deprecated container injections and compiler passes
Uses a service-locator for collected translation loaders and replace the single call of
getParameter()by an optional constructor argument.