rdlowrey / auryn

IoC Dependency Injector
MIT License
723 stars 65 forks source link

Pass custom parameters by type name to make/execute methods #154

Closed guilhermeaiolfi closed 1 year ago

guilhermeaiolfi commented 7 years ago

Feature request for be able to do this:

class My Class {
   public function myMethod (AnotherClass $long_arg_name_can_be_changed_by_developers) {
   }
}
$injector->execute("MyClass::myMethod", [ "AnotherClass" => $anotherClassInstance]);

You can see examples and reasoning about why it's a desired feature here:

https://github.com/rdlowrey/auryn/issues/78

kelunik commented 7 years ago

I think we'd need a new prefix for this one, otherwise it may be ambiguous.

guilhermeaiolfi commented 7 years ago

I have never seen a case where the name of the arg is the same as the class name. Even if they are the same, it will have the same effect: MyClass $MyClass (you get the same result if it decides to use the type name or arg name).

You won't have, at least it doesn't make any sense to have:

MyClass $arg1, AnotherClass $MyClass

Em 10 de jan de 2017 13:58, "Niklas Keller" notifications@github.com escreveu:

I think we'd need a new prefix for this one, otherwise it may be ambiguous.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rdlowrey/auryn/issues/154#issuecomment-271614304, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPhmE07BpD12BVqQqlvdsmjLL0-uwrMks5rQ6qUgaJpZM4LfdSP .

kelunik commented 7 years ago

@bwoebi @Danack Are you fine with this? I'd like to implement it, as I need it myself. Only having the possibility of using param names is rather stupid.

Danack commented 7 years ago

I still am of the opinion that using the 2nd param of make/execute leads to inherent bad surprises, so I only use it in throwaway code.

I'm travelling at the moment, will try to write down the detail of the problem and suggest alternatives by monday.

kelunik commented 7 years ago

@Danack Current case is having a single FileKeyValueStorage and making plugins that should get a PrefixKeyValueStorage with their class name as prefix.

$plugin = $injector->make($plugin, [
    "+storage" => function () use ($plugin, $storage) {
        $lower = \strtolower(\strtr($plugin, "\\", "."));
        return new PrefixKeyValueStorage($storage, $lower . ".");
    },
]);
mattsah commented 7 years ago

I too would like this feature. The case that I have for it is right at the route level (I'd be open to other ideas), but basically the route is getting injected into whateve controller/actions it's calling. While it may be fine to say "call your parameter $router" -- it seems much more elegant to base it on the concrete class. I also think this should be possible with interfaces. It was mentioned elsewhere you might be injecting two classes which have the same interface, and there would be no way to determine which is which, however:

  1. I don't know that I've ever run into this, where I run into the aforementioned concern quite frequently.
  2. Even if I did run into this, nothing would prevent me, in that instance, from then using the parameter name based injection.
kelunik commented 7 years ago

@mattsah No worries, Auryn won't make any difference between interfaces and classes.

Are you passing the route or router into your controller / actions? Why do you need it there?

mattsah commented 7 years ago

@kelunik

The router so that the controller/action has reference to what called it. See: https://github.com/hiraeth-php/rad/blob/master/src/AbstractResponder.php#L53

kelunik commented 7 years ago

@mattsah Seems like that should inject Request and Response instead, and seems like a well-defined interface and thus not needing dynamic injection at all.

mattsah commented 7 years ago

@kelunik

That this example only makes use of the request/response from the router is just one example. The reason the router is injected as opposed to the request and response is specifically to enable the broadest possible types of routable actions. In this particular case, we only need the request/response, elsewhere, we may need to run a sub-request through it, or generate reverse routes.

While it would certainly be possible to define a series of interfaces and have the router pass different things depending on the interface which the routing controller/action implemented, this tends to be a far more flexible approach I've found.

The __invoke() method is executed through Auryn. So what also seems like a "well-defined" interface, is not. The router resolver will essentially construct/execute to whatever you give it, and only if you specify that you need the router should it be passing it's own instance.

The only other option I know of would be to share the router (so it's not building a new router when someone typehints it), but that seems far less preferable.

Danack commented 7 years ago

Hi @kelunik - I knew I was tired when I read this, but didn't realise I'd misread it completely.

Although I have concerns about people using make/execute with the 2nd parameter set, they are orthogonal to adding the ability to specify the args by type, rather than by name.

I think we'd need a new prefix for this one, otherwise it may be ambiguous.

Yeah - that looks like we made a mistake in the api. It looks like exec already requires the colon, like define does - but make() doesn't.

I have no objection to this, but would be good to discuss the implementation more before merging.

Danack commented 7 years ago

@mattsah

The __invoke() method is executed through Auryn. So what also seems like a "well-defined" interface, is not. The router resolver will essentially construct/execute to whatever you give it, and only if you specify that you need the router should it be passing it's own instance.

The only other option I know of would be to share the router

For my own 'framework', I deliberately have a base configuration for the injector, and then clone it within the framework when more information is available, as to what objects should be used to process a request.

e.g. in your router resolver, where you've determined which Router to use, and know which responder class should be used to handle the request.

$injector = clone $injector;
$injector->share($router);
if (get_class($router) !== 'Journey\Router') {
    $injector->alias('Journey\Router', get_class($router));
}

$injector->execute('FooResponder');

And Robert is your father's brother.

One limitation/benefit of that approach is that objects are only shared within processing one request. Which may need thinking about for long running processes.

kelunik commented 7 years ago

How about \ as a prefix for by type and +\ for a callable by type?

Danack commented 1 year ago

I'm going to close this issue on the grounds that:

$route_injector = clone $injector;
$route_injector->defineParam($key, $value);
$route_injector->execute($route->getCallable());
kelunik commented 1 year ago

It's something that works in amphp/injector then.