Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[DependencyInjection] Support autowiring for EventDispatcher/EventDispatcherInterface#20260

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

Merged

Conversation

@chalasr
Copy link
Member

@chalasrchalasr commentedOct 20, 2016•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsn/a
LicenseMIT
Doc PRn/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, theTraceableEventDispatcher will be injected in dev and theContainerAwareEventDispatcher 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.

hason reacted with hooray emoji
@chalasrchalasr changed the base branch frommaster to2.8October 20, 2016 21:17
@chalasrchalasrforce-pushed theeventdispatcher_autowiring_support branch from89de71d to5a47ec7CompareOctober 20, 2016 21:18
@chalasr
Copy link
MemberAuthor

TheTraceableEventDispatcher (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
Copy link
Contributor

ogizanagi commentedOct 21, 2016•
edited
Loading

TheTraceableEventDispatcher is not registered in the container by using thedecorates feature of the DIC, but instead by doing this weird thing in theFrameworkExtension.

If we want to make this PR work properly with the event dispatcher in debug mode, we should transfer theautowiring_type to thedebug.event_dispatcher service definition...
Or maybe we should remove the code from theFrameworkExtension and use thedecorates 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, hason, and dunglas reacted with thumbs up emoji

@chalasrchalasr changed the base branch from2.8 tomasterOctober 21, 2016 11:19
@chalasrchalasrforce-pushed theeventdispatcher_autowiring_support branch 3 times, most recently from29dbb87 tocd003a0CompareOctober 21, 2016 12:04
@chalasr
Copy link
MemberAuthor

chalasr commentedOct 21, 2016•
edited
Loading

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

Thedebug.event_dispatcher.parent service doesn't exist anymore, it is nowdebug.event_dispatcher.inner. AFAIK it should not be a BC break as the service was just created internally to mimic whatdecorates does

However we still need to manually cleanup theevent_dispatcher autowiring typesfrom the extension, otherwise it is always aContainerAwareEventDispatcher (the decorated) which is injected. Maybe theAutowirePass 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.

@chalasrchalasr changed the titleSupport autowiring for EventDispatcher/EventDispatcherInterface[DependencyInjection] Support autowiring for EventDispatcher/EventDispatcherInterfaceOct 21, 2016
@chalasrchalasrforce-pushed theeventdispatcher_autowiring_support branch fromda8fb14 tod7ca505CompareOctober 21, 2016 13:33
@chalasr
Copy link
MemberAuthor

Failing build on travis unrelated (yaml)

@ogizanagi
Copy link
Contributor

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 thought71d502a was supposed to handle this case 😕

@chalasr
Copy link
MemberAuthor

Needs#20267 for removingthis method call.

@chalasrchalasrforce-pushed theeventdispatcher_autowiring_support branch 2 times, most recently from2c0636b to3acbff9CompareOctober 21, 2016 20:20
<argumenttype="service"id="debug.stopwatch" />
<argumenttype="service"id="logger"on-invalid="null" />
<autowiring-type>Symfony\Component\EventDispatcher\EventDispatcherInterface</autowiring-type>
<autowiring-type>Symfony\Component\EventDispatcher\EventDispatcher</autowiring-type>
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This will not be needed with#20267 too

fabpot added a commit that referenced this pull requestOct 23, 2016
…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/aWhen 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
symfony-splitter pushed a commit to symfony/dependency-injection that referenced this pull requestOct 23, 2016
…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/aWhen 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. Seesymfony/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
Copy link
MemberAuthor

Needs branch 2.8 to be merged in master

@chalasrchalasrforce-pushed theeventdispatcher_autowiring_support branch from3acbff9 to3c6c488CompareOctober 25, 2016 09:21
@chalasr
Copy link
MemberAuthor

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

@chalasrchalasrforce-pushed theeventdispatcher_autowiring_support branch from3c6c488 tod2d68ddCompareNovember 5, 2016 11:03
@chalasrchalasrforce-pushed theeventdispatcher_autowiring_support branch 2 times, most recently from7ce69e0 toe25dd2fCompareNovember 5, 2016 11:34
@chalasr
Copy link
MemberAuthor

Tests have been fixed.

@xabbuh
Copy link
Member

👍

Status: Reviewed

@javiereguiluz
Copy link
Member

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

@dunglas
Copy link
Member

👍

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

@chalasrchalasrforce-pushed theeventdispatcher_autowiring_support branch frome25dd2f to5fd4733CompareNovember 16, 2016 18:46
@fabpotfabpot modified the milestones:3.3,3.2Nov 16, 2016
@chalasr
Copy link
MemberAuthor

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

@dunglas
Copy link
Member

dunglas commentedNov 17, 2016•
edited
Loading

@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
Copy link
Member

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
Copy link
MemberAuthor

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

@chalasr
Copy link
MemberAuthor

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

@fabpot
Copy link
Member

Thank you@chalasr.

@fabpotfabpot merged commit5fd4733 intosymfony:masterDec 6, 2016
fabpot added a commit that referenced this pull requestDec 6, 2016
…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@weaverryanIMHO 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
@chalasrchalasr deleted the eventdispatcher_autowiring_support branchDecember 6, 2016 14:47
chalasr pushed a commit to chalasr/symfony that referenced this pull requestDec 8, 2016
…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@weaverryanIMHO 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
chalasr pushed a commit to chalasr/symfony that referenced this pull requestDec 8, 2016
…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@weaverryanIMHO 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
@nicolas-grekasnicolas-grekas modified the milestones:3.x,3.3Mar 24, 2017
@fabpotfabpot mentioned this pull requestMay 1, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

8 participants

@chalasr@ogizanagi@xabbuh@javiereguiluz@dunglas@fabpot@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp