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

[DependencyInjection] added AutoDecorationServicePass#38432

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
Yondz wants to merge1 commit intosymfony:masterfromYondz:decorated_interface

Conversation

Yondz
Copy link

@YondzYondz commentedOct 6, 2020•
edited
Loading

QA
Branch?5.x
Bug fix?no
New feature?yes
Deprecations?no
TicketsN/A
LicenseMIT
Doc PRTBD if feature is accepted

Hi everyone,

Here is a feature proposal for symfony/dependency-injection : implement a configuration-less decoration feature (auto decoration ?).

Every service definition whose class implementsDecoratorInterface will be automatically marked as a decorator for the service returned byDecoratorInterface::getDecoratedServiceId()

In addition toDecoratorInterface, a second interfaceDecorationPriorityAwareInterface is provided to provide the decoration priority.

To explain the "why" part, I encountered a few times already the use-case where it is required to maximize the use of decoration, leading to multiple decorators cascading, and therefore, to multiple lines inservices.yaml. This will allow developers to use "abstract decorators" or traits to share the decoration behaviour upon multiple classes.

The advantage is that it would no longer be required to write decorator definition in configuration files for the majority of decoration use cases.
The limitations of this approach are the same as the one using autowiring, plus, It would add code coupling between service classes and the DI component, which is why this should be considered as an optional feature.

Here is an example

Before

services:App\CacheableSomeService:decorates:App\SomeService
class CacheableSomeServiceimplements SomeInterface{private$decorated;publicfunction__construct(SomeInterface$decorated) {$this->decorated =$decorated;  }publicfunctiondoSomething()  {// .... something to do with $this->decorated  }}

After

class CacheableSomeServiceimplements SomeInterface, DecoratorInterface{// ...publicstaticfunctiongetDecoratedServiceId():string  {return SomeService::class;  }}

or

class CacheableSomeServiceimplements SomeInterface, DecoratorInterface, DecorationPriorityAwareInterface{// ...publicstaticfunctiongetDecoratedServiceId():string  {return SomeService::class;  }publicstaticfunctiongetDecorationPriority():int  {return42;  }}

Looking forward for your remarks/reviews, especially as it is my first time here, I might have done something wrong 🐶

@nicolas-grekasnicolas-grekas added this to thenext milestoneOct 6, 2020
@nicolas-grekas
Copy link
Member

It would add code coupling between service classes and the DI component, which is why this should be considered as an optional feature.

That's a major issue IMHO. I think we should not do this.

We added a new syntax to stack decorators in 5.1, did you try it? See#36373

dunglas reacted with thumbs up emoji

@Yondz
Copy link
Author

It would add code coupling between service classes and the DI component, which is why this should be considered as an optional feature.

That's a major issue IMHO. I think we should not do this.

We added a new syntax to stack decorators in 5.1, did you try it? See#36373

Hi@nicolas-grekas , thanks for the feedback ;) I guess we agree on this point :p

I did not use thestack syntax yet, but took a look at it : while it's a nice improvement that handles a lot of use-cases, I find it not that "readable" compared to the simpledecorates: syntax :s

The thing is that you will always be required to add new elements to the stack, right ? Could there be an option to bind this stack to an interface (which would come from the app, and not the DI component then, to avoid coupling, but in that case, I don't see many options to handle the priority) ?

@nicolas-grekas
Copy link
Member

Could there be an option to bind this stack to an interface

I think that decorators are best configured via explicit configuration. I don't see how an interface can help auto-stack them without leaking DI configuration details (service ids) into actual code.

This looks like a dead-end to me sorry.

Yondz and jvasseur reacted with thumbs up emoji

@Yondz
Copy link
Author

Could there be an option to bind this stack to an interface

I think that decorators are best configured via explicit configuration. I don't see how an interface can help auto-stack them without leaking DI configuration details (service ids) into actual code.

This looks like a dead-end to me sorry.

I did spend some time turning this around and I eventually came to the same conclusion, nothing to be sorry about@nicolas-grekas , itdoes look like a wrong design, thanks again for the feedbacks ;)

Alternative thoughts : I was wondering if there could be a way to allowdecorates: keyword under_instanceof: bindings, allowing then to achieve what I was looking for (except for the priority configuration), without this coupling issue.

Still, the journey inside the contribution process and the components was quite interesting and gave me more confidence, can't wait for future opportunities to try to fix a bug or else ;)

I'll close this one !

nicolas-grekas and xabbuh reacted with thumbs up emoji

@YondzYondz closed thisOct 6, 2020
@nicolas-grekasnicolas-grekas modified the milestones:next,5.2Oct 7, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.

3 participants
@Yondz@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp