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

Support for variadic arguments #619

Open
tssge opened this issue Aug 31, 2018 · 12 comments · May be fixed by #624
Open

Support for variadic arguments #619

tssge opened this issue Aug 31, 2018 · 12 comments · May be fixed by #624

Comments

@tssge
Copy link

tssge commented Aug 31, 2018

PHP-DI does not currently support variadic arguments for autowiring. It will only inject the first element to the variadic parameter in a constructor, silently discarding the rest.

For example for this class:

class UserProfile implements Timeline\Engine
{
    public function __construct(DbConnection $dbConnection, TimelineModule ...$modules)
    {

cannot be autowired by:

            Engine::class => \DI\autowire(UserProfile::class)
                ->constructor(
                    \DI\get(DbConnection::class),
                    \DI\get(Module\CasinoFollow::class),
                    \DI\get(Module\UserFollow::class),
                    \DI\get(Module\Review::class)
                ),

but must be instead autowired by:

            Engine::class => function(ContainerInterface $c) {
                return new UserProfile(
                    $c->get(DbConnection::class),
                    $c->get(Module\CasinoFollow::class),
                    $c->get(Module\UserFollow::class),
                    $c->get(Module\Review::class)
                );
            },

which in turn reduces the efficiency of caching I believe and is harder to debug.

@mnapoli
Copy link
Member

mnapoli commented Aug 31, 2018

Thanks for the detailed issue!

Can you confirm that:

            Engine::class => \DI\autowire(UserProfile::class)
                ->constructor(
                    \DI\get(DbConnection::class),
                    \DI\get(Module\CasinoFollow::class),
                    \DI\get(Module\UserFollow::class),
                    \DI\get(Module\Review::class)
                ),

doesn't work and that it injects only $dbConnection and the first module?

@tssge
Copy link
Author

tssge commented Sep 3, 2018

Indeed, it only injects the first module: https://i.imgur.com/BANmom5.png

@tssge
Copy link
Author

tssge commented Sep 6, 2018

I would like to start working on this, but I don't feel so accustomed to PHP-DI internals yet. Do you have any tips @mnapoli for me? Where should I start?

I have written some preliminary tests for this behavior.

@mnapoli
Copy link
Member

mnapoli commented Sep 6, 2018

@tssge that's great!

I suggest you have a look here: https://github.com/PHP-DI/PHP-DI/blob/master/src/Definition/Resolver/ParameterResolver.php#L51

You can see that we do foreach on parameters returned by PHP's reflection. So a variadic parameter will be interpreted as 1 parameter only.

Try to write unit tests but more importantly functional tests in here: https://github.com/PHP-DI/PHP-DI/blob/master/tests/IntegrationTest/Definitions/CreateDefinitionTest.php

@tssge tssge linked a pull request Sep 6, 2018 that will close this issue
@tssge
Copy link
Author

tssge commented Sep 18, 2018

@mnapoli Don't know if you noticed, but I added pull request for this functionality. Can you check if it's of an okay quality? Would like some feedback

@mnapoli
Copy link
Member

mnapoli commented Sep 18, 2018

@tssge yes that is awesome thanks! I had a quick look last week but because of conferences I didn't have enough time to review it carefully (this PR is more complex that other simple PRs :) )

I'll try to have a look at it this week.

@yanakey
Copy link

yanakey commented Sep 24, 2018

Need this feature too.

@yanakey
Copy link

yanakey commented Sep 24, 2018

Is it possible to make it work the same way with constructorParameter() method too?
To be able to extend definition for one param only.

Example:

class MyClass {
  public function __construct(Foo $foo, Bar $bar, Baz $baz, ...$args) {}
}

MyClass::class => DI\autowire()->constructorParameter('args', ...['foo', 'bar', 'baz', 'etc'])

@tssge
Copy link
Author

tssge commented Sep 24, 2018

With the patch provided, the following should be possible:

MyClass::class => DI\autowire()
    ->constructorParameter(0, 'foo')
    ->constructorParameter(1, 'bar')
    ->constructorParameter(2, 'baz')
    ->constructorParameter(3, 'etc')

if the signature is something like this:

class MyClass {
    function __construct(string ...$strings) {}
}

Multiple named parameters are not supported (as of yet at least) as the parameters are saved to an array keyed by their name.

@yanakey
Copy link

yanakey commented Sep 25, 2018

Not quite what I would like to do (-:

Now I can avoid defining other constructor arguments and have them autowired only this way:

class MyClass {
  public function __construct(Foo $foo, Bar $bar, Baz $baz, $args) {}
}

[
  MyClass::class => DI\autowire()
    ->constructorParameter('args', ['a', 'b', 'c', 'etc'])
]

But I would like to type-hint $args:
public function __construct(Foo $foo, Bar $bar, Baz $baz, string ...$args) {}

And define it like this:

[
  MyClass::class => DI\autowire()
    ->constructorParameter('args', ...['a', 'b', 'c', 'etc'])
]

@tssge
Copy link
Author

tssge commented Sep 25, 2018

You could do it this way (with the patch merged):

class MyClass {
  public function __construct(Foo $foo, Bar $bar, Baz $baz, string ...$args) {}
}

 MyClass::class => DI\autowire()
    ->constructorParameter(3, 'a')
    ->constructorParameter(4, 'b')
    ->constructorParameter(5, 'c')

Parameters 0 to 2 (types of Foo, Bar and Baz) would be autowired automatically without defining them as you'd expect with PHP-DI.

I do not personally prefer this

MyClass::class => DI\autowire()
    ->constructorParameter('args', ...['a', 'b', 'c', 'etc'])

to work as you described, because that would show the user in the constructorParameter method signature that multiple parameters are valid to any constructor parameter, when the situation with PHP is that only the last parameter can be variadic.

For example: user might think based on method signature and documentation that this would be valid:

MyClass::class => DI\autowire()
    ->constructorParameter('bar', ...['a', 'b', 'c', 'etc'])
    ->constructorParameter('args', ...['a', 'b', 'c', 'etc'])

When obviously the part where bar is defined would not work. I understand that you'd perhaps want to use the parameter name instead of the argument number for clarity, but I don't think it's a good idea.

@mnapoli
Copy link
Member

mnapoli commented Oct 12, 2020

I had a quick look at this and I think it might be worth implementing the same rules as PHP 8's named parameters: https://wiki.php.net/rfc/named_params (over all the PHP-DI features)

The integration with variadics is clearly defined (and constrained) in there, so it could be both a good lead to follow, as well as minimize confusion and friction down the road.

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

Successfully merging a pull request may close this issue.

3 participants