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

[5.6] Improve allowed method injection parameters #24234

Merged

Conversation

mpociot
Copy link
Contributor

@mpociot mpociot commented May 16, 2018

This PR adds the ability to pass a specific object instance to a method called from the container, regardless of the parameter name.

This can be useful in situations where you want to inject a specific object instance into a method call, no matter what name that parameter has.

@freekmurze
Copy link
Contributor

Pretty sweet, and exactly what I need for some stuff I'm working on 😄

@nunomaduro
Copy link
Member

Seems pretty nice to me. 👍

@sisve
Copy link
Contributor

sisve commented May 17, 2018

  1. It looks like only the first matching parameter of the specified type will be matched. Is this intentional?
  2. How do you handle the weird cases of conflicts where a parameter name also happens to match a class name? The syntax is identical to the caller...

@mpociot
Copy link
Contributor Author

mpociot commented May 17, 2018

@sisve

  1. It looks like only the first matching parameter of the specified type will be matched. Is this intentional?

Yeah that's correct. I'm not sure if we need to handle this logic. Until now, it wasn't possible to override specific hinted classes with method injection, so the case that someone needs to override two specific instances of the same class in combination with method injection could be quite rare.

  1. How do you handle the weird cases of conflicts where a parameter name also happens to match a class name? The syntax is identical to the caller...

Do we need to handle this extreme edge case?

The same edge case occurs right now when using the call method with a parameter that's also a class name that could be resolved from the container.

@taylorotwell
Copy link
Member

If you already know the function needs that instance... couldn't you have done...

function () use ($stub) {}

@taylorotwell
Copy link
Member

Ping @mpociot

@mpociot
Copy link
Contributor Author

mpociot commented May 23, 2018

@taylorotwell I don't quite understand what you mean. Could you explain it some more?

I want to be able to inject a specific object instance using method injection, but I have no idea what's the name of the parameter - I only know the type-hinted class.

For example, if you're developing a package that needs to inject a specific instance into a method, the only way to do it right now, is to tell people how they need to name the method properties.

Then we could pass an array of parameters with this exact parameter name.

With this PR, we would be able to say: Just typehint this class and it will be injected automatically and pass the parameters with the fully qualified class name - just like I did in the added test.

@sisve
Copy link
Contributor

sisve commented May 23, 2018

@mpociot Would it make sense to implement that as a parent/child container? Where a child container would forward unknown resolve attempts to the parent container. This would also solve scenarios where a dependency of a dependency of a dependency were to resolve the contract, instead of just the method you're calling.

$childContainer = new Container($parentContainer);
$childContainer->bind(Contract::class, Implementation::class);
$myClass = $childContainer->call([$myService, 'myMethod']);

@taylorotwell taylorotwell merged commit 49549b7 into laravel:5.6 May 24, 2018
@TBlindaruk
Copy link
Contributor

@taylorotwell ,should be documentation update?

If yes, @mpociot could you update the documentation?

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

Successfully merging this pull request may close these issues.

7 participants