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
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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[DependencyInjection] added AutoDecorationServicePass #38432

wants to merge 1 commit into from

Conversation

Yondz
Copy link

@Yondz Yondz commented Oct 6, 2020 •
edited
Loading

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR TBD 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 implements DecoratorInterface will be automatically marked as a decorator for the service returned by DecoratorInterface::getDecoratedServiceId()

In addition to DecoratorInterface, a second interface DecorationPriorityAwareInterface 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 in services.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 CacheableSomeService implements SomeInterface
{
  private $decorated;

  public function __construct(SomeInterface $decorated) {
    $this->decorated = $decorated;
  }

  public function doSomething()
  {
     // .... something to do with $this->decorated
  }
}

After

class CacheableSomeService implements SomeInterface, DecoratorInterface
{
  // ...

  public static function getDecoratedServiceId(): string
  {
    return SomeService::class;
  }
}

or

class CacheableSomeService implements SomeInterface, DecoratorInterface, DecorationPriorityAwareInterface
{
  // ...

  public static function getDecoratedServiceId(): string
  {
    return SomeService::class;
  }

  public static function getDecorationPriority(): int
  {
    return 42;
  }
}

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

@Yondz Yondz force-pushed the decorated_interface branch 2 times, most recently from c76bab0 to 4787f9d Compare October 6, 2020 11:22
@Yondz Yondz force-pushed the decorated_interface branch from 4787f9d to 07ebc13 Compare October 6, 2020 11:24
@nicolas-grekas nicolas-grekas added this to the next milestone Oct 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

@Yondz
Copy link
Author

Yondz commented Oct 6, 2020

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 the stack 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 simple decorates: 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
Copy link
Author

Yondz commented Oct 6, 2020

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 , it does look like a wrong design, thanks again for the feedbacks ;)

Alternative thoughts : I was wondering if there could be a way to allow decorates: 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 !

@Yondz Yondz closed this Oct 6, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
DependencyInjection Feature Status: Needs Review
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
3 participants
@Yondz @nicolas-grekas @carsonbot
Morty Proxy This is a proxified and sanitized view of the page, visit original site.