Skip to content

[5.6] Improve allowed method injection parameters#24234

Merged
taylorotwell merged 1 commit into
laravel:5.6from
mpociot:allow-overriding-classes-in-method-injection
May 24, 2018
Merged

[5.6] Improve allowed method injection parameters#24234
taylorotwell merged 1 commit into
laravel:5.6from
mpociot:allow-overriding-classes-in-method-injection

Conversation

@mpociot

@mpociot mpociot commented May 16, 2018

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

@nunomaduro

Copy link
Copy Markdown
Member

Seems pretty nice to me. 👍

@sisve

sisve commented May 17, 2018

Copy link
Copy Markdown
Contributor
  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

mpociot commented May 17, 2018

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
Member

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

function () use ($stub) {}

@taylorotwell

Copy link
Copy Markdown
Member

Ping @mpociot

@mpociot

mpociot commented May 23, 2018

Copy link
Copy Markdown
Contributor Author

@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

sisve commented May 23, 2018

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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