auraphp / Aura.Di

Dependency Injection System
MIT License
349 stars 63 forks source link

[v3] Container#call(callable $callable, array $params) #110

Closed glen-84 closed 8 years ago

glen-84 commented 8 years ago

Edit: See this comment for an update.

I'm thinking about writing a controller dispatcher that invokes a method on the controller class and injects dependencies as required (route parameters would also be passed in).

For this, a non-lazy lazyGetCall would probably work (though I haven't yet tested the current lazy option). There's not really any reason for this to be lazy – would you consider adding a non-lazy alternative?

harikt commented 8 years ago

Hi @glen-84 ,

I don't really know what you are looking for. Can you show some code ?

When I read the above message what quickly came into my mind it the http://github.com/auraphp/Aura.Dispatcher which actually does the same with the help of lazy di functionality.

<?php
use Aura\Dispatcher\Dispatcher;

$dispatcher = new Dispatcher;
$dispatcher->setObjectParam('controller');
$dispatcher->setMethodParam('action');
$dispatcher->setObject('blog', $di->lazyNew('SomeController'));
$params = [
    'controller' => 'blog',
    'action' => 'index',
    'id' => 12,
];

$result = $dispatcher($params);

assuming

<?php
class SomeController
{
    public function index($id)
   {
   }
}

What ever you pass in the $params array can be caught via the index.

glen-84 commented 8 years ago

@harikt Similar to that, except that it would perform method injection.

I'm using Zend Expressive, currently with FastRoute.

A controller action might look like this:

public function editAction($id, ServiceForThisActionOnly $service) {}

The dispatch code would then look something like this:

$result = $container->getCall(My\Controller::class, 'editAction', ...$routeParams);

$routeParams includes the ID, and the DI container injects the service.

harikt commented 8 years ago

@glen-84 cool.

So probably you are using zend-mvc to make use of controller having multiple actions.

Else I would have suggested to go with setter injection or constructor injection itself.

For method injection currently my guess it lazyGetCall is the only way to go.

glen-84 commented 8 years ago

I'm not using zend-mvc. I want to create simple dispatch middleware that invokes the appropriate action of the controller.

The controller will use setter & constructor injection, but method injection would be an optional feature in cases where not all actions require the dependency. Lumen supports this functionality (link).

Yes, I can probably use lazyGetCall, but the lazy part seems unnecessary, so I thought I'd ask if you would be open to adding a non-lazy alternative (getCall).

harikt commented 8 years ago

@glen-84 cool. Thank you. I get you now.

I would have suggested a different approach here, but currently I assume we could not go for the container will be locked after an instance is created. Else it would be something like

$di->set('mycontroller', $di->lazyGetCall(My\Controller::class, 'editAction', $routeParams));

and use $di->get('mycontroller') .

The controller and action will be dynamic ones.

For the current time it will not work the way.

glen-84 commented 8 years ago

I'll only have access to the route parameters once the routing has taken place, and at that point I'd prefer not to be adding anything new to the container. Do you see any problems with my proposed solution?

pmjones commented 8 years ago

@glen-84 The interesting thing here is that you'll need the DI container itself to actually make the call. I presume that means injecting the container itself into the dispatching mechanism?

pmjones commented 8 years ago

@glen-84 I'm happy to entertain this idea to tease out its ramifications, but meanwhile you may wish to examine Action-Domain-Responder as an alternative to Model-View-Controller. The key point for this conversation is that each Action has only one method in it, as vs a Controller that might have many action methods. That distinction makes it possible to create just the object you want using constructor injection (and then invoke it), rather than doing dependency-injection-of-optional-dependencies-at-method-call-time.

glen-84 commented 8 years ago

I presume that means injecting the container itself into the dispatching mechanism?

Correct. This is "bootstrap" or "framework" code as opposed to "application" code, so I think that it's acceptable.

The key point for this conversation is that each Action has only one method in it, as vs a Controller that might have many action methods.

Yep, I'm all too familiar with this conundrum. I posted this question to StackOverflow when I was working on a ZF2 application back in 2013. I can't really get used to having a single action per class, and therefore per file – it just seems excessive. I like to group the basic CRUD and possibly one or two other actions in each "controller".

That said, the plan is to support single-action __invoke-style "controllers" as well, for flexibility. I just think that method-based injection gives you one more level of control. It's optional, so it doesn't have to be used when it's not appropriate.

jakejohns commented 8 years ago

There is what I believe to be somewhat related discuss on this 'invoke via container with parameter resolution' business over at container-interop/container-interop#43

For what it's worth, I personally don't like the smell as far as I can tell.

As @harikt points out, this looks like a different concern to me, namely dispatching. I believe the tail end of the 'interop' discussion starts to come to a similar conclusion.

Full disclosure, I am currently an ADR partisan. That being said, I must note that the idea that a controller has a number of methods each with varying dependencies seems like a strong argument in favor of breaking it into discrete objects with their own dependencies. I feel like I'd much rather have a few action objects, each with a dependency or two, rather than a giant controller class coupled to a multitude of other services, but each method requiring only subset of the total coupling.

glen-84 commented 8 years ago

@jakejohns,

Thanks for the link, that is certainly relevant.

It's not a "giant controller" – firstly it would be skinny, and secondly it would usually be capped at 4 - 6 methods. It might look something like this:

class UserController
{
    public function __constructor(UserService $service);

    public function indexAction();

    public function addAction(EmailService $emailService);

    public function editAction();

    public function deleteAction();
}

A silly example, but I think you get the idea. =)

I'm hoping for something more than just "it seems smelly" – if there are concrete reasons for why this is a bad idea, then I'd like to hear them.

harikt commented 8 years ago

Hi @glen-84 ,

If I am correct the EmailService is to send emails ?

In that case I will say use an event listener and you can catch it there than injecting it here.

If it is doing something else and the UserController class don't need EmailService at all it would be a good idea to move that to a separate controller .

glen-84 commented 8 years ago

@harikt Yes, but it's just an example of a dependency that may be required by only a single action. It's a poor example because I would personally send mail from the user service, but you can replace it with something else (like forms or whatever).

I'm not sure about using event listeners for this type of thing – it may be awkward to debug or not explicit enough.

Anyway, this issue is really just about adding a non-lazy service method invoker.

harikt commented 8 years ago

Sure, was just telling what I thought from the example. Feel free to ignore.

Thank you.

glen-84 commented 8 years ago

@harikt I appreciate your input. =)

glen-84 commented 8 years ago

I realize now that it's call or invoke that I need, not getCall. (similar to this)

$container->call(callable $callable, array $params)

... where $params are name/value pairs that are used before asking the container to resolve each parameter.

Also, it would be nice if you could configure parameters ahead of time as well, as you do with constructor parameters. Something like:

$container->types['Foo']['myMethod']['myParam'] = $container->lazyGet('service');

jakejohns commented 8 years ago

This might not be a useful, but I had a thought, so why not share?

If you really didn't want to break up a controller into "Action Objects", maybe you could just wrap the controller in various action closures like so:

$di->set(
    'UserController/addAction:caller',
    function () use ($di) {
        return function ($param) use ($di) {
            $ctrl  = $di->newInstance('UserController');
            $email = $di->get('emailService');
            return call_user_func([$ctrl, 'add'], $email, $param);
        };
    }
);

$params = ['id' => 1];
$action = $di->get('UserController/addAction:caller');
$action($params);

Not sure this actually gets you much, though.

Weird caveat: Setting a service as a callable auto-wraps it in a Lazy. That seems little weird and magical to me, but I haven't investigated the reasoning/implications. Anyway, thats the reason I'm setting my/action:caller to a closure that returns a closure, rather than just the final closure.

harikt commented 8 years ago

@jakejohns nice :) .

glen-84 commented 8 years ago

@jakejohns I don't really understand what the purpose of that is.

My use case is simple: I get a controller class name from routing, and I need to instantiate that (newInstance), and then invoke an action on it, filling in arguments as necessary (call).

jakejohns commented 8 years ago

@glen-84 I dont think it the ideal solution for your desire. However, it does move configuring the resolution of the method dependencies to the closure, basically replicating the 'separate action objects' paradigm in function. And removing the need to worry about the dependencies when calling.

You can't do this cuz of the dependency:

$controllerName = $route->getController();
$actionName = $route->getAction();
$controller = $di->newInstance($controllerName);
$controller->$actionName($route->getParams());

But you could do something like this:

$controllerName = $route->getController();
$actionName = $route->getAction();
$serviceName = "$controllerName/$actionName:caller"; // or whatever

$callable = $di->has($serviceName)
    ? $di->get($serviceName)
    : [$di->newInstance($controllerName), $actionName];

$result = $callable($route->getParams());

Again, just an idea. Probably not that helpful, honestly.

pmjones commented 8 years ago

Overall, after consideration, I think this smacks too much of the Container acting as a Dispatcher, which is something I'd like to avoid.

glen-84 commented 8 years ago

I might use PHP-DI/Invoker for this.

pmjones commented 8 years ago

Yes, that does seem to support your use-case. You may also wish to examine Aura.Dispatcher for something similar.

glen-84 commented 8 years ago

Yes, @harikt mentioned that as well. I need DI though.

pmjones commented 8 years ago

/me nods

What happens with the Dispatcher is that, when setting it up, you pass a factory for the named object to be dispatched to; e.g., $dispatcher->set('ClassName', $di->lazyNew('ClassName')). Or, and this is the intended use, $dispatcher->set('controller-value-from-uri', $di->lazyNew('ControllerClass')).

Either way, that keeps the concerns well-separated, and you can pass around the dispatcher without passing the entire container.

However, that level of separation may not be your thing.

glen-84 commented 8 years ago

That would do constructor and setter injection, but this is all about method injection.

... unless I'm missing something – I haven't really looked closely at Aura.Dispatcher yet.

pmjones commented 8 years ago

Mmm, yes, you're right, with Aura.Dispatcher, the params are passed to the method as passed as-is, and are not used as hints to resolve against. Invoker sounds like a better fit for your case. (I opine that injection of that kind is unwise, but then, if it's what you need, it's what you need.) I'm interested to hear how it works out for you; good luck!

glen-84 commented 8 years ago

I opine that injection of that kind is unwise, but then, if it's what you need, it's what you need.

In most cases, we'll inject via the controller constructor – this just adds a bit of extra flexibility. TBH, we may even find it to be unnecessary once we have progressed further with the project.

I'm interested to hear how it works out for you; good luck!

Thanks! I appreciate all of your assistance.