[7.x] Allow Contextual Binding outside of constructors

[wonderlandzor]

This PR fixes a long standing bug where Contextual Binding wouldn't work outside of the constructor of a class.

Notable relevant userland examples include the handle method of a Queue Job, and the injected services for a Controller.

As for the exact implementation here, I just added a new method which directly modifies the 'build stack', as that seemed to be the least intrusive way of getting stuff done. Initially I just passed along the extra parameter in makeFor, but that became messy very quickly.

I'm not sure wat this should be targeted to. It's a backwards compatible bugfix, but it also adds a 'new feature' in the sense that there's a new public method on the container. Any advice here would be welcome.

[devcircus]

To expand on the "thumbs down", I disagree that this is a bug. To me, contextual binding only makes sense for constructors. If anything this would be a new feature.

What if a class has multiple methods where the dependency is needed and the dependency that is needed, differs between the two? The context in "contextual-binding" is the class. It just seems confusing to use it in methods. 🤷

I guess if the implementation is solid and it doesn't cause any problems anywhere else, it doesn't matter to me either way. Just wanted to clarify the 👎.

[wonderlandzor]

@devcircus thanks for explaining your rationale.

What irks me with the current situation is that the logic behind DI isn't the same between constructors and the other cases where the framework is kind enough to help us out, Job@handle being the obvious one.

What if a class has multiple methods where the dependency is needed and the dependency that is needed, differs between the two? The context in "contextual-binding" is the class. It just seems confusing to use it in methods.

What would be the current behaviour if you needed two different implementations of a Concrete class in the constructor?

I think your issue could be solved in a different PR, after this one, if necessary, by providing ContextualBindingBuilder with an additional method:

$this->app->when('FooController@show')->needs(IBarService::class)->give(BarService::class)

or even

$this->app->when(FooController::class)->needs(IBarService::class)->in('show')->give(BarService::class)

Either way that's beyond the scope of this PR, but it's definitely a solvable problem.

[taylorotwell]

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

[wonderlandzor]

Hey @taylorotwell ,

I do feel the issue I'm trying to solve here is a bug within the framework. I'm personally a big fan of depending on contracts instead of concrete instances, and I love how Laravel provides contextual binding to help with that. However, at the few places where Laravel provides DI outside of the constructor (Controllers, Commands and Jobs amongst others).

The issue has been raised before (#23602, #29766, #8906, #6177) and I've been looking for a solution for a while now. You personally mentioned seeing the use case.

I'd love to have one without adding more public methods to the Container, but due to the way the 'build stack' works right now, I don't think that's possible without refactoring the container.

The current situation is at least inconsistent, but in my opinion especially in the case of Jobs quite annoying. I'll give a brief outline of the three cases I mentioned.

Jobs

Jobs don't work with contextual binding at all at the moment, because their constructor is used for Job payload data and therefore not available for DI. Right now my solution for these cases is to use a 'service locator' kind of approach, where there is a dedicated service for resolving the concrete instance necessary for the job at runtime, based on payload data. An abstract class depending on the shared contract between concrete instances feels like the better approach to me here.

Commands

Using contextual binding in Commands is possible by injecting in the constructor. Unfortunately, as those need to be resolved every time an artisan command is ran that comes at a performance loss when instantiating these services is an expensive operation. My go to to example is that some external API SDK packages have a habit of requiring stateful authentication.

Controllers

The Controllers have more or less the same issue Commands have, although the problem of performance impact is not quite as big as they're local per Controller. I like to inject a service in the methods of a controller if it's only used once or twice. For example, it doesn't make sense to include something you need for create/update routes in the show method, and by injecting it in the constructor, it will still be there.

[X-Coder264]

Jobs don't work with contextual binding at all at the moment, because their constructor is used for Job payload data and therefore not available for DI. Right now my solution for these cases is to use a 'service locator' kind of approach...

@nmokkenstorm Why don't you use the map method of the job dispatcher to separate your jobs/commands from the handlers?

That way you could use the contextual binding for your handler as you'd have the dependencies in the constructor.

[wonderlandzor]

Jobs don't work with contextual binding at all at the moment, because their constructor is used for Job payload data and therefore not available for DI. Right now my solution for these cases is to use a 'service locator' kind of approach...

@nmokkenstorm Why don't you use the map method of the job dispatcher to separate your jobs/commands from the handlers?

That way you could use the contextual binding for your handler as you'd have the dependencies in the constructor.

That's definitely an interesting way of tackling that specific example. Do you happen to have any modern (>Laravel 4) documentation for this?

The 'standard' Laravel jobs don't work correctly out of the box however, which was my original point.

[X-Coder264]

I wouldn't say that it's an interesting way of tackling this, I'd say that Laravel's way of making self-handling jobs by default is actually what's interesting/unusual. Such a concept is made just for DX/RAD purposes and doesn't exist in other frameworks (e.g. Symfony Messenger doesn't have such a concept, it has separate jobs/handlers as my example in the previous post with the only difference that you don't have to map them manually as in Laravel because Symfony is smart enough to read the job typehint from the handler method).

As far as documentation goes, I don't think that it was ever properly documented in Laravel, but the map method was added in 2016 to Laravel 5.3 via #14897 and was later added to the interface in 2018 in Laravel 5.6 via #22958.

[wonderlandzor]

I wouldn't say that it's an interesting way of tackling this, I'd say that Laravel's way of making self-handling jobs by default is actually what's interesting/unusual. Such a concept is made just for DX/RAD purposes and doesn't exist in other frameworks (e.g. Symfony Messenger doesn't have such a concept, it has separate jobs/handlers as my example in the previous post with the only difference that you don't have to map them manually as in Laravel because Symfony is smart enough to read the job typehint from the handler method).

I'm going to look into this, I don't know Symfony outside of a few individual components. I genuinely think this is a cool way of tackling this, maybe my choice of the word interesting was not the clearest. Thanks for bringing this to my attention.

Developer experience is the reason I finally decided to make this PR. I keep forgetting that you can't do contextual binding in Jobs, and the error message you get when Laravel gets confused about this is the reason there has been a few tickets re: this. I hope @taylorotwell will reconsider, seeing as replicating this core functionality behaviour in a package is going to be messy.

[deleugpn]

This for Jobs would be extremely handy.

[wonderlandzor]

This for Jobs would be extremely handy.

Jobs where my primary reason for adding this, but I don't think it's going to get in unfortunately :(

[Vusys]

Bumped into this with jobs as well. Given how powerful Laravel's container is, this seems like a bug or missing feature. It violates the principle of least surprise for me.

As a work around, I extended the class I want contextual bindings for and type hint that instead. Ugly, but works.

Extended class:

namespace App\Domain\Amazon;

class SqsClient extends \Aws\Sqs\SqsClient {}

Job:

public function handle(\App\Domain\Amazon\SqsClient $client)
{
    //...
}

Bindings in a service provider:

$this->app->singleton(\App\Domain\Amazon\SqsClient::class, function () {
	  return new \App\Domain\Amazon\SqsClient([
	  		//...
	  ]);
});

[wonderlandzor]

[...] this seems like a bug or missing feature. It violates the principle of least surprise for me.

Agreed, hence the PR. Nowadays I agree with the general sentiment that it's a (missing) feature and not a bug. The use case is mostly for controller methods and Job@handle I'd love to pick this up again for a recent version of Laravel but as you can see @taylorotwell closed it and I don't think it has been reconsidered elsewhere since.

[michael-rubel]

If someone wants this feature, it is available through a special proxy class that refers to the container, in the package:
https://github.com/michael-rubel/laravel-enhanced-container#contextual-binding-resolution-outside-of-constructor