Skip to content
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

[DependencyInjection] only allow ReflectionNamedType for ServiceSubscriberTrait #43922

Merged
merged 1 commit into from Nov 4, 2021

Conversation

@kbond
Copy link
Contributor

@kbond kbond commented Nov 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 the SubscribedService attribute (once ServiceSubscriberInterface supports this).

@derrabus
Copy link
Member

@derrabus derrabus commented Nov 3, 2021

Can you provide a test that covers your change?

Loading

@kbond
Copy link
Contributor Author

@kbond kbond commented Nov 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...

Loading

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 4, 2021

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

Loading

@stof
Copy link
Member

@stof stof commented Nov 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 of symfony/service-contract (or whatever the exact package name is) in the requirement of the DI component.

Loading

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 4, 2021

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

Loading

@kbond kbond force-pushed the service-subscriber-fix2 branch from 4844ddc to d5feef6 Nov 4, 2021
@kbond
Copy link
Contributor Author

@kbond kbond commented Nov 4, 2021

Ok, test has been moved.

Loading

@kbond kbond force-pushed the service-subscriber-fix2 branch from d5feef6 to d9dcc35 Nov 4, 2021
@nicolas-grekas nicolas-grekas force-pushed the service-subscriber-fix2 branch from d9dcc35 to b616042 Nov 4, 2021
@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 4, 2021

Thank you @kbond.

Loading

@nicolas-grekas nicolas-grekas merged commit 4f1c67a into symfony:4.4 Nov 4, 2021
9 of 10 checks passed
Loading
@kbond kbond deleted the service-subscriber-fix2 branch Nov 4, 2021
@derrabus
Copy link
Member

@derrabus derrabus commented Nov 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?

Loading

@kbond
Copy link
Contributor Author

@kbond kbond commented Nov 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)?

Loading

@derrabus
Copy link
Member

@derrabus derrabus commented Nov 4, 2021

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

That would be awesome. ❤️

Loading

derrabus added a commit that referenced this issue Nov 4, 2021
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
fabpot added a commit that referenced this issue Nov 4, 2021
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
@fabpot fabpot mentioned this pull request Nov 22, 2021
@fabpot fabpot mentioned this pull request Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants