[DependencyInjection] added a simple way to replace a service by keeping a reference to the old one #9003

fabpot · 2013-09-12T10:19:25Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#5920
License	MIT
Doc PR	not yet

TODO:

add a cookbook entry for this new feature
add support in all loaders/dumpers
add unit tests
see if there are use cases in Symfony that would benefit from this new feature
find the best name for this feature

When overriding an existing definition, the old service is lost:

$c->set('foo', 'stdClass');

// this is going to remove the old definition with the new one
// old definition is lost
$c->set('foo', 'stdClass');

Most of the time, that's exactly what you want to do, but sometimes, you might want to decorate the old one instead. In this case, the old service should be kept around to be able to reference it in the new one. This PR implements this new feature:

$c->set('foo', 'stdClass');

// I want to replace foo with a new one, but I need a reference of the old one (foo1.parent here)
$c->set('foo1', 'stdClass')->addArgument('foo1.parent')->setDecoratedService('foo');

Here is what's going on here: the setDecoratedService() method tells the container that the foo1 service should replace the foo service. By convention, the old foo service is going to be renamed foo1.parent, so I can inject it into my new service.

You can change the parent service name if you want to:

$c->set('foo1', 'stdClass')->addArgument('foo1.wooz')->setDecoratedService('foo', 'foo1.wooz');

Also, several service can decorate a single one and everything should work fine:

$c->set('foo', 'stdClass');

$c->set('foo1', 'stdClass')->addArgument('foo1.parent')->setDecoratedService('foo');
$c->set('foo2', 'stdClass')->addArgument('foo2.parent')->setDecoratedService('foo');

Note that this is different from the existing parent definition attribute, where a service inherit/extends another one (as the two services co-exist independently).

In XML, that would work like this:

<service id="foo1" class="stdClass" decorate="foo">
    <argument type="service" id="foo1.parent" />
</service>

stof · 2013-09-12T10:22:41Z

src/Symfony/Component/DependencyInjection/Definition.php

+    public function setSquashedService($id, $renamedId = null)
+    {
+        if ($renamedId && $id == $renamedId) {
+            throw new \LogicException(sprintf('A squashed service must have a unique name ("%s")', $id));


This exception message looks weird to me

stof · 2013-09-12T10:25:24Z

I like this implementation as it does not even require registering our own compiler pass.

fabpot · 2013-09-12T10:27:37Z

Before implemeting the missing features (loaders/dumpers), I'd like to settle on a name for this feature. Is squash the best word to describe what we are doing here? Any other suggestions?

stof · 2013-09-12T10:30:00Z

@fabpot what about decorate="foo" ? I think it would be easier to understand than squash="true"

fabpot · 2013-09-12T10:37:53Z

decorate was on my list as well, I thought that it does not convey the fact that we are actually overriding/replacing the service that we are decorating. But I'm fine with decorate if that's the preferred choice.

stof · 2013-09-12T10:45:10Z

@fabpot squash seems quite hard to understand IMO. another solution could be wrap

igorw · 2013-09-12T10:58:17Z

Have you considered a generic decorator compiler pass? Something like a generic version of this. Could possibly even be extended with priorities.

The basic premise is that adding tags (metadata) is much more generic and requires less adjustments to loaders/dumpers/schemata than adding a new service attribute.

I agree that if the intent of this feature is decoration, then that term should be used.

fabpot · 2013-09-12T11:03:33Z

@iGrow I'm not a big fan of using tags for built-in features. In fact (I might be wrong as I haven't checked), I think we don't have any tags in the component.

stof · 2013-09-12T11:04:48Z

@igorw This is how I suggested to implement it in the original issue, by simply making the 3 ids configurable in my original code
But allowing to define it directly in the service definition is easier than registering a compiler pass each time IMO.

@fabpot we indeed don't have any tag defined in the DI component itself.

Taluu · 2013-09-12T15:20:36Z

Hum, wrap or decorate seems better IMO. But something I don't really get ; you declare a new service, which will kinda redefine the parent (service "foo1" ?), or could it have the same name as the parent (which would then effectively erase the old one, unless we set the squash / decorator / wrapper / whatever on it) ? Or have I got something wrong along the way ?

stof · 2013-09-12T15:54:00Z

@Taluu At the end of this, the foo service will be an alias of your foo1 service, and foo1.parent will be a private service with the original definition of foo, so that you can inject it as an argument in foo1 (which is needed to decorate it).
When using this, foo1 will generally be a private service (you have no reason to access it as `foo1directly but only by asking forfoo``.
If you reuse the id of the parent, it erases it while keeping no info at all about the parent, so you have no way to wrap the parent implementation (it does not exist anymore in the ContainerBuilder).

the goal of this feature is not to replace a service totally (which can already be done currently) but to apply a decorator on it.

wcluijt · 2013-09-13T20:08:54Z

How about mimic or mimicked?

fabpot · 2013-09-14T08:16:31Z

renamed squash to decorate and setSquashedService to setDecoratedService

yjv · 2013-09-16T14:36:23Z

does decorates perhaps better convey the fact that service foo1 is decorating foo?

toaotc · 2013-09-17T13:18:30Z

what about intersect?
because of decorate is used very often (almost 150 times).

yjv · 2013-09-17T16:13:50Z

It seems like all of these suggestions are of something the service definition should do but it seems like all the other attributes (name, class etc) are about describing what the definition is that's why i like say decorates over decorate.

docteurklein · 2013-10-25T09:00:51Z

would it be possible for a (non shared) service to decorate many (non shared) services ?

stof · 2013-10-25T12:47:25Z

@docteurklein I don't understand what you mean when saying "decorate many services" ?

elnur · 2013-10-25T12:57:13Z

What about transclude as the name for it?

This term is used in AngularJS when replacing a view element with something else and getting a copy/reference to the replaced element to be able to insert it inside the replacing element.

docteurklein · 2013-10-25T13:02:13Z

@stof here's an example of the resulting object graph i'd like to have:

$firstVoter = new FirstVoter;
$secondVoter = new SecondVoter;

$registry->addVoter(new LoggerVoter($firstVoter));
$registry->addVoter(new LoggerVoter($secondVoter));

The LoggerVoter decorates $firstVoter and $secondVoter.

I hope what I say is comprehensible :)

stof · 2013-10-25T13:20:19Z

@docteurklein You will need 2 service definitions (one per LoggerVoter) as you have 2 services (the can use the definition inheritance to share common stuff though).

docteurklein · 2013-10-25T13:35:33Z

@stof I implemented the behavior I describe using a compiler pass (trying to make a generic version of @igorw and yours). I know tags are not the preferred solution, I just can't use something else ATM.

https://gist.github.com/docteurklein/7154758

stof · 2013-10-25T13:43:39Z

@docteurklein you are doing the config in the opposite way in your case, which makes it much less useful as a sevrice needs to know who is decorating it (and it limits it to doing it once).

this feature is about making 1 service declare that it will overwirte another service using composition (and so we want to be able to access the previous definition, not just overwriting it totally by reusing the same id). This approach is much more flexible: the I18nRoutingBundle will be able to decorate the core router without requiring changing the core service, and applying both the i18n decorator and another decorator (doing something else) would work without having to make one bundle aware of the other one.
Note that I'm using the example of I18nRoutingBundle because https://github.com/BeSimple/BeSimpleI18nRoutingBundle is one of the bundle in which I implemented such decorator pattern using a compiler pass (not sure if it was the first one or no)

cordoval · 2013-10-25T13:49:13Z

src/Symfony/Component/DependencyInjection/Compiler/DecoratorServicePass.php

+
+            list ($decorated, $renamedId) = $decorated;
+            if (!$renamedId) {
+                $renamedId = $id.'.parent';


should this be .decorator instead of parent?

It is not the decorator. It is the decorated service.

I agree that .parent may not be the best choice when using composition through. .inner may be easier to understand

or .overridden

@cordoval It would be weird to use be_simple.i18n_router.overriden to store the router service being decorated by be_simple.i18n_router. .overridden would be even worse than .parent in term of understanding when looking at the bundle services.

.wrapped then?

renamed to .inner.

…ing a reference to the old one

fabpot · 2014-04-01T06:02:25Z

closing in favor of #10600

…service by keeping a reference to the old one (romainneutron) This PR was merged into the 2.5-dev branch. Discussion ---------- [DependencyInjection] added a simple way to replace a service by keeping a reference to the old one | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #5920 | License | MIT | Doc PR | symfony/symfony-docs#3745 This PR replaces #9003. Here's the todo list: - [x] add a cookbook entry for this new feature - [x] add support in all loaders/dumpers - [x] add unit tests - [x] see if there are use cases in Symfony that would benefit from this new feature - [x] find the best name for this feature I've implemented YAML and XML Loader / Dumper. From what I see, PhpDumper, PhpLoader, IniLoader, GraphvizDumper do not require an update, am I wrong? Commits ------- 140f807 [DependencyInjection] Update dumpers and loaders, add unit tests 1eb1f4d [DependencyInjection] added a simple way to replace a service by keeping a reference to the old one

Bad practice and error prone: also see symfony/symfony#11881 and symfony/symfony#9003

fabpot mentioned this pull request Sep 12, 2013

pattern to extend a service definition and aliasing new definition to the old name #5920

Closed

stof reviewed Sep 12, 2013
View reviewed changes

cordoval reviewed Oct 25, 2013
View reviewed changes

[DependencyInjection] added a simple way to replace a service by keep…

64dcd0a

…ing a reference to the old one

romainneutron mentioned this pull request Mar 31, 2014

[DependencyInjection] added a simple way to replace a service by keeping a reference to the old one #10600

Merged

5 tasks

stof referenced this pull request in romainneutron/symfony Mar 31, 2014

[DependencyInjection] Minor fixes

d727be1

fabpot closed this Apr 1, 2014

fabpot mentioned this pull request Sep 8, 2014

[3.0] [DependencyInjection] Remove the *.class parameters from core #11881

Closed

takeit mentioned this pull request Aug 11, 2016

[RFC] Remove the *.class parameters superdesk/web-publisher#106

Closed

lunetics added a commit to prooph/service-bus-symfony-bundle that referenced this pull request Apr 21, 2017

Remove the .class parameter

8655925

Bad practice and error prone: also see symfony/symfony#11881 and symfony/symfony#9003

lunetics mentioned this pull request Apr 21, 2017

Refactor service bus bundle prooph/service-bus-symfony-bundle#23

Merged

codeliner pushed a commit to prooph/service-bus-symfony-bundle that referenced this pull request Sep 29, 2017

Remove the .class parameter

5cb319a

Bad practice and error prone: also see symfony/symfony#11881 and symfony/symfony#9003

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DependencyInjection] added a simple way to replace a service by keeping a reference to the old one #9003

[DependencyInjection] added a simple way to replace a service by keeping a reference to the old one #9003

fabpot commented Sep 12, 2013

stof Sep 12, 2013

stof commented Sep 12, 2013

fabpot commented Sep 12, 2013

stof commented Sep 12, 2013

fabpot commented Sep 12, 2013

stof commented Sep 12, 2013

igorw commented Sep 12, 2013

fabpot commented Sep 12, 2013

stof commented Sep 12, 2013

Taluu commented Sep 12, 2013

stof commented Sep 12, 2013

wcluijt commented Sep 13, 2013

fabpot commented Sep 14, 2013

yjv commented Sep 16, 2013

toaotc commented Sep 17, 2013

yjv commented Sep 17, 2013

docteurklein commented Oct 25, 2013

stof commented Oct 25, 2013

elnur commented Oct 25, 2013

docteurklein commented Oct 25, 2013

stof commented Oct 25, 2013

docteurklein commented Oct 25, 2013

stof commented Oct 25, 2013

cordoval Oct 25, 2013

stof Oct 25, 2013

cordoval Oct 25, 2013

stof Oct 25, 2013

cordoval Oct 25, 2013

fabpot Dec 12, 2013

fabpot commented Apr 1, 2014

[DependencyInjection] added a simple way to replace a service by keeping a reference to the old one #9003

[DependencyInjection] added a simple way to replace a service by keeping a reference to the old one #9003

Conversation

fabpot commented Sep 12, 2013

stof Sep 12, 2013

Choose a reason for hiding this comment

stof commented Sep 12, 2013

fabpot commented Sep 12, 2013

stof commented Sep 12, 2013

fabpot commented Sep 12, 2013

stof commented Sep 12, 2013

igorw commented Sep 12, 2013

fabpot commented Sep 12, 2013

stof commented Sep 12, 2013

Taluu commented Sep 12, 2013

stof commented Sep 12, 2013

wcluijt commented Sep 13, 2013

fabpot commented Sep 14, 2013

yjv commented Sep 16, 2013

toaotc commented Sep 17, 2013

yjv commented Sep 17, 2013

docteurklein commented Oct 25, 2013

stof commented Oct 25, 2013

elnur commented Oct 25, 2013

docteurklein commented Oct 25, 2013

stof commented Oct 25, 2013

docteurklein commented Oct 25, 2013

stof commented Oct 25, 2013

cordoval Oct 25, 2013

Choose a reason for hiding this comment

stof Oct 25, 2013

Choose a reason for hiding this comment

cordoval Oct 25, 2013

Choose a reason for hiding this comment

stof Oct 25, 2013

Choose a reason for hiding this comment

cordoval Oct 25, 2013

Choose a reason for hiding this comment

fabpot Dec 12, 2013

Choose a reason for hiding this comment

fabpot commented Apr 1, 2014