rdlowrey / auryn

IoC Dependency Injector
MIT License
722 stars 65 forks source link

Argument unpacking #134

Closed designermonkey closed 1 year ago

designermonkey commented 8 years ago

Before I even delve into trying this approach to a problem I have with some code I'm working on, I thought it best to ask first.

How does Auryn handle argument unpacking as outlined here: https://wiki.php.net/rfc/argument_unpacking

Basically, I need to provide a variable number of classes to constructors, and instead of defining a constructor function for each instance, I want to use an abstract and use argument unpacking to allow the variation. Then I want to define each class in Auryn so that it can auto-wire them up for me.

Would this approach work?

abstract class AbstractBase
{
    public function __construct(PayloadInterface $payloadInstance, RepositoryInterface ...$repositories)
    {
        // $repositories is an array of RepositoryInterface instances
    }
}
designermonkey commented 8 years ago

Just a quick test here

<?php

require __DIR__.'/vendor/autoload.php';

class Test
{
    public function __construct($payloadInstance, array ...$repositories)
    {
        var_dump($payloadInstance, $repositories);
    }
}

$injector = new Auryn\Injector();

$injector->define('Test', [
    ':payloadInstance' => 'my test string',
    ':comments' => ['test'],
    ':pages' => ['test'],
    ':apps' => ['test'],
]);

$test = $injector->make('Test');

yields

PHP Catchable fatal error:  Argument 2 passed to Test::__construct() must be of the type array, null given in /Users/designermonkey/Projects/localhost/auryn-test.php on line 7
PHP Stack trace:
PHP   1. {main}() /Users/designermonkey/Projects/localhost/auryn-test.php:0
PHP   2. Auryn\Injector->make() /Users/designermonkey/Projects/localhost/auryn-test.php:22
PHP   3. Auryn\Injector->provisionInstance() /Users/designermonkey/Projects/localhost/vendor/rdlowrey/auryn/lib/Injector.php:367
PHP   4. ReflectionClass->newInstanceArgs() /Users/designermonkey/Projects/localhost/vendor/rdlowrey/auryn/lib/Injector.php:400
PHP   5. Test->__construct() /Users/designermonkey/Projects/localhost/vendor/rdlowrey/auryn/lib/Injector.php:400

Catchable fatal error: Argument 2 passed to Test::__construct() must be of the type array, null given in /Users/designermonkey/Projects/localhost/auryn-test.php on line 7

Call Stack:
    0.0004     228920   1. {main}() /Users/designermonkey/Projects/localhost/auryn-test.php:0
    0.0033     534304   2. Auryn\Injector->make() /Users/designermonkey/Projects/localhost/auryn-test.php:22
    0.0033     535264   3. Auryn\Injector->provisionInstance() /Users/designermonkey/Projects/localhost/vendor/rdlowrey/auryn/lib/Injector.php:367
    0.0034     540192   4. ReflectionClass->newInstanceArgs() /Users/designermonkey/Projects/localhost/vendor/rdlowrey/auryn/lib/Injector.php:400
    0.0034     540304   5. Test->__construct() /Users/designermonkey/Projects/localhost/vendor/rdlowrey/auryn/lib/Injector.php:400
designermonkey commented 8 years ago

If I change one of the parameters to ':repositories' then it passes it through, but there's no way I can see to allow for arg unpacking :(

designermonkey commented 8 years ago

Interestingly, if I use indexed params like so

<?php

require __DIR__.'/vendor/autoload.php';

class Test
{
    public function __construct($payloadInstance, array ...$repositories)
    {
        var_dump($payloadInstance, $repositories);
    }
}

$injector = new Auryn\Injector();

$injector->define('Test', [
    'my test string',
    ['test'],
    ['test'],
    ['test'],
]);

$test = $injector->make('Test');

I get the following

string(14) "my test string"
array(1) {
  [0] =>
  array(1) {
    [0] =>
    string(4) "test"
  }
}

with the expected being

string(14) "my test string"
array(1) {
  [0] =>
  array(1) {
    [0] =>
    string(4) "test"
  }
  [1] =>
  array(1) {
    [0] =>
    string(4) "test"
  }
  [2] =>
  array(1) {
    [0] =>
    string(4) "test"
  }
}

Looking at the code, I can see where this is happening here https://github.com/rdlowrey/auryn/blob/master/lib/Injector.php#L452

Also, reading a bit about reflection, would the ReflectionParameter::isVariadic() method be useful here?

I'll stop spamming now :D

Danack commented 8 years ago

How does Auryn handle argument unpacking as outlined here:

It doesn't. It shouldn't.

Auryn supports injecting some parameters by name because PHP is a bit shit. However supporting variadics types is a different kettle of fish.

Why Auryn supports injecting params by name at all

PHP is missing a feature; the ability to have strongly typed scalars. Something similar to the code below should work, if that feature was added:

class DatabaseUsername extends string {}

function foo(string $username) {
  // ...
}

function connectToDB(DatabaseUsername $dbUsername) {
    foo($username);
}

However we don't have that feature in the language. Because of that if we want to create types like that we have to do more typing on the keyboard, to achieve that typing

class DatabaseUsername {
    private $value;
    public function __construct($value) {
        $this->value = $value;
    }
    public function getValue() {
        return $this->value;
    }
}

function foo(string $username) {
  // ...
}

function connectToDB(DatabaseUsername $dbUsername) {
    foo($username->getValue());
}

This is not an immense burden, but it is one that is quite annoying to have to do continuously, and particularly when you're upgrading a legacy code base to use Auryn.

To make it less of a burden Auryn supports defining parameters through $injector->defineParam() to define a single param by name or $injector->define($classname, [...]) to define multiple params for a single class.

Variadics aren't the same problem

From your example:

class Foo {
    public function __construct(array ...$repositories) {
        // ...
    }
}

In that scenario $repositories does not represent a single simple variable and so Auryn's ability to make life a bit easier for the programmer does not apply.

Instead ...$repositories represents a complex type. This will need a more advanced technique to be able to inject. I think the two possible solutions are to either use

Use a delegate method

The simplest way to achieve what you want to do is to use a delegate function for creating objects that have this variable dependency.

function createFoo(RepositoryLocator $repoLocator)
{
    //Or whatever code is needed to find the repos.
    $repositories = $repoLocator->getRepos('Foo');

    return new Foo($repositories);
}

$injector->delegate('Foo', 'createFoo')

This does what you want....however it has some limitations. In particular it can only be used for constructor injection. It cannot be used to inject the dependencies as a parameter in normal functions or methods.

Refactor the code to use a context

Using a 'context' doesn't have these limitations. Or to give it the full name, using the 'Encapsulated context patten'

The trade-off is that it would require you to refactor your code a little bit. This is a good trade-off to make, in this case. In fact it's a fantastic trade-off. It makes your code far easier to reason about.

// This is the context that holds the 'repositories'
class FooRepositories {
    private $repositories;

    private function __construct(RepositoryLocator $repoLocator)
    {
        //Or whatever code is needed to find the repos.
        $repositories = $repoLocator->getRepos('Foo');
    }
}

class Foo {
    public function __construct(FooRepositories $fooRepositories) {
        // ...
    }
}

There are a couple of reasons why using a context is a suprior solution:

TL:DR - Auryn shouldn't handle variadics at all imo. They aren't a type and so can't be reasoned about by a DIC. People should either use delegation or contexts to achieve what they're trying to do in a sane way.

I'll stop spamming now :D

np. A lot more interesting that internals....

designermonkey commented 8 years ago

This is by far the best thought our response I've seen so far, thanks.

I was using a context as you describe, but found that to be too much of a ServiceLocator pattern which I was trying to avoid.

When I think about it though, it does make sense that way, and may be the best solution. Unfortunately due to my codebase, using delegates in this scenario makes the code more complicated.

because PHP is a bit shit

Hah, maybe. Or maybe it's still a little immature in the OOP world, and will get there (I hope).

Thanks again.

Danack commented 8 years ago

I uploaded the above as a blag: http://blog.basereality.com/blog/16/Variadics_and_dependency_injection

Unfortunately due to my codebase, using delegates in this scenario makes the code more complicated.

I'm going to guess that the real problem is caused by:

  1. you're using a framework that only allows one layer of dispatch/execution.
  2. The dependency of ...$repositories can't be resolved until some other code has been run.

If that's true - that's actually a real problem. It's unfortunately not ready for use, but the reason why I'm working on my Tier 'framework' is that when you have 'variable dependencies' that can only be resolved after some other code has been run, then you need to have multiple levels of dispatch, with the earlier layers building up information about dependencies, and passing it to the DIC so that it's available to the later layers.

too much of a ServiceLocator pattern which I was trying to avoid.

So long as your application/domain code isn't aware of the ServiceLocator, it's usually an okay solution to use it in factories or other injector->delegate methods. If you think of it as a delayed bootstrap layer, using a servicelocator becomes less objectionable......and it's often better than the other choices.

And if you're using a framework that doesn't allow multiple levels of dispatch, it's almost certainly going to be required at some point.

designermonkey commented 8 years ago

I currently use Slim3 shimmed with Auryn, although I find it too 'locked in' like all the other frameworks, so will be working on bootstrapping together smaller components. I see the benefit of Injection over containers, where dependencies are prepared and configured in advance over, like you say, relying on pre code execution to allow definition.

Anyway, getting there.

kelunik commented 7 years ago

@Danack I think we could support it with explicit definitions like raw parameters, but I think one should not really need variadics for dependencies.

Danack commented 1 year ago

I added some words on why it's an excluded feature.