[DependencyInjection] Support autowiring for EventDispatcher/EventDispatcherInterface #20260

chalasr · 2016-10-20T20:57:51Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	n/a

As it is a very common dependency. Currently it gives:

[Symfony\Component\DependencyInjection\Exception\RuntimeException]
Unable to autowire argument of type "Symfony\Component\EventDispatcher\EventDispatcherInterface" for the service "dummy". Multiple services exist for this interface (debug.event_dispatcher, debug.event_dispatcher.parent).

After this, the TraceableEventDispatcher will be injected in dev and the ContainerAwareEventDispatcher in prod, as when injecting @event_dispatcher explicitly.

ping @weaverryan

IMHO this could be treated as a an enhancement for the autowiring feature and be part of 3.2.

chalasr · 2016-10-21T07:28:52Z

The TraceableEventDispatcher (debug.event_dispatcher service) must be the injected one in dev environment as it decorates it. However the current implementation gives always the decorated service (event_dispatcher), not the decorator.

Status: needs work

ogizanagi · 2016-10-21T08:33:01Z

The TraceableEventDispatcher is not registered in the container by using the decorates feature of the DIC, but instead by doing this weird thing in the FrameworkExtension.

If we want to make this PR work properly with the event dispatcher in debug mode, we should transfer the autowiring_type to the debug.event_dispatcher service definition...
Or maybe we should remove the code from the FrameworkExtension and use the decorates feature instead ? It'll allow to handle correctly the autowiring type with decorated event dispatcher instances more straightforwardly. But I don't know if it can cause any BC break...?

chalasr · 2016-10-21T12:17:25Z

@ogizanagi As you suggested, I made @debug.event_dispatcher decorates @event_dispatcher and removed the definition replacement logic from the FrameworkExtension.

The debug.event_dispatcher.parent service doesn't exist anymore, it is now debug.event_dispatcher.inner. AFAIK it should not be a BC break as the service was just created internally to mimic what decorates does

However we still need to manually cleanup theevent_dispatcher autowiring types from the extension, otherwise it is always a ContainerAwareEventDispatcher (the decorated) which is injected. Maybe the AutowirePass has aliases in its map (the decorated @event_dispatcher)?

Anyway, that's really not a big deal (one method call on the definition).
Also I added some tests and changed this to a feature on master.

chalasr · 2016-10-21T15:49:54Z

Failing build on travis unrelated (yaml)

ogizanagi · 2016-10-21T16:18:01Z

However we still need to manually cleanup the event_dispatcher autowiring types from the extension, otherwise it is always a ContainerAwareEventDispatcher (the decorated) which is injected. Maybe the AutowirePass has aliases in its map (the decorated @event_dispatcher)?

Anyway, that's really not a big deal (one method call on the definition).

It's kind of a big deal. 😄 I thought 71d502a was supposed to handle this case 😕

chalasr · 2016-10-21T19:49:19Z

Needs #20267 for removing this method call.

chalasr · 2016-10-23T09:06:00Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/debug.xml

            <argument type="service" id="debug.stopwatch" />
            <argument type="service" id="logger" on-invalid="null" />
+            <autowiring-type>Symfony\Component\EventDispatcher\EventDispatcherInterface</autowiring-type>
+            <autowiring-type>Symfony\Component\EventDispatcher\EventDispatcher</autowiring-type>


This will not be needed with #20267 too

…the autowiring types (chalasr) This PR was merged into the 2.8 branch. Discussion ---------- [DependencyInjection] A decorated service should not keep the autowiring types | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20260 (comment) | License | MIT | Doc PR | n/a When decorating a service which is not abstract and has `autowiring_types`, the decorator should be the one used for autowiring methods of autowired services, so we should explicitly empty them on the decorated definition after merged them into the child. See #20260 (comment) for a use case where we are forced to manually empty the decorated service's `autowiring_types`. Commits ------- 5951378 A decorated service should not keep the autowiring types

…the autowiring types (chalasr) This PR was merged into the 2.8 branch. Discussion ---------- [DependencyInjection] A decorated service should not keep the autowiring types | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/symfony#20260 (comment) | License | MIT | Doc PR | n/a When decorating a service which is not abstract and has `autowiring_types`, the decorator should be the one used for autowiring methods of autowired services, so we should explicitly empty them on the decorated definition after merged them into the child. See symfony/symfony#20260 (comment) for a use case where we are forced to manually empty the decorated service's `autowiring_types`. Commits ------- 5951378 A decorated service should not keep the autowiring types

chalasr · 2016-10-24T08:59:26Z

Needs branch 2.8 to be merged in master

chalasr · 2016-10-25T09:23:22Z

I made the changes after #20267, this is ready.

chalasr · 2016-11-05T11:46:00Z

Tests have been fixed.

xabbuh · 2016-11-05T11:49:03Z

👍

Status: Reviewed

javiereguiluz · 2016-11-07T09:31:42Z

@dunglas could you please review/approve this proposal? Do you think it's ready for 3.2? Thanks!

dunglas · 2016-11-07T09:39:13Z

👍

For me it's ok to merge this PR in 3.2.

CS Fixes

chalasr · 2016-11-17T10:07:04Z

apparently it will finally be for 3.3 as many others, it would be nice to have an explanation.

dunglas · 2016-11-17T10:11:48Z

@chalasr sorry about that, the core team is very busy, reviewing usually takes time. When reviewing is done, @fabpot is the only one to do the tedious work of merging most PRs and preparing stable releases. It can, again, take some time.

fabpot · 2016-11-17T12:52:02Z

End of development happened more than a month and a half ago. Since then, we should not have merged new features or significant changes. We did some for features that were discussion since a very long time, but we had priorities. RC1 is just around the corner, so merging anything substantial at this point is not possible anymore. Sorry about that, but we need to make choices. That's our release process. Date is fixed, features are not.

chalasr · 2016-11-17T12:53:50Z

And I can perfectly understand, thank you for the hint.

chalasr · 2016-12-06T14:11:55Z

I believe this can be merged now that 3.2 is released.

fabpot · 2016-12-06T14:46:47Z

Thank you @chalasr.

@weaverryan

…atcher/EventDispatcherInterface (chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [DependencyInjection] Support autowiring for EventDispatcher/EventDispatcherInterface | Q | A | | --- | --- | | Branch? | master | | Bug fix? | no | | New feature? | yes | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | n/a | | License | MIT | | Doc PR | n/a | As it is a very common dependency. Currently it gives: > [Symfony\Component\DependencyInjection\Exception\RuntimeException] > Unable to autowire argument of type "Symfony\Component\EventDispatcher\EventDispatcherInterface" for the service "dummy". Multiple services exist for this interface (debug.event_dispatcher, debug.event_dispatcher.parent). After this, the `TraceableEventDispatcher` will be injected in dev and the `ContainerAwareEventDispatcher` in prod, as when injecting `@event_dispatcher` explicitly. ping @weaverryan IMHO this could be treated as a an enhancement for the autowiring feature and be part of 3.2. Commits ------- 5fd4733 Support autowiring for EventDispatcher/EventDispatcherInterface

@weaverryan

…entDispatcher/EventDispatcherInterface (chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [DependencyInjection] Support autowiring for EventDispatcher/EventDispatcherInterface | Q | A | | --- | --- | | Branch? | master | | Bug fix? | no | | New feature? | yes | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | n/a | | License | MIT | | Doc PR | n/a | As it is a very common dependency. Currently it gives: > [Symfony\Component\DependencyInjection\Exception\RuntimeException] > Unable to autowire argument of type "Symfony\Component\EventDispatcher\EventDispatcherInterface" for the service "dummy". Multiple services exist for this interface (debug.event_dispatcher, debug.event_dispatcher.parent). After this, the `TraceableEventDispatcher` will be injected in dev and the `ContainerAwareEventDispatcher` in prod, as when injecting `@event_dispatcher` explicitly. ping @weaverryan IMHO this could be treated as a an enhancement for the autowiring feature and be part of 3.2. Commits ------- 5fd4733 Support autowiring for EventDispatcher/EventDispatcherInterface

@weaverryan

…entDispatcher/EventDispatcherInterface (chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [DependencyInjection] Support autowiring for EventDispatcher/EventDispatcherInterface | Q | A | | --- | --- | | Branch? | master | | Bug fix? | no | | New feature? | yes | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | n/a | | License | MIT | | Doc PR | n/a | As it is a very common dependency. Currently it gives: > [Symfony\Component\DependencyInjection\Exception\RuntimeException] > Unable to autowire argument of type "Symfony\Component\EventDispatcher\EventDispatcherInterface" for the service "dummy". Multiple services exist for this interface (debug.event_dispatcher, debug.event_dispatcher.parent). After this, the `TraceableEventDispatcher` will be injected in dev and the `ContainerAwareEventDispatcher` in prod, as when injecting `@event_dispatcher` explicitly. ping @weaverryan IMHO this could be treated as a an enhancement for the autowiring feature and be part of 3.2. Commits ------- 5fd4733 Support autowiring for EventDispatcher/EventDispatcherInterface

carsonbot added the Status: Needs Review label Oct 20, 2016

chalasr changed the base branch from master to 2.8 October 20, 2016 21:17

chalasr force-pushed the eventdispatcher_autowiring_support branch from 89de71d to 5a47ec7 Compare October 20, 2016 21:18

xabbuh added the DependencyInjection label Oct 21, 2016

carsonbot added Status: Needs Work and removed Status: Needs Review labels Oct 21, 2016

chalasr changed the base branch from 2.8 to master October 21, 2016 11:19

chalasr force-pushed the eventdispatcher_autowiring_support branch 3 times, most recently from 29dbb87 to cd003a0 Compare October 21, 2016 12:04

chalasr changed the title ~~Support autowiring for EventDispatcher/EventDispatcherInterface~~ [DependencyInjection] Support autowiring for EventDispatcher/EventDispatcherInterface Oct 21, 2016

carsonbot added Status: Needs Review and removed Status: Needs Work labels Oct 21, 2016

chalasr force-pushed the eventdispatcher_autowiring_support branch from da8fb14 to d7ca505 Compare October 21, 2016 13:33

chalasr mentioned this pull request Oct 21, 2016

[DependencyInjection] A decorated service should not keep the autowiring types #20267

Merged

chalasr force-pushed the eventdispatcher_autowiring_support branch 2 times, most recently from 2c0636b to 3acbff9 Compare October 21, 2016 20:20

chalasr commented Oct 23, 2016

View reviewed changes

chalasr force-pushed the eventdispatcher_autowiring_support branch from 3acbff9 to 3c6c488 Compare October 25, 2016 09:21

chalasr force-pushed the eventdispatcher_autowiring_support branch from 3c6c488 to d2d68dd Compare November 5, 2016 11:03

chalasr force-pushed the eventdispatcher_autowiring_support branch 2 times, most recently from 7ce69e0 to e25dd2f Compare November 5, 2016 11:34

carsonbot added Status: Reviewed and removed Status: Needs Review labels Nov 5, 2016

javiereguiluz added the Feature label Nov 7, 2016

javiereguiluz added this to the 3.2 milestone Nov 7, 2016

Support autowiring for EventDispatcher/EventDispatcherInterface

5fd4733

CS Fixes

chalasr force-pushed the eventdispatcher_autowiring_support branch from e25dd2f to 5fd4733 Compare November 16, 2016 18:46

fabpot modified the milestones: 3.3, 3.2 Nov 16, 2016

fabpot merged commit 5fd4733 into symfony:master Dec 6, 2016

chalasr deleted the eventdispatcher_autowiring_support branch December 6, 2016 14:47

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DependencyInjection] Support autowiring for EventDispatcher/EventDispatcherInterface #20260

[DependencyInjection] Support autowiring for EventDispatcher/EventDispatcherInterface #20260

chalasr commented Oct 20, 2016 •

edited

Loading

chalasr commented Oct 21, 2016

ogizanagi commented Oct 21, 2016 •

edited

Loading

chalasr commented Oct 21, 2016 •

edited

Loading

chalasr commented Oct 21, 2016

ogizanagi commented Oct 21, 2016

chalasr commented Oct 21, 2016

chalasr Oct 23, 2016

chalasr commented Oct 24, 2016

chalasr commented Oct 25, 2016

chalasr commented Nov 5, 2016

xabbuh commented Nov 5, 2016

javiereguiluz commented Nov 7, 2016

dunglas commented Nov 7, 2016

chalasr commented Nov 17, 2016

dunglas commented Nov 17, 2016 •

edited

Loading

fabpot commented Nov 17, 2016

chalasr commented Nov 17, 2016

chalasr commented Dec 6, 2016

fabpot commented Dec 6, 2016

[DependencyInjection] Support autowiring for EventDispatcher/EventDispatcherInterface #20260

[DependencyInjection] Support autowiring for EventDispatcher/EventDispatcherInterface #20260

Conversation

chalasr commented Oct 20, 2016 • edited Loading

chalasr commented Oct 21, 2016

ogizanagi commented Oct 21, 2016 • edited Loading

chalasr commented Oct 21, 2016 • edited Loading

chalasr commented Oct 21, 2016

ogizanagi commented Oct 21, 2016

chalasr commented Oct 21, 2016

chalasr Oct 23, 2016

Choose a reason for hiding this comment

chalasr commented Oct 24, 2016

chalasr commented Oct 25, 2016

chalasr commented Nov 5, 2016

xabbuh commented Nov 5, 2016

javiereguiluz commented Nov 7, 2016

dunglas commented Nov 7, 2016

chalasr commented Nov 17, 2016

dunglas commented Nov 17, 2016 • edited Loading

fabpot commented Nov 17, 2016

chalasr commented Nov 17, 2016

chalasr commented Dec 6, 2016

fabpot commented Dec 6, 2016

chalasr commented Oct 20, 2016 •

edited

Loading

ogizanagi commented Oct 21, 2016 •

edited

Loading

chalasr commented Oct 21, 2016 •

edited

Loading

dunglas commented Nov 17, 2016 •

edited

Loading