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\object()->method() does not support calling same method twice #193

Closed
DizzyC opened this Issue Oct 1, 2014 · 8 comments

Comments

Projects
None yet
3 participants
@DizzyC

DizzyC commented Oct 1, 2014

This doesn't work:

'My\AddNumbers' => DI\object()
        ->method('addNumber', 1)
        ->method('addNumber', 2),

Only ->method('addNumber', 2) is taken into account.

The method ClassDefinitionHelper::method() (code here) uses the method name as a key for the array that holds methods that should be executed on the constructed object but if you try to call the same method twice it will obviously not work as the array key will be overridden, always retaining only the last method call.

@mnapoli mnapoli added bug and removed bug labels Oct 1, 2014

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Oct 1, 2014

Member

Hi! You are completely correct, this was done by design (hence the adding/removal of the "bug" tag).

The reason for this is "definition overriding". There are 3 ways to define injections in PHP-DI: autowiring (type-hints), annotations and array definitions.

The thing is that array definitions are often useful to complete autowiring definitions, for example:

class Foo {
    public function configure($someParam, Logger $logger) {...}
}

Here we only need to define the $someParam parameter because PHP-DI can figure the other one out. So we don't want to configure all the other parameters because if the method changes, then we don't want to have to change too many things in the array configuration.

So that's what you can do:

'Foo' => DI\object()
        ->method('configure', 'Hello world'), // leave the second parameter out

// Or even:
'Foo' => DI\object()
        ->methodParameter('configure', 'someParam', 'Hello world'),

So what happens is that the definitions read from autowiring are merged with the one found in annotations and the one found in array configs. And if it was possible to call several times the same method, there is no way to know which method calls to merge into one, and which to not merge and keep separate.

Anyway, this is the reason, but the problem is still there. What I thought at the time is that if you find any limitation of the array config helpers, you can always use a factory closure and do anything you want.

So for now, use a closure. In the future (version 5), I would like to make the definition merging/overriding more explicit and controllable. So I will be adding this issue to the v5.0 tracker and hopefully it will be solved in the next major.

Member

mnapoli commented Oct 1, 2014

Hi! You are completely correct, this was done by design (hence the adding/removal of the "bug" tag).

The reason for this is "definition overriding". There are 3 ways to define injections in PHP-DI: autowiring (type-hints), annotations and array definitions.

The thing is that array definitions are often useful to complete autowiring definitions, for example:

class Foo {
    public function configure($someParam, Logger $logger) {...}
}

Here we only need to define the $someParam parameter because PHP-DI can figure the other one out. So we don't want to configure all the other parameters because if the method changes, then we don't want to have to change too many things in the array configuration.

So that's what you can do:

'Foo' => DI\object()
        ->method('configure', 'Hello world'), // leave the second parameter out

// Or even:
'Foo' => DI\object()
        ->methodParameter('configure', 'someParam', 'Hello world'),

So what happens is that the definitions read from autowiring are merged with the one found in annotations and the one found in array configs. And if it was possible to call several times the same method, there is no way to know which method calls to merge into one, and which to not merge and keep separate.

Anyway, this is the reason, but the problem is still there. What I thought at the time is that if you find any limitation of the array config helpers, you can always use a factory closure and do anything you want.

So for now, use a closure. In the future (version 5), I would like to make the definition merging/overriding more explicit and controllable. So I will be adding this issue to the v5.0 tracker and hopefully it will be solved in the next major.

@mnapoli mnapoli added the enhancement label Oct 1, 2014

@mnapoli mnapoli added this to the 5.0 milestone Oct 1, 2014

@mnapoli mnapoli changed the title from ClassDefinitionHelper::method() does not support calling same method multiple times to DI\object()->method() does not support calling same method twice Oct 1, 2014

@DHager

This comment has been minimized.

Show comment
Hide comment
@DHager

DHager Mar 18, 2015

Just chiming in: I got surprised by this today. I think it may be worth a note in the documentation, e.g.: "Multiple uses of the same setter are not supported."

I think the reason it was surprising is that the fluent/chained-method-call API suggests there's nothing wrong with using a bunch of them in a big chain.

DHager commented Mar 18, 2015

Just chiming in: I got surprised by this today. I think it may be worth a note in the documentation, e.g.: "Multiple uses of the same setter are not supported."

I think the reason it was surprising is that the fluent/chained-method-call API suggests there's nothing wrong with using a bunch of them in a big chain.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Mar 18, 2015

Member

@DHager thanks for the feedback. Yep I don't really like that situation too. I'm planning to improve that in 5.0 (currently working on it) but it will mean a bit of BC breaks.

Member

mnapoli commented Mar 18, 2015

@DHager thanks for the feedback. Yep I don't really like that situation too. I'm planning to improve that in 5.0 (currently working on it) but it will mean a bit of BC breaks.

@DHager

This comment has been minimized.

Show comment
Hide comment
@DHager

DHager Mar 19, 2015

One idea for a 4.0 workaround: Something like ClassDefinitionHelper::afterward($func) which runs after everything else (instantiation, other setter injection) occurs. Building on a documentation example, suppose you want to invoke setFoo multiple times with multiple arguments?

'My\OtherClass' => DI\object()
    ->scope(Scope::PROTOTYPE())
    ->constructor(DI\link('db.host'), DI\link('db.port'))
    ->method('setFoo2', DI\link('My\Foo1'), DI\link('My\Foo2'))
    ->property('bar', 'My\Bar')
    ->afterward(function(Container $c, $obj){
        $obj->setFoo($c->get("ThingOne"));
        $obj->setFoo($c->get("ThingTwo"));
        $obj->setFoo($c->get("ThingThree"));
    }),
'My\ThingOne' => DI\object(),
'My\ThingTwo' => DI\object(),
'My\ThingThree' => DI\object(),

That way all the problematic setters can be tossed in there, without sacrificing the normal ways of dealing with constructor-injection, properties, etc.

DHager commented Mar 19, 2015

One idea for a 4.0 workaround: Something like ClassDefinitionHelper::afterward($func) which runs after everything else (instantiation, other setter injection) occurs. Building on a documentation example, suppose you want to invoke setFoo multiple times with multiple arguments?

'My\OtherClass' => DI\object()
    ->scope(Scope::PROTOTYPE())
    ->constructor(DI\link('db.host'), DI\link('db.port'))
    ->method('setFoo2', DI\link('My\Foo1'), DI\link('My\Foo2'))
    ->property('bar', 'My\Bar')
    ->afterward(function(Container $c, $obj){
        $obj->setFoo($c->get("ThingOne"));
        $obj->setFoo($c->get("ThingTwo"));
        $obj->setFoo($c->get("ThingThree"));
    }),
'My\ThingOne' => DI\object(),
'My\ThingTwo' => DI\object(),
'My\ThingThree' => DI\object(),

That way all the problematic setters can be tossed in there, without sacrificing the normal ways of dealing with constructor-injection, properties, etc.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Mar 19, 2015

Member

The best workaround in 4.* is to use DI\factory though because it let you do all that already?

Member

mnapoli commented Mar 19, 2015

The best workaround in 4.* is to use DI\factory though because it let you do all that already?

@DHager

This comment has been minimized.

Show comment
Hide comment
@DHager

DHager Mar 19, 2015

Doesn't it force you to give up on the constructor-argument autowiring?

DHager commented Mar 19, 2015

Doesn't it force you to give up on the constructor-argument autowiring?

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Mar 19, 2015

Member

Yes using a closure for that will force you to take care of instantiating the object entirely. Usually I need this pretty rarely so it's kind of acceptable (in my experience).

Member

mnapoli commented Mar 19, 2015

Yes using a closure for that will force you to take care of instantiating the object entirely. Usually I need this pretty rarely so it's kind of acceptable (in my experience).

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Apr 12, 2015

Member

Fixed in 5.0 (to be released in the next months)

Member

mnapoli commented Apr 12, 2015

Fixed in 5.0 (to be released in the next months)

@mnapoli mnapoli closed this Apr 12, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment