[*Bundle] Add autowiring aliases for common services #22098
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stof
requested changes
Mar 21, 2017
| @@ -7,6 +7,7 @@ | |||
| <services> | |||
| <!-- ResolvedFormTypeFactory --> | |||
| <service id="form.resolved_type_factory" class="Symfony\Component\Form\ResolvedFormTypeFactory" /> | |||
| <service id="Symfony\Component\Form\ResolvedFormTypeFactoryInterface" alias="form.resolved_type_factory" public="false" /> | |||
There was a problem hiding this comment.
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.
dunno, but it's public and has an interface, so does no harm at least
| @@ -80,6 +80,9 @@ | |||
| </service> | |||
|
|
|||
| <service id="router" alias="router.default" /> | |||
| <service id="Symfony\Component\Routing\RouterInterface" alias="router.default" public="false" /> | |||
There was a problem hiding this comment.
the alias should point to router, not to router.default, to inject the right service when overwriting or decorating the router
There was a problem hiding this comment.
updated, I also added aliases for session.storage and session.handler
| @@ -27,6 +28,7 @@ | |||
| <!-- Run after all custom normalizers --> | |||
| <tag name="serializer.normalizer" priority="-1000" /> | |||
| </service> | |||
| <service id="Symfony\Component\Serializer\Normalizer\ObjectNormalizer" alias="serializer.normalizer.object" public="false" /> | |||
There was a problem hiding this comment.
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.
It's probably a better idea to alias Symfony\Component\Serializer\Normalizer\NormalizerInterface and Symfony\Component\Serializer\Normalizer\DenormalizerInterface to the serializer service in case someone want to autowire a normalizer.
|
I agree about encouraging interfaces rather than implementations (especially for the translator and dispatcher, as we have decorator implementations) |
aab1717 to
eb93b71
Compare
dunglas
requested changes
Mar 21, 2017
| * @required | ||
| */ | ||
| protected function getSession(): Session | ||
| protected function getSession(): SessionInterface |
There was a problem hiding this comment.
This change violates the Liskov substitution principle, getFlashBag() is not part of this interface.
There was a problem hiding this comment.
See PR description for why we need to do this.
The addFlash() method now throws a LogicException when needed.
| @@ -14,6 +14,7 @@ | |||
| <argument type="collection" /> | |||
| <argument type="collection" /> | |||
| </service> | |||
| <service id="Symfony\Component\Serializer\SerializerInterface" alias="serializer" public="false" /> | |||
There was a problem hiding this comment.
I would also mark Symfony\Component\Serializer\NormalizerInterface, Symfony\Component\Serializer\DenormalizerInterface, Symfony\Component\Serializer\EncoderInterface and Symfony\Component\Serializer\DecoderInterface as aliases of the serializer.
a1b115a to
b1f1b12
Compare
b1f1b12 to
08c2ee3
Compare
|
Thank you @nicolas-grekas. |
fabpot
added a commit
that referenced
this pull request
Mar 21, 2017
…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
fabpot
added a commit
that referenced
this pull request
Apr 5, 2017
…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
fabpot
added a commit
that referenced
this pull request
Apr 9, 2017
…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
fabpot
added a commit
that referenced
this pull request
Apr 12, 2017
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 #22098 Commits ------- 22e72d0 Fix serializer aliases
Merged
ogizanagi
added a commit
that referenced
this pull request
Jun 16, 2022
…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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.