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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DI] move RegisterServiceSubscribersPass before DecoratorServicePass #29582

Merged
merged 1 commit into from Dec 12, 2018

Conversation

Projects
None yet
4 participants
@kbond
Copy link
Contributor

kbond commented Dec 12, 2018

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets #29550
License MIT
Doc PR n/a

From #29550 (comment): The fact that RegisterServiceSubscribersPass runs after DecoratorServicePass makes it impossible to decorate a service implementing ServiceSubscriberInterface if the decorator does not implement it.

@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Dec 12, 2018

We should prevent regressions on this one, at least with a comment or (ideally) with a new test case for PassConfig

@kbond

This comment has been minimized.

Copy link
Contributor

kbond commented Dec 12, 2018

Do you think simply checking the order of the passes returned from PassConfig::getPasses() is sufficient?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 12, 2018

A unit test wouldn't prevent anything IMHO. An integration one might be best!

@nicolas-grekas nicolas-grekas added this to the 4.2 milestone Dec 12, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 12, 2018

That's for 3.4 btw!

@kbond kbond force-pushed the kbond:voter-pass-fix branch from 962293c to 11d4ba9 Dec 12, 2018

@kbond kbond requested review from dunglas and xabbuh as code owners Dec 12, 2018

@kbond kbond changed the base branch from 4.2 to 3.4 Dec 12, 2018

@kbond

This comment has been minimized.

Copy link
Contributor

kbond commented Dec 12, 2018

I switched to 3.4. I'll work on an integration test.

@chalasr chalasr modified the milestones: 4.2, 3.4 Dec 12, 2018

@kbond

This comment has been minimized.

Copy link
Contributor

kbond commented Dec 12, 2018

Can I get some direction on the best place for an integration test for this would be?

@chalasr

This comment has been minimized.

@kbond kbond force-pushed the kbond:voter-pass-fix branch from 11d4ba9 to f30c2d5 Dec 12, 2018

@kbond

This comment has been minimized.

Copy link
Contributor

kbond commented Dec 12, 2018

Wow, looked everywhere but the obvious place - thanks!

Integration test added.

@kbond kbond force-pushed the kbond:voter-pass-fix branch from f30c2d5 to c3271d9 Dec 12, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 12, 2018

Thank you @kbond.

@nicolas-grekas nicolas-grekas merged commit c3271d9 into symfony:3.4 Dec 12, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Dec 12, 2018

bug #29582 [DI] move RegisterServiceSubscribersPass before DecoratorS…
…ervicePass (kbond)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] move RegisterServiceSubscribersPass before DecoratorServicePass

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #29550
| License       | MIT
| Doc PR        | n/a

From #29550 (comment): The fact that `RegisterServiceSubscribersPass` runs after `DecoratorServicePass` makes it impossible to decorate a service implementing `ServiceSubscriberInterface` if the decorator does not implement it.

Commits
-------

c3271d9 [DI] move RegisterServiceSubscribersPass before DecoratorServicePass

This was referenced Jan 6, 2019

@fabpot fabpot referenced this pull request Jan 6, 2019

Merged

Release v4.2.2 #29803

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment