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

[DependencyInjection] Add`SubscribedService` attribute, deprecate current`ServiceSubscriberTrait` usage#42238

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:5.4fromkbond:service-subscriber-fix

Oct 27, 2021

Merged

[DependencyInjection] Add`SubscribedService` attribute, deprecate current`ServiceSubscriberTrait` usage#42238

nicolas-grekas merged 1 commit intosymfony:5.4fromkbond:service-subscriber-fix

Oct 27, 2021

Conversation

Copy link

Member

kbond commentedJul 23, 2021•
edited
Loading

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	yes
Deprecations?	yes
Tickets	Fix#42217
License	MIT
Doc PR	TODO

This is a fix for#42217 using what I proposed there (addingSubscribedService attribute). The current usage is deprecated in 5.4 only when using PHP 8+. I suggest removing the current usage entirely in Symfony 6.0 - this willfully fix the bug.

Deprecation Layer:

Symfony 5.4 with PHP < 8, everything works as it does currently (no deprecation warnings).
Symfony 5.4 with PHP > 8 and using the trait on a class w/o any methods marked with theSubscribedService attribute, works as it does currently but a deprecation warning is triggered.
Symfony 5.4 with PHP > 8 and using the trait on a class with at least one method marked with theSubscribedService attribute, you've opted-into the new functionality (methods w/o the attribute are ignored).

carsonbot added Status: Needs Review Bug DependencyInjection Deprecation Feature RFCRFC = Request For Comments (proposals about features that you want to be discussed) labels

Jul 23, 2021

carsonbot changed the title~~[RFC][DependencyInjection] Add SubscribedService attribute, deprecate current ServiceSubscriberTrait usage~~[DependencyInjection] Add SubscribedService attribute, deprecate current ServiceSubscriberTrait usage

Jul 23, 2021

kbond force-pushed theservice-subscriber-fix branch from796f455 to9baa209Compare

July 23, 2021 12:54

weaverryan reviewed

Jul 23, 2021

View reviewed changes

Copy link

Member

weaverryan left a comment

Choose a reason for hiding this comment

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

I like this approach - we have decided not to useServiceSubscriberTrait in our site because, when trying to introduce it, one dev spent a long time trying to figure out a sudden autowiring error (it was because it was trying to wire a model class due to an unrelated method on the service that we did not want to be considered for the locator).

stof reviewed

Jul 23, 2021

View reviewed changes

src/Symfony/Contracts/Service/ServiceSubscriberTrait.php OutdatedShow resolvedHide resolved

src/Symfony/Contracts/Service/ServiceSubscriberTrait.php Outdated


		if (!$returnTypeinstanceof \ReflectionNamedType) {
		// todo, what exception to throw?
		thrownew \RuntimeException(sprintf('Cannot use "%s" on methods with a union return type in "%s::%s()".', SubscribedService::class,$method->name,self::class));

Copy link

Member

stofJul 23, 2021

Choose a reason for hiding this comment

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

Why ? the previous loop does not have this restriction.

Copy link

MemberAuthor

kbondJul 23, 2021

Choose a reason for hiding this comment

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

I treated this as a bug. If the return type isService1|Service2, what should be used for the service locator?

Copy link

Member

derrabusOct 20, 2021

Choose a reason for hiding this comment

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

@kbond Does#43479 help to answer that question?

Copy link

MemberAuthor

kbondOct 20, 2021

Choose a reason for hiding this comment

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

Yes :). Is there an example of using union/intersection types withServiceSubscriberInterface?

Copy link

Member

derrabusOct 20, 2021

Choose a reason for hiding this comment

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

The example from the PR was an intersection of two serializer interfaces that would yield theserializer service, e.g.NormalizerInterface&DenormalizerInterface, so let's explore that:

class MyServiceimplements ServiceSubscriberInterface{use ServiceSubscriberTrait;privatefunctiongetSerializer():NormalizerInterface&DenormalizerInterface    {return$this->container->get(__METHOD__);    }}

Copy link

MemberAuthor

kbondOct 20, 2021•
edited
Loading

Choose a reason for hiding this comment

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

Yep, I'll try to make that work. I'm looking for what the array returned fromServiceSubscriberInterface::getSubscribedServices() looks like when using union/intersection.

Copy link

Member

derrabusOct 20, 2021

Choose a reason for hiding this comment

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

I see. We need to come up with a new convention for that, I guess.

Copy link

Member

stof commentedJul 23, 2021

To make the trait usable in 5.4 without WTF, we would need a way to opt-in for the new behavior (here, there is nothing that will stop registering other private methods, with no way to avoid the deprecation warning if they are not meant to be subscribed)

Copy link

Member

nicolas-grekas commentedJul 23, 2021

What about deprecating the trait instead?

Copy link

MemberAuthor

kbond commentedJul 23, 2021

To make the trait usable in 5.4 without WTF, we would need a way to opt-in for the new behavior

What about, if using the attribute on any method, you've opted into the new way and methods without the attribute are ignored?

Copy link

MemberAuthor

kbond commentedJul 23, 2021•
edited
Loading

What about deprecating the trait instead?

And creating a new trait entirely? Suggestions on a new name?SubscribedServiceTrait?

Copy link

Member

stof commentedJul 23, 2021

Well, providing an easy implemention of thegetSubscribedServices method to provide better DX might make sense.
but maybe it should be a new trait entirely instead (and deprecating the existing one)

Copy link

Member

nicolas-grekas commentedJul 23, 2021

I was considering deprecating the trait and not replacing it, but if you think we should keep it, no need to create a new one if we can make a nice deprecation layer.

Copy link

MemberAuthor

kbond commentedJul 23, 2021•
edited
Loading

I was considering deprecating the trait and not replacing it, but if you think we should keep it...

I personally like this feature so I think we should keep it but I understand it's a bit... controversial (and could be easily abused).

If you're ok keeping it, I'll add the nice deprecation layer and finish this PR. Alternatively, if you want to deprecate/remove, I can easily maintain this feature in a 3rd party package.

Copy link

Member

nicolas-grekas commentedJul 23, 2021

I'm fine keeping it now that we have it.

kbond force-pushed theservice-subscriber-fix branch 2 times, most recently from07877ad tofee84f7Compare

July 23, 2021 15:04

Copy link

MemberAuthor

kbond commentedJul 23, 2021

I've updated/added tests and improved the deprecation layer:

Symfony 5.4 with PHP < 8, everything works as it does currently (no deprecation warnings).
Symfony 5.4 with PHP > 8 and using the trait on a class w/o any methods marked with theSubscribedService attribute, works as it does currently but a deprecation warning is triggered.
Symfony 5.4 with PHP > 8 and using the trait on a class with at least one method marked with theSubscribedService attribute, you've opted-into the new functionality (methods w/o the attribute are ignored).

kbond force-pushed theservice-subscriber-fix branch fromfee84f7 to4306aaaCompare

July 23, 2021 15:27

Copy link

MemberAuthor

kbond commentedJul 23, 2021

I could use some direction on fixing the remaining CI errors.

chalasr removed Bug RFCRFC = Request For Comments (proposals about features that you want to be discussed) labels

Jul 26, 2021

OskarStark changed the title~~[DependencyInjection] Add SubscribedService attribute, deprecate current ServiceSubscriberTrait usage~~[DependencyInjection] AddSubscribedService attribute, deprecate currentServiceSubscriberTrait usage

Aug 1, 2021

OskarStark reviewed

Aug 1, 2021

View reviewed changes

src/Symfony/Contracts/Service/ServiceSubscriberTrait.php OutdatedShow resolvedHide resolved

chalasr added this to the5.4 milestone

Aug 6, 2021

kbond force-pushed theservice-subscriber-fix branch 2 times, most recently from54535f1 to1cbc600Compare

September 8, 2021 12:37

Copy link

MemberAuthor

kbond commentedOct 20, 2021

Assuming this is still wanted for 5.4, once merged, I'll work on a PR to drop the deprecation layer in 6.0.

Still need a bit of direction for fixing the tests. I believe it's a dependency issue,symfony/dependency-injection is requiring the wrong version ofsymfony/service-contracts.

nicolas-grekas reviewed

Oct 20, 2021

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

ideally tests on DI should work on all versions of contracts, or be skipped on non-supported ones

src/Symfony/Contracts/Service/ServiceSubscriberTrait.php OutdatedShow resolvedHide resolved

kbond force-pushed theservice-subscriber-fix branch 3 times, most recently from7afa27c to716b275Compare

October 20, 2021 15:14

nicolas-grekas reviewed

Oct 20, 2021

View reviewed changes

src/Symfony/Contracts/Service/Attribute/SubscribedService.php OutdatedShow resolvedHide resolved

kbond force-pushed theservice-subscriber-fix branch from716b275 toa5bb886Compare

October 20, 2021 15:30

Copy link

MemberAuthor

kbond commentedOct 20, 2021

I believe all related tests are now passing. I fixed some pslam issues but don't understand the remaining ones.

nicolas-grekas reviewed

Oct 20, 2021

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

one last non-trivial note :)

src/Symfony/Contracts/Service/Attribute/SubscribedService.php OutdatedShow resolvedHide resolved

src/Symfony/Contracts/Service/ServiceSubscriberTrait.php Outdated

		thrownew \LogicException(sprintf('Cannot use "%s" on methods without a return type in "%s::%s()".', SubscribedService::class,$method->name,self::class));
		}

		$services[$attribute->newInstance()->key ??self::class.'::'.$method->name] ='?'.$returnType->getName();

Copy link

Member

nicolas-grekasOct 20, 2021

Choose a reason for hiding this comment

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

this should likely do$returnType instanceof \ReflectionNamedType ? ->getName() : (string) for the last part.

also, when the attribute is used, I'm not sure about the? prefix. Shouldn't we add it only when the type is nullable? (and remove|null from the string representation of the union)

Copy link

MemberAuthor

kbondOct 20, 2021

Choose a reason for hiding this comment

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

this should likely do $returnType instanceof \ReflectionNamedType ? ->getName() : (string) for the last part.

Like this?

$services[$attribute->newInstance()->key ??self::class.'::'.$method->name] ='?'.($returnTypeinstanceof \ReflectionNamedType ?$returnType->getName() :$returnType);

I just realizedReflectionType::__toString isdeprecated.. Should we be relying on this?

also, when the attribute is used, I'm not sure about the ? prefix. Shouldn't we add it only when the type is nullable? (and remove |null from the string representation of the union)

Good call, I'll make this change. Should I leave the BC layer as is?

Copy link

MemberAuthor

kbondOct 20, 2021

Choose a reason for hiding this comment

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

I've made these changes (let me know if you'd like me to backport this to the BC layer).

I'm not sure how to handle union/intersection types. See#42238 (comment).

src/Symfony/Contracts/Service/ServiceSubscriberTrait.php OutdatedShow resolvedHide resolved

kbond force-pushed theservice-subscriber-fix branch froma5bb886 to2fa213fCompare

October 20, 2021 15:53

nicolas-grekas approved these changes

Oct 27, 2021

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(with minor comments)

src/Symfony/Contracts/Service/ServiceSubscriberTrait.php OutdatedShow resolvedHide resolved

src/Symfony/Contracts/Service/ServiceSubscriberTrait.phpShow resolvedHide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Oct 27, 2021

kbond force-pushed theservice-subscriber-fix branch fromf045e22 to3cef7b7Compare

October 27, 2021 12:48

[DependencyInjection] AddSubscribedService attribute, deprecate cu…

4a7aa8e

…rrent `ServiceSubscriberTrait` usage

nicolas-grekas force-pushed theservice-subscriber-fix branch from3cef7b7 to4a7aa8eCompare

October 27, 2021 13:22

Copy link

Member

nicolas-grekas commentedOct 27, 2021

Thank you@kbond.

nicolas-grekas merged commit2771d6e intosymfony:5.4

Oct 27, 2021

nicolas-grekas mentioned this pull request

Oct 27, 2021

[DependencyInjection] AddSubscribedService attribute, deprecate curr…symfony/symfony-docs#16013

Closed

kbond deleted the service-subscriber-fix branch

October 27, 2021 13:28

Copy link

MemberAuthor

kbond commentedOct 27, 2021

Great, once this makes its way into the 6.0 branch, I'll remove the deprecation layer.

Copy link

Member

nicolas-grekas commentedOct 27, 2021

now merged up to 6.0

kbond mentioned this pull request

Oct 27, 2021

[DependencyInjection] removeServiceSubscriberTrait deprecation layer#43783

Merged

fabpot added a commit that referenced this pull request

Oct 27, 2021

minor#43783[DependencyInjection] removeServiceSubscriberTraitde…

9284964

…precation layer (kbond)This PR was merged into the 6.0 branch.Discussion----------[DependencyInjection] remove `ServiceSubscriberTrait` deprecation layer| Q             | A| ------------- | ---| Branch?       | 6.0| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | Followup to#42238| License       | MIT| Doc PR        | TODOThis removes the deprecation layer we added in#42238.Commits-------4a6226d [DependencyInjection] remove ServiceSubscriberTrait deprecation layer

This was referencedNov 3, 2021

ServiceSubscriberTrait doesn't fully support PHP8 union types#43913

Closed

[DependencyInjection] only allowReflectionNamedType forServiceSubscriberTrait#43922

Merged

This was referencedNov 5, 2021

Release v5.4.0-BETA1#43937

Merged

Release v6.0.0-BETA1#43939

Merged