auraphp / Aura.Di

Dependency Injection System
MIT License
349 stars 63 forks source link

Wrong resolution of variadic parameters #164

Closed denfil closed 5 years ago

denfil commented 6 years ago

Here is the code example:

class Test {
    public function __construct(...$items) {
        print_r($items);
    }
}

class Config extends Aura\Di\ContainerConfig {
    public function define(Aura\Di\Container $di)  {
        $di->params[Test::class][] = 'one';
        $di->params[Test::class][] = 'two';
        $di->set('test', $di->lazyNew(Test::class));
    }
}

(new Aura\Di\ContainerBuilder())
    ->newConfiguredInstance([Config::class])
    ->get('test');

Expected output is Array ( [0] => one [1] => two ) but actual output is Array ( [0] => one )

harikt commented 6 years ago

Try

$di->params[Test::class]['items'][] = 'one';
$di->params[Test::class]['items'][] = 'two';

or

$di->params[Test::class]['items'] = [ 'one' ,'two'];
denfil commented 6 years ago

@harikt it's not the same behaviour I want to get, because __construct(...$items) is not the same as __construct(array $items)

You see, in my example constructor should get two parameters of type string new Test('one', 'two'), but in your solution it will get one parameter of type array new Test(['one', 'two'])

harikt commented 6 years ago

I get you now. If you can send a patch I ll look into it. On Feb 19, 2018 4:29 PM, "Denis Filimonov" notifications@github.com wrote:

@harikt https://github.com/harikt it's not the same behaviour I want to get, because construct(...$items) is not the same as construct(array $items)

You see, in my example constructor should get two parameters of type string new Test('one', 'two'), but in your solution it will get one parameter of type array new Test(['one', 'two'])

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/auraphp/Aura.Di/issues/164#issuecomment-366655770, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHWht09MC6VlOA7qRBnzk-ngyU6MxmAks5tWVQigaJpZM4R26uA .

marcusjhdon commented 6 years ago

I've also experienced this issue, so I decided to create a patch. The approach I've taken is to treat the parameter as an array, then to expand it just before object instantiation using the (cached) reflection info.

Please consider applying this patch to the main branch soon, so I can avoid having to maintain a custom fork. Thanks.

variadic-patch.txt

harikt commented 6 years ago

Hey @marcusjhdon ,

Could you send a PR, it may be helpful.

Thank you.

marcusjhdon commented 6 years ago

Done: https://github.com/auraphp/Aura.Di/pull/167

harikt commented 6 years ago

Fixed in https://github.com/auraphp/Aura.Di/pull/167 . Will shortly push the new tag.

harikt commented 6 years ago

Sorry the issue looks not fixed yet. So re-opening it.

harikt commented 6 years ago

167 actually fixed the issue with constructor params, but not for setter. The example I copied from here was wrong and that made me think that failed. Currently things are on 3.x branch. I am not tagging for now for I am still debating with myself whether it should be moved to the Resolver::resolve method.

frederikbosch commented 5 years ago

I would suggest that this functionality can be achieved by using a mutation as in PR #176 or by using factory and that the library should not resolve variadic arguments for setters.

frederikbosch commented 5 years ago

Since #176 is merged into master, one can choose to mutate an object and therefore call methods with multiple arguments and/or variadic arguments.