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

[DI] Service decoration: autowire the inner service#25631

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

Closed
dunglas wants to merge2 commits intosymfony:masterfromdunglas:autowire-decorators

Conversation

@dunglas
Copy link
Member

@dunglasdunglas commentedDec 30, 2017•
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

Try to automatically inject the decorated service.

Before:

services:_defaults:autowire:trueApp\Foo:~App\FooDecorator:decorates:App\Fooarguments:{$decorated: @App\FooDecorator.inner}

After:

services:_defaults:autowire:trueApp\Foo:~App\FooDecorator:decorates:App\Foo

To trigger the autowiring, the following conditions must be met:

  • the decorator is autowired
  • there is only one argument in the constructor of the type of the decorated service

derrabus, linaori, sroze, theofidry, yceruto, jvasseur, welcoMattic, Koc, dkarlovi, andreybolonin, and 4 more reacted with thumbs up emojiToflar reacted with heart emoji

if (null !==$innerIndex) {
// There is more than one argument of the type of the decorated class
return;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this case should result in an exception with helpful feedback for the developer. The developer obviously tried to autowire the decorator, so I think we should tell them why it fails.

Copy link
MemberAuthor

@dunglasdunglasDec 30, 2017•
edited
Loading

Choose a reason for hiding this comment

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

It would be a BC break: it currently tries to autowire these services "the regular way", if we throw an exception, it would not be the case anymore.

sroze and derrabus reacted with thumbs up emoji
@derrabus
Copy link
Member

Good idea. I needed that feature recently.

@linaori
Copy link
Contributor

Oh this is a nice improvement! But what about the following?

there is only one argument in the constructor of the type of the decorated service

Often enough I decorate because of logging or caching, which would still make me configure everything. In your commit it does seem to work though, or am I mistaken?https://github.com/symfony/symfony/pull/25631/files#diff-0cd60484959ffb82f3498fcc8bdcac11R21

@dunglas
Copy link
MemberAuthor

@iltar I mean:

// This is handledclass BarDecoratorimplements BarInterface{publicfunction__construct(LoggerInterface$logger,BarInterface$decorated)    {    }}// This is ignoredclass BarDecoratorimplements BarInterface{publicfunction__construct(BarInterface$decorated,BarInterface$anotherBar)    {    }}

@linaori
Copy link
Contributor

Ah okay, so I just misunderstood your sentence! In that case, big fat 👍 !

}

if (null !==$innerIndex) {
$definition->setArgument($innerIndex,newReference($renamedId));
Copy link
Member

Choose a reason for hiding this comment

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

perhaps use aTypedReference?

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedDec 30, 2017•
edited
Loading

I really like the idea.
About the implementation, it feels like this duplicates a lot of logic.
What about just adding a binding between the decorated name (if it's also a type), and the inner service? That should span just a few lines, and delegate the hard work to ResolveBindingsPass?

@dunglas
Copy link
MemberAuthor

dunglas commentedDec 30, 2017•
edited
Loading

@nicolas-grekas, good idea I'll try that. The only difference I can see is this case:

class BarDecoratorimplements BarInterface{publicfunction__construct(BarInterface$decorated,BarInterface$anotherBar)    {    }}

With the current implementation, it's ignored (and I think it's the right thing to do), with bindings, the decorated service will be injected in both parameters. IMO it's an edge case, if we're fine with that, I'll update the code.

@dunglas
Copy link
MemberAuthor

In fact it's not possible to use bindings because they exact match the type. Here we need to useis_a.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Minor CS comments. This misses checking for@required setters, where other passes deal with it AFAIK. Should maybe be fixed.

usePsr\Log\LoggerInterface;

/**
* @author Kévin Dunglas <dunglas@gmail.com>

Choose a reason for hiding this comment

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

Not sure about this line, that's a fixture :)


privatefunctionautowire(ContainerBuilder$container,Definition$definition,string$renamedId):void
{
if (

Choose a reason for hiding this comment

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

CS wise I think we prefer putting the next line on this one

!$definition->isAutowired() ||
null === ($innerClass =$container->findDefinition($renamedId)->getClass()) ||
!($reflectionClass =$container->getReflectionClass($definition->getClass())) ||
!$reflectionMethod =$reflectionClass->getConstructor()

Choose a reason for hiding this comment

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

$constructor?


$innerIndex =null;
foreach ($reflectionMethod->getParameters()as$index =>$parameter) {
if (

Choose a reason for hiding this comment

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

Same CS comment

@dunglas
Copy link
MemberAuthor

dunglas commentedDec 30, 2017•
edited
Loading

This misses checking for@required setters

Actually I omitted setters willingly. Setter injection is sometime legit, but for a decorator it's more than weird to inject the decorated service this way. I think we should only support constructors (if someone has this specific use case, he can still use manual wiring).

@dunglasdunglasforce-pushed theautowire-decorators branch 2 times, most recently fromaaddb17 to3e06dd4CompareDecember 31, 2017 09:31
@nicolas-grekas
Copy link
Member

I omitted setters willingly

Seems arbitrary to me, and inconsistent, we should support setters everywhere IMHO.

@dunglas
Copy link
MemberAuthor

dunglas commentedDec 31, 2017•
edited
Loading

Ok I'll add support for setters, it cannot hurt if you don't use them :)

@dunglas
Copy link
MemberAuthor

@Tobion this PR changes nothing regarding service decoration. It will work as excepted.

@chalasrchalasr dismissed theirstale reviewFebruary 5, 2018 09:51

unsupported case, not a blocker for me

@dunglas
Copy link
MemberAuthor

@chalasr I added support for renamed id in an easy but a bit hacky way. WDYT?

@chalasr
Copy link
Member

@dunglas Looks good enough to me.
I'm wondering how this behaves with multiple levels of decoration e.g.service1 -> service2 decorates service1 -> service3 decorates service2, which one wins? A test case would be good :)

hason reacted with thumbs up emoji

@dunglas
Copy link
MemberAuthor

@chalasr AFAIK, this PR doesn't change anything to this behavior.

@dunglasdunglasforce-pushed theautowire-decorators branch 2 times, most recently fromead558c toecd14aaCompareFebruary 19, 2018 14:19
@dunglasdunglas added the ❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze" labelMar 12, 2018
@dunglas
Copy link
MemberAuthor

Failures not related, ping @symfony/deciders. It would be great to have it in 4.1.

Copy link
Member

@javiereguiluzjaviereguiluz left a comment

Choose a reason for hiding this comment

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

DX-wise this is fantastic! I love it ... but I left a minor comment.

*
* Used to store the name of the renamed id when using service decoration together with autowiring
*/
public$previousRenamedId;

Choose a reason for hiding this comment

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

The name of this property is confusing to me:previousRenamedId "previous" and "renamed" together is confusing: is this the previous ID before the renaming? Is this the new renamed ID different from the previous one? Is this the first renamed ID and now we are doing the second renaming (so this is the "previous rename"? etc.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I think it's a rest of the previous implementation (whererenamedId was already used). ispreviousrenamedId ok to you?

Copy link
Member

@javiereguiluzjaviereguiluzMar 16, 2018•
edited
Loading

Choose a reason for hiding this comment

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

It depends on what this property really is (sorry, but its PHPdoc is confusing to me too). Here are some alternative proposals:

  • $decoratedServiceOriginalId
  • $decoratedServiceNewId
  • $decoratedServiceId
  • $innerServiceId
  • $decoratingServiceId
  • ...

Copy link
Member

Choose a reason for hiding this comment

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

I agree the name is confusing

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Done (I pickedinnerServiceId). Is it good for you guys?

ogizanagi, javiereguiluz, and chalasr reacted with thumbs up emoji

Choose a reason for hiding this comment

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

Much better. Thanks!

Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@fabpotfabpot 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


* added support for variadics in named arguments
* added PSR-11`ContainerBagInterface` and its`ContainerBag` implementation to access parameters as-a-service
* added support for autowiring to service's decorators
Copy link
Member

Choose a reason for hiding this comment

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

added support for service's decorators autowiring?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

done

/**
* @internal
*
* Used to store the name of the renamed id when using service decoration together with autowiring
Copy link
Member

Choose a reason for hiding this comment

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

renamed -> inner?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

done

@fabpot
Copy link
Member

Thank you@dunglas.

nicolas-grekas added a commit that referenced this pull requestApr 3, 2018
…bbuh)This PR was merged into the 4.1-dev branch.Discussion----------[DependencyInjection] fix expected exception message| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |Updates a test that was introduced in#25631 to match the changes made in#26595.Commits-------042eb4f fix expected exception message
@fabpotfabpot mentioned this pull requestMay 7, 2018
nicolas-grekas added a commit that referenced this pull requestAug 8, 2018
This PR was merged into the 4.1 branch.Discussion----------[DI] Fix autowire inner service| Q             | A| ------------- | ---| Branch?       | 4.1| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#25631| License       | MIT| Doc PR        | -This PR fix multiple levels of decoration. Unfortunately, this [good question](#25631 (comment)) in origin PR has not been heard 🎧 😄.@dunglas@chalasrCommits-------b79d097 [DI] Fix autowire inner service
symfony-splitter pushed a commit to symfony/dependency-injection that referenced this pull requestAug 8, 2018
This PR was merged into the 4.1 branch.Discussion----------[DI] Fix autowire inner service| Q             | A| ------------- | ---| Branch?       | 4.1| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | #25631| License       | MIT| Doc PR        | -This PR fix multiple levels of decoration. Unfortunately, this [good question](symfony/symfony#25631 (comment)) in origin PR has not been heard 🎧 😄.@dunglas@chalasrCommits-------b79d097c2a [DI] Fix autowire inner service
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@javiereguiluzjaviereguiluzjaviereguiluz approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@derrabusderrabusderrabus left review comments

@fabpotfabpotfabpot approved these changes

@chalasrchalasrchalasr approved these changes

+5 more reviewers

@TobionTobionTobion left review comments

@srozesrozesroze left review comments

@keraduskeraduskeradus left review comments

@SimperfitSimperfitSimperfit approved these changes

@ProfessorCoalProfessorCoalProfessorCoal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

DependencyInjectionFeature❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze"Status: Reviewed

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

14 participants

@dunglas@derrabus@linaori@nicolas-grekas@Tobion@chalasr@fabpot@javiereguiluz@sroze@keradus@Simperfit@ProfessorCoal@xabbuh@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp