NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[DependencyInjection] only allow`ReflectionNamedType` for`ServiceSubscriberTrait`#43922

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

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:4.4fromkbond:service-subscriber-fix2

Nov 4, 2021

Merged

[DependencyInjection] only allow`ReflectionNamedType` for`ServiceSubscriberTrait`#43922

nicolas-grekas merged 1 commit intosymfony:4.4fromkbond:service-subscriber-fix2

Nov 4, 2021

Conversation

Copy link

Member

kbond commentedNov 3, 2021

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix#43913
License	MIT
Doc PR	n/a

I'll follow this up with a PR on 5.4 to allow union/intersections when using theSubscribedService attribute (onceServiceSubscriberInterfacesupports this).

carsonbot added Status: Needs Review Bug DependencyInjection labels

Nov 3, 2021

carsonbot added this to the4.4 milestone

Nov 3, 2021

Copy link

Member

derrabus commentedNov 3, 2021

Can you provide a test that covers your change?

Copy link

MemberAuthor

kbond commentedNov 3, 2021

Test added. Regarding the failing CI: when I had this issue in#42238,this was how I solved - not sure what to do here...

kbond mentioned this pull request

Nov 3, 2021

ServiceSubscriberTrait doesn't fully support PHP8 union types#43913

Closed

Copy link

Member

nicolas-grekas commentedNov 4, 2021

The test case should be moved tosrc/Symfony/Contracts/Tests/Service/ServiceSubscriberTraitTest.php, that would prevent cross-packages issues.

Copy link

Member

stof commentedNov 4, 2021

@nicolas-grekas given that the test relies on the DI component, this would be wrong.

I think that the right fix is to bump the min version ofsymfony/service-contract (or whatever the exact package name is) in the requirement of the DI component.

Copy link

Member

nicolas-grekas commentedNov 4, 2021

The test should be updated to not rely on the DI component. There is no need for this dep.

kbond force-pushed theservice-subscriber-fix2 branch from4844ddc tod5feef6Compare

November 4, 2021 12:34

Copy link

MemberAuthor

kbond commentedNov 4, 2021

Ok, test has been moved.

kbond force-pushed theservice-subscriber-fix2 branch fromd5feef6 tod9dcc35Compare

November 4, 2021 12:39

[DependencyInjection] only allowReflectionNamedType for `ServiceSu…

b616042

…bscriberTrait`

nicolas-grekas force-pushed theservice-subscriber-fix2 branch fromd9dcc35 tob616042Compare

November 4, 2021 13:32

Copy link

Member

nicolas-grekas commentedNov 4, 2021

Thank you@kbond.

nicolas-grekas merged commit4f1c67a intosymfony:4.4

Nov 4, 2021

kbond deleted the service-subscriber-fix2 branch

November 4, 2021 13:35

Copy link

Member

derrabus commentedNov 4, 2021

@kbond I think I might have messed up the merging this change up to 5.4 and 6.0. Can you please have a look?

Copy link

MemberAuthor

kbond commentedNov 4, 2021

@derrabus, 5.4 just needs a test marked as legacy. 6.0 needs some work to re-remove the deprecation layer. Do you want me to open PR(s)?

Copy link

Member

derrabus commentedNov 4, 2021•
edited
Loading

@derrabus Do you want me to open PR(s)?

That would be awesome. ❤️

kbond mentioned this pull request

Nov 4, 2021

[DependencyInjection] mark test as legacy#43933

Merged

derrabus added a commit that referenced this pull request

Nov 4, 2021

minor#43933[DependencyInjection] mark test as legacy (kbond)

4bcd0a4

This PR was merged into the 5.4 branch.Discussion----------[DependencyInjection] mark test as legacy| Q             | A| ------------- | ---| Branch?       | 5.4| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | -Ref:#43922 (comment)`@derrabus`, once this makes it into 6.0 give me a ping and I'll open a PR to fix the 6.0 issues.Commits-------0b6cfcb [DependencyInjection] mark test as legacy

kbond mentioned this pull request

Nov 4, 2021

[DependencyInjection] clean up merge issue#43934

Merged

fabpot added a commit that referenced this pull request

Nov 4, 2021

minor#43934[DependencyInjection] clean up merge issue (kbond)

687b179

This PR was merged into the 6.0 branch.Discussion----------[DependencyInjection] clean up merge issue| Q             | A| ------------- | ---| Branch?       | 6.0| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | -Cleans up merge issue from#43922.Commits-------ed30e4c [DependencyInjection] remove deprecation layer

This was referencedNov 22, 2021

Release v4.4.34#44193

Merged

Release v5.3.11#44194

Merged