Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle][Translator] Make the Translator works with any PSR-11 container#22010
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
1f0acaa to645cf66Compare| * @throws InvalidArgumentException | ||
| */ | ||
| publicfunction__construct(ContainerInterface$container,MessageSelector$selector,$loaderIds =array(),array$options =array()) | ||
| publicfunction__construct(ContainerInterface$container,MessageSelector$selector,$defaultLocale =null,$loaderIds =array(),array$options =array()) |
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.
missing "array" type hint for $loaderIds
| $loaderIds =$defaultLocale; | ||
| if (!$containerinstanceof SymfonyContainerInterface) { | ||
| thrownew \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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd suggest moving this before the trigger_error, and making the message simpler:Missing third $defaultLocale argument.
5bec128 tobd666f8Comparechalasr commentedMar 16, 2017
@nicolas-grekas comments addressed |
7d881cc to94073dfCompare
nicolas-grekas left a comment
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.
👍
| <argumenttype="service-locator" /><!-- translation loaders--> | ||
| <argumenttype="service"id="translator.selector" /> | ||
| <argumenttype="collection" /><!-- translation loaders--> | ||
| <argument>%kernel.default_locale%</argument> |
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.
Why not pass the default locale as an option instead?
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.
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 toc5d9781Comparechalasr commentedMar 16, 2017
|
nicolas-grekas commentedMar 17, 2017
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? |
fabpot commentedMar 17, 2017
My point was just that we already have an |
a8f4c75 toef8c191Comparechalasr commentedMar 17, 2017
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 to58e3db1Comparechalasr commentedMar 18, 2017
rebased |
| ; | ||
| $translator =newTranslator($container,newMessageSelector()); | ||
| $container =$this->getMockBuilder('Psr\Container\ContainerInterface')->getMock(); |
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.
Psr\Container\ContainerInterface::class ?
aead0cd tof9f7f3fComparechalasr commentedMar 22, 2017
Tests green, ready. |
fabpot commentedMar 22, 2017
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/aUses 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/aCommits-------16a2fcf Remove deprecated container injections and compiler passes
Uh oh!
There was an error while loading.Please reload this page.
Uses a service-locator for collected translation loaders and replace the single call of
getParameter()by an optional constructor argument.