Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[*Bundle] Add autowiring aliases for common services#22098
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
| <services> | ||
| <!-- ResolvedFormTypeFactory--> | ||
| <serviceid="form.resolved_type_factory"class="Symfony\Component\Form\ResolvedFormTypeFactory" /> | ||
| <serviceid="Symfony\Component\Form\ResolvedFormTypeFactoryInterface"alias="form.resolved_type_factory"public="false" /> |
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.
Is it actually needed ? I never had a need to inject it directly. This service should probably only be used by the form registry and the form factory, not by userland code
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.
dunno, but it's public and has an interface, so does no harm at least
| </service> | ||
| <serviceid="router"alias="router.default" /> | ||
| <serviceid="Symfony\Component\Routing\RouterInterface"alias="router.default"public="false" /> |
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 alias should point torouter, not torouter.default, to inject the right service when overwriting or decorating the router
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.
updated, I also added aliases for session.storage and session.handler
| <!-- Run after all custom normalizers--> | ||
| <tagname="serializer.normalizer"priority="-1000" /> | ||
| </service> | ||
| <serviceid="Symfony\Component\Serializer\Normalizer\ObjectNormalizer"alias="serializer.normalizer.object"public="false" /> |
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.
is it necessary ?
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.
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'm not sure that there is a good use case to inject this normalizer using autowiring. I would not recommend to do it.
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.
It's probably a better idea to aliasSymfony\Component\Serializer\Normalizer\NormalizerInterface andSymfony\Component\Serializer\Normalizer\DenormalizerInterface to theserializer service in case someone want to autowire a normalizer.
stof commentedMar 21, 2017
I agree about encouraging interfaces rather than implementations (especially for the translator and dispatcher, as we have decorator implementations) |
| * @required | ||
| */ | ||
| protectedfunctiongetSession():Session | ||
| protectedfunctiongetSession():SessionInterface |
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.
This change violates the Liskov substitution principle,getFlashBag() is not part of this interface.
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.
See PR description for why we need to do this.
The addFlash() method now throws a LogicException when needed.
| <argumenttype="collection" /> | ||
| <argumenttype="collection" /> | ||
| </service> | ||
| <serviceid="Symfony\Component\Serializer\SerializerInterface"alias="serializer"public="false" /> |
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 would also markSymfony\Component\Serializer\NormalizerInterface,Symfony\Component\Serializer\DenormalizerInterface,Symfony\Component\Serializer\EncoderInterface andSymfony\Component\Serializer\DecoderInterface as aliases of the serializer.
a1b115a tob1f1b12Compare
dunglas 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.
👍
fabpot commentedMar 21, 2017
Thank you@nicolas-grekas. |
…nicolas-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[*Bundle] Add autowiring aliases for common services| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -As spotted while working on#22060, we're missing many aliases to prevent any autowiring ambiguities.I also removed the "Symfony\Component\EventDispatcher\EventDispatcher" and "Symfony\Component\DependencyInjection\Container" aliases: we'd better encourage using the corresponding interfaces instead.On ControllerTrait, we need to type hint against SessionInterface, because otherwise, when session support is disabled, autowiring still auto-registers an "autowired.Session" service, which defeats the purpose of being able to enable/disable it.Commits-------08c2ee3 [*Bundle] Add autowiring aliases for common services
…g reflection against all existing services (nicolas-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[BC BREAK][DI] Always autowire "by id" instead of using reflection against all existing services| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | no| New feature? | yes| BC breaks? | yes - compile time only| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -(patch best reviewed [ignoring whitespaces](https://github.com/symfony/symfony/pull/22295/files?w=1).)"By-id" autowiring, as introduced in#22060 is free from all the issues that "by-type" autowiring has:- it has no magic and requires explicit type<>id matching (*vs* using reflection on all services to cherry-pick *the* one that matches some type-hint *at that time*, which is fragile)- it is free from any ambiguities (*vs* the Damocles' sword of breaking config just by enabling some unrelated bundle)- it is easily introspected: just look at DI config files (*vs* inspecting the type-hierarchy of all services + their type-hints)- ~~it is side-effect free, thus plain predictable (*vs* auto-registration of discovered types as services)~~- it plays nice with deprecated services or classes (see#22282)- *etc.*Another consideration is that any "by-type" autowired configuration is either broken (because of future ambiguities) - or equivalent to a "by-id" configuration (because resolving ambiguities *means* adding explicit type<>id mappings.) For theoreticians, we could say that "by-id" autowiring is the asymptotic limit of "by-type" autowiring :-)For all these reasons - and also because it reduces the complexity of the code base - I propose to change the behavior and only support "by-id" autowiring in 3.3. This will break some configurations relying on "by-type" autowiring. Yet the break will only happen at compile-time, which means this won't *silently* break any apps. For all core Symfony services, they will work out of the box thanks to#22098 *et al.* For the remaining services, fixing ones config should be pretty straightforward: just follow the suggestions provided by the exception messages. If they are fine to you, you'll end up with the exact same config in the end. And maybe you'll spot issues that were hidden previously.Commits-------cc5e582 [BC BREAK][DI] Always autowire "by id" instead of using reflection against all existing services
…Scott)This PR was merged into the 3.3-dev branch.Discussion----------[FrameworkBundle] Add autowiring alias for Stopwatch| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Following in the footsteps of#22098 to add aliases for various services, this adds an alias for `Symfony\Component\Stopwatch\Stopwatch`.Commits-------707f74b [FrameworkBundle] Add autowiring alias for Stopwatch
This PR was merged into the 3.3-dev branch.Discussion----------[FrameworkBundle] Fix serializer aliases| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| License | MIT| Doc PR |Fix serializer aliases introduced in#22098Commits-------22e72d0 Fix serializer aliases
…concrete normalizers (chalasr)This PR was merged into the 6.2 branch.Discussion----------[Serializer] Deprecate autowiring aliases pointing to concrete normalizers| Q | A| ------------- | ---| Branch? | 6.2| Bug fix? | no| New feature? | no| Deprecations? | yes| Tickets | -| License | MIT| Doc PR | -They should've never been added. See#22098 (comment)Commits-------c1680be [Serializer] Deprecate autowiring aliases pointing to concrete normalizers
Uh oh!
There was an error while loading.Please reload this page.
As spotted while working on#22060, we're missing many aliases to prevent any autowiring ambiguities.
I also removed the "Symfony\Component\EventDispatcher\EventDispatcher" and "Symfony\Component\DependencyInjection\Container" aliases: we'd better encourage using the corresponding interfaces instead.
On ControllerTrait, we need to type hint against SessionInterface, because otherwise, when session support is disabled, autowiring still auto-registers an "autowired.Session" service, which defeats the purpose of being able to enable/disable it.