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

[DI] Decorate the service directly instead of override #30599

Closed
rapisarda opened this issue Mar 19, 2019 · 13 comments · Fixed by #36373
Closed

[DI] Decorate the service directly instead of override #30599

rapisarda opened this issue Mar 19, 2019 · 13 comments · Fixed by #36373
Labels
DependencyInjection Feature Help wanted Issues and PRs which are looking for volunteers to complete them.
Projects

Comments

@rapisarda
Copy link

Description
Add possibility to decorate the service directly instead of override.

Example

override:

services:
    ham_burger:
        class: Burger

    cheese_burger:
        class: Burger

    ham.ham_decorator:
        class: HamDecorator
        decorate: ham_burger

    cheese.cheese_decorator:
        class: CheeseDecorator
        decoration_priority: 2
        decorate: cheese_burger

    ham.cheese_decorator:
        class: CheeseDecorator
        decoration_priority: 1
        decorate: cheese_burger

decorate :

services:
    ham_burger:
        class: Burger
        decorators:
            - HamDecorator

    cheese_burger:
        class: Burger
        decorators:
            - CheeseDecorator
            - HamDecorator
@ro0NL
Copy link
Contributor

ro0NL commented Mar 19, 2019

At first i liked it :) but we introduce a new (less flexible) concept i believe.

In the first case you are free to modify each decorator as needed. In your proposal we lose this possibility. If we make e.g. decorators: [id, ..., idN] we're back to the current situation more or less by referencing existing services (you could configure them with decorate as well).

TLDR; im not sure how much it increases the complexity of service configuration or what traps we create, given it opens multiple paths.

@stof
Copy link
Member

stof commented Mar 19, 2019

your proposal forbids configuring the decorator services. So it is inferior to the current solution.

@rapisarda
Copy link
Author

I don't understand what forbids configuring the decorator services either what is less flexible

services:
    ham_burger:
        class: Burger
        autowire: true
        decorators:
            - HamDecorator

    cheese_burger:
        class: Burger
        decorators:
            - CheeseDecorator
            - HamDecorator
        arguments:
            - 'foo'

  CheeseDecorator:
    arguments: 
      - '@CheeseDecorator.inner'
      - 'cheddar'

  ketchupDecorator:
    decorate: '@ham_burger'

@ro0NL
Copy link
Contributor

ro0NL commented Mar 19, 2019

it shouldnt be required to use class named service IDs

My worry is CheeseDecorator becomes an implicit decorator (you wouldnt know, only if you see the corresponding cheese_burger service).

What if i start adding decorate: here? Hence we increase complexity IMHO.

@rapisarda
Copy link
Author

It's ok, CheeseDecorator definition is ugly

Class named service IDs is mandatory if you want a lot of configuration for the same class and this is probably in case you implement decorator pattern

Thanks for your feedback

@ro0NL
Copy link
Contributor

ro0NL commented Mar 19, 2019

Class named service IDs is mandatory if you want a lot of configuration for the same class and this is probably in case you implement decorator pattern

not sure i understand that, the same class can be used as a decorator N times.

@stof
Copy link
Member

stof commented Mar 19, 2019

and it also requires the service being decorated to know about its decorators, so it is also less flexible as an extension point.

@stof
Copy link
Member

stof commented Mar 19, 2019

Well, in your proposal, things indeed break if 2 services declare HamDecorator as being their decorator as they would refer to the same service, and so HamDecorator.inner would be one of them.

@rapisarda
Copy link
Author

rapisarda commented Mar 20, 2019

What if decorators: [CheeseDecorator] make a decorator service named like "cheese_burger.CheeseDecorator.decorator" which extend CheeseDecorator abstract service ?
And decorators: {CheeseDecorator: fooDecorator} make a decorator service named "fooDecorator" ?

@stof
Copy link
Member

stof commented Mar 20, 2019

Well, the fact that this would have to create other services would make it a nightmare to configure them if there is any need for an explicit configuration.
And it still does not change the fact that having a service knowing about all its decorator at config time is a much less powerful extension point.

@HellPat
Copy link

HellPat commented Apr 24, 2019

Is your proposal just syntactical sugar for smth like that (which is already possible)?

services:
    burger:
        class: Burger
        arguments:
            - !service
                class: Ham
                arguments:
                    - !service
                        class: Cheese

@rapisarda
Copy link
Author

Exactly. In my case i have something like 30 decorator service with something like 15 decorated services

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 10, 2020

This could work and would be pretty neat to me:

services:
    burger:
        stack:
            - SomeClass: ['@stack.next']
            - AnotherClass:
                calls:
                    - setDecorated: ['@stack.next']
            - TheLastClass # string <=> TheLastClass: []

This would be turned into actual decorators by loaders.

@nicolas-grekas nicolas-grekas added the Help wanted Issues and PRs which are looking for volunteers to complete them. label Mar 10, 2020
@nicolas-grekas nicolas-grekas added this to Nicolas' in Help Wanted Mar 13, 2020
@fabpot fabpot closed this as completed Apr 24, 2020
fabpot added a commit that referenced this issue Apr 24, 2020
This PR was merged into the 5.1-dev branch.

Discussion
----------

[DI] add syntax to stack decorators

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix #30599
| License       | MIT
| Doc PR        | -

Declare this:
```yaml
services:
    my_stack_of_decorators:
        stack:
            - class: App\ExternalDecorator
            - class: App\InternalDecorator
            - class: App\DecoratoredClass
```

And get this:
![image](https://user-images.githubusercontent.com/243674/78615803-b8c8e580-7872-11ea-95c2-22cb78f88ca8.png)

The PR is now ready with support for Yaml, XML and the PHP-DSL. It needs #36388, #36392 and #36389 to pass, and relates to #36390 to be DX-friendly.

The new syntax now supports composable stacks - i.e stack you can reuse in the middle of another stack.

RIP middleware, simple decorators FTW :)

From the test cases:
```yaml
services:
    reusable_stack:
        stack:
            - class: stdClass
              properties:
                  label: A
                  inner: '@.inner'
            - class: stdClass
              properties:
                  label: B
                  inner: '@.inner'

    concrete_stack:
        stack:
            - parent: reusable_stack
            - class: stdClass
              properties:
                  label: C
```

This will create a service similar to:
```php
(object) [
    'label' => 'A',
    'inner' => (object) [
        'label' => 'B',
        'inner' => (object) [
             'label' => 'C',
        ]
    ],
];
```

When used together with autowiring, this is enough to declare a stack of decorators:
```yaml
services:
    my_processing_stack:
        stack:
            - App\ExternalDecorator: ~
            - App\InternalDecorator: ~
            - App\TheDecoratedClass: ~
```

See fixtures for the other configuration formats.

See also https://twitter.com/nicolasgrekas/status/1248198573998604288

Todo:
- [x] rebase on top of #36388, #36392 and #36389 once they are merged
- [x] test declaring deeper nested stacks

Commits
-------

98eeeae [DI] add syntax to stack decorators
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DependencyInjection Feature Help wanted Issues and PRs which are looking for volunteers to complete them.
Projects
Help Wanted
Nicolas'
Development

Successfully merging a pull request may close this issue.

7 participants