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

Auto-wiring dependencies for setter injection #1

Closed
J7mbo opened this issue May 22, 2013 · 22 comments
Closed

Auto-wiring dependencies for setter injection #1

J7mbo opened this issue May 22, 2013 · 22 comments
Assignees

Comments

@J7mbo
Copy link
Contributor

J7mbo commented May 22, 2013

As the title explains, would it be possible to include this capability in here, so that when Auryn performs it's reflection to read constructor signatures, could the reflection also be performed for methods within the class?

Obviously this would increase overheard significantly, but if everything's cached - that shouldn't be an issue. I'm going to take a look at the source over the course of this week but, what are your opinions on this @rdlowrey?

@morrisonlevi
Copy link
Collaborator

I'm not @rdlowrey but I'm personally against this behavior. Every time I have personally wanted a setter method for a dependency it been because of poor program design.

My $0.02

@J7mbo
Copy link
Contributor Author

J7mbo commented May 22, 2013

@morrisonlevi Hey levi, I'm actually thinking controller-wise, if my url matches controller/action, and say my login controller requires a user service for the login action, and another service for a different action - why instantiate both dependencies when only one is used for each method? Is that poor program design by mapping url to controller / action?

@morrisonlevi
Copy link
Collaborator

If it is a different action that requires different dependencies, then I would say it is bad design. The alternative option is to pass the dependency needed for just that action at method call time:

    $controller->action($id, $param, $extraDependencyForAction)

I'd like to clarify that this is just based off my experience. I could be wrong here, but I wanted to voice my opinion on the matter.

@rdlowrey
Copy link
Owner

I generally prefer constructor injection over setter injection as well. However, I have in the past used Auryn in conjunction with a custom router class to (1) provision a "controller" class AND (2) subsequently provision the parameters for the controller's "action" method using a combination of arguments parsed from a URI and definitions for parameter typehints. While it's not the same thing as setter injection, it's not too far off. I think the better course of action may be to expand the ReflectionStorage interface so that it can readily cache function and parameter list reflections for methods that aren't __construct. This would make it relatively straightforward to implement your own flavor of automatic setter injection by doing something like the following hypothetical:

class MyRouter {

    private $injector;
    private $reflStorage;

    function __construct(Auryn\Injector $injector, Auryn\ReflectionStorage $reflStorage) {
        $this->injector = $injector;
        $this->reflStorage = $reflStorage;
    }

    function route($method, $uri) {
        // ... do some routing to get $className, $methodName and $uriArgs parsed from $uri ... //

        $controller = $this->injector->make($className);
        $reflMethod = $this->reflStorage->getMethod($className, $methodName); // hypothetical, not implemented
        $reflParams = $this->reflStorage->getMethodParams($reflMethod); // hypothetical, not implemented

        $mergedArgs = [];

        foreach ($reflParams as $reflParam) {
            $paramName = $reflParam->name;
            if (array_key_exists($paramName, $uriArgs)) {
                $mergedArgs[$paramName] = $uriArgs[$paramName];
            } elseif ($typehint = $this->reflStorage->getParamTypeHint($reflMethod, $reflParam)) {
                $mergedArgs[$paramName] = $this->injector->make($typehint);
            } elseif ($reflParam->isDefaultValueAvailable()) {
                $mergedArgs[$paramName] = $reflParam->getDefaultValue();
            } else {
                $mergedArgs[$paramName] = NULL;
            }
        }

        $reflMethod->invokeArgs($controller, $mergedArgs);
    }
}

@J7mbo
Copy link
Contributor Author

J7mbo commented May 22, 2013

I know that setter injection is used for optional dependencies in the class - and I thought it was pretty common.

Auryn already uses $injector->make($controller); which instantiates required dependencies.

I was just hoping for it to also instantiate dependencies depending on the $action being called too.

Running $controller->action($id, $param, $extraDependencyForAction) is not automatically wired (I'd need to compose a graph for every single optional one). Although if doing something like this would inspire bad architecture, I suppose it would be for the best leaving it out.

@rdlowrey
Copy link
Owner

Note that you can implement something like the code above right now. You just can't use the reflection storage instance to cache the method and parameter typehints with the current code. You'd simply need to replace the two hypothetical calls to ReflectionStorage::getMethod and ReflectionStorage::getMethodParams and retrieve those reflections directly using PHP's reflection API.

@rdlowrey
Copy link
Owner

@J7mbo Is this still something you're interested in? I'm kicking around the idea of adding an Injector::execute method that accepts a callable (or an array [$className, $methodName] that can be provisioned automatically) and an optional array of values. So the API would look something like this:

class HelloDependency {
    function saySomething() { return 'Hello'; }
}

class Greeting {
    function greet(HelloDependency $hello, $name) {
        echo $hello->saySomething() . ', ' . $name;
    }
}

// Will echo "Hello, world"
$provider->execute(['Greeting', 'greet'], $args = [':name' => 'world']);

This would accomplish what you're asking and would eliminate some steps when the ultimate goal is to actually do something with the objects you instantiate.

@J7mbo
Copy link
Contributor Author

J7mbo commented Jun 26, 2013

@rdlowrey Definitely still interested in having this added: can the constructor injection still happen automatically for class-based dependencies and then, when running your $provider-execute() method, have the method dependencies recursively instantiated? If so, 👍 !

@rdlowrey
Copy link
Owner

Yeah that's the plan. Alternatively you could pass in a preexisting instance and only the relevant method's parameters would be provisioned:

$provider->execute([$existingGreetingObj, 'greet'], $args = [':name', 'world']);

It would also work for any other valid callable. For example:

$myGreeter = function(HelloDependency $dep, $name) {
    echo $dep->saySomething() . ', '. $name;
};
$provider->execute($myGreeter, $args = [':name' => 'world']);

I'll put this on the menu to work on. I may get around to it today but we'll just see how things go. Regardless I'm likely to implement it soon.

@J7mbo
Copy link
Contributor Author

J7mbo commented Jun 26, 2013

Looks awesome! Looking forward to it :)

@rdlowrey
Copy link
Owner

@J7mbo I've just committed the new Injector::execute() functionality. You can now execute any valid PHP callable with full recursive provisioning of its method signature:

$injector->execute(function(){});
$injector->execute([$objectInstance, 'methodName']);
$injector->execute('globalFunctionName');
$injector->execute('MyStaticClass::myStaticMethod');
$injector->execute(['MyStaticClass', 'myStaticMethod']);
$injector->execute(['MyChildStaticClass', 'parent::myStaticMethod']);
$injector->execute('ClassThatHasMagicInvoke');
$injector->execute($instanceOfClassThatHasMagicInvoke);

Additionally, you can pass in the name of a class for a non-static method and the injector will automatically provision an instance of the class (subject to any definitions or shared instances already stored by the injector) before provisioning and invoking the specified method:

class Dependency {}
class AnotherDependency {}
class Example {
    function __construct(Dependency $dep){}
    function myMethod(AnotherDependency $adep, $arg) {
        return $arg;
    }
}

$injector = new Auryn\Provider;
var_dump($injector->execute(['Example', 'myMethod'], $args = [':arg' => 42]));
// outputs: int(42)

In conclusion, please test this and let me know if you have any questions/comments/feedback. It's very well unit-tested already so it should be bug-free but you never know. If everything seems cool in a couple of days I'll go ahead and tag a new release.

@rdlowrey
Copy link
Owner

Woops -- looks like I forgot to add support for classes with a magic __invoke() method. I've just committed, tested and pushed the additional Injector::execute() functionality for invokable objects.

@ghost ghost assigned rdlowrey Jun 27, 2013
@J7mbo
Copy link
Contributor Author

J7mbo commented Jun 27, 2013

I've been playing around with the new code this evening, and so far it's totally brilliant! Can't fault it, method injection for Controller / Action works perfectly. :D

@rdlowrey
Copy link
Owner

Cool. Thanks for the feedback. And thanks or the initial suggestion -- it took me awhile to get over my initial misgivings but I think it's a nice feature at this point.

@ascii-soup
Copy link
Contributor

This is really cool. That is all. :)

@rdlowrey
Copy link
Owner

rdlowrey commented Jul 4, 2013

@ascii-soup Thanks; hopefully people find the feature useful.

@vlakarados
Copy link
Contributor

Just stumbled upon this problem here:

Beforehand I had an instance of a controller a

$controller = $injector->make($controllerClass);

and called it like

$result = call_user_func_array(array($controller, $method), $args);

That worked great (I could pass any number of any params as $args).

Now I tried to make that possible using ->execute(), but that requires passing all the arguments manually filling the array.

So how may I pass all arguments plus injecting the ones typehinted?

I think that way controller method would look like this:

public function dosomething($id, $name, \App\Repository\Users $users)
{
}

That looks possible only with the 'raw named parameter', but not unnamed. Forcing all controller actions to use named params like $arg1, $arg2 etc. is definitely not worth it.

I hope you understand my issue, maybe it's a poor design, but I don't think this is the case

@J7mbo
Copy link
Contributor Author

J7mbo commented Feb 17, 2015

Are the $id and $name parameters parameters passed in from your GET / POST request (as in slugs)?

@vlakarados
Copy link
Contributor

Exactly, so they're not constant, there may be one, more or none

@vlakarados
Copy link
Contributor

I see that I may do that like (IIRC) it was in codeigniter using something like ->segment(n), but that's ugly :)

@J7mbo
Copy link
Contributor Author

J7mbo commented Feb 19, 2015

Get your $params as an array from your request, then prefix them with a : for scalar injection with Auryn:

    array_walk($params, function($value, $key) use (&$args) {
        $args[sprintf(':%s', $key)] = $value;
    });

This is how it works for my in Symfony, but you can customise for yourself. See how I'm making the controller, then executing the action with the given $args (the : params).

    /** Executed by the HTTP Kernel **/
    return function () use ($controller, $action, $args) {
        return $this->injector->execute([$this->injector->make($controller), $action], $args);
    };

@vlakarados
Copy link
Contributor

@J7mbo, thank you, works great, the only downside is that I should name the arguments the same way as they're named in $params, which comes from nikic/FastRoute, but that makes the code even cleaner by forcing to stick to same names!

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

No branches or pull requests

5 participants