Framework: Primitive Contextual Bindings are ignored if parameter has class

[ragboyjr]

• Laravel Version: 5.5.40
• PHP Version: 7.1.14
• Database Driver & Version: N/A

Description:

When using the contextual binding for a primitive on the service container like:

$this->app->when(Service::class)->needs('$param1')->give(function() { /* resolve param 1 */ });

The contextual binding will be ignored if $param1 is type-hinted with a class.

// this will not work because $param1 is typehinted as a class
class Service
{
    public function __construct(OtherService $param1) {

    }
}

The code that configures this behavior is here: https://github.com/laravel/framework/blob/0200dc58ca805e86a366a5aaaa779870856b16f8/src/Illuminate/Container/Container.php#L826-L828

I feel like if there is any contextual binding (regardless if it's a class or not) the binding should be upheld.

Steps To Reproduce:

1. Create a class with a parameter typehinted to some class
2. Apply a primitive contextual binding
3. Try to access the service, and you'll receive an unresolvable entry exception

Use Case

In my specific situation, I have an ApiClient for magento2, and I'm registering multiple client instances in my service container based off of different credentials for different magento2 store instances. e.g. One api client setup for connecting to Store A and another for Store B.

My service class needs to inject both clients into the constructor, and they share the same class name, but differ in the variable name (which is why I was using the contextual bindings)

[deleugpn]

Your ApiClient doesn't seem to be very open/close. I'd say you need a single parameter that could act as a VO/Generics for your multiple instances.

[ragboyjr]

I appreciate the reply, but I'm not sure that it's completely relevant to this specific issue/problem I raised.

[ragboyjr]

After re-reading the docs, I think this would better fit as a feature request rather than a bug report.

The docs do mention that primitive and give an example of an int, so anything that's not a class, but at the same time, I do think it's a useful feature to extend the primitive binding to be able to contextually bind anything based off of variable name.

This is a feature of the symfony container system: http://symfony.com/doc/current/service_container.html#binding-arguments-by-name-or-type.

I only mention symfony to give validity to my feature request/usage, not trying to say that symfony is better/worse than Laravel.

I'm also willing to submit a PR for the above change, but don't want to go through the work if it's just going to be closed.

[devcircus]

The symfony feature that you mention seems to be a lot different than what you are proposing. Symfony, unless I'm misreading, doesn't let you go back-and-forth between primitives and objects on the same binding. It's still one or the other. Symfony utilizes an alias feature which allows you to alias an object to a string that represents the object. This doesn't seem to be the same thing.

[ragboyjr]

@devcircus that link i pasted is pointing to the wrong spot in the docs for some reason (sorry about that). If you scroll down, then You should see the title "Binding Arguments by Name or Type" which is the section I'm referring to.

This is the yaml configuration they provide as an example:

# config/services.yaml
services:
    _defaults:
        bind:
            # pass this value to any $adminEmail argument for any service
            # that's defined in this file (including controller arguments)
            $adminEmail: '[email protected]'

            # pass this service to any $requestLogger argument for any
            # service that's defined in this file
            $requestLogger: '@monolog.logger.request'

            # pass this service for any LoggerInterface type-hint for any
            # service that's defined in this file
            Psr\Log\LoggerInterface: '@monolog.logger.request'

But this yaml definition would be roughly equivalent to the following laravel:

$this->app->singleton('monolog.logger.request', function() { return RequestLogger(); });
$this->app->when(Service::class)->needs('$adminEmail')->give('[email protected]');
$this->app->when(Service::class)->needs(LoggerInterface::class)->give(function($c) { return $c->make('monolog.logger.request'); });
// this is the part that doesn't work currently if $requestLogger is typehinted
$this->app->when(Service::class)->needs('$requestLogger')->give(function($c) { return $c->make('monolog.logger.request'); });

[ragboyjr]

If I submitted a PR for this do you think it has a chance of being accepted? Don’t want to do the work if @taylorotwell plans on just closing it

[ragboyjr]

@deleugpn @devcircus ^

[deleugpn]

It's hard to say, I'd guess there's a 50/50 there. Since it's a change in behavior, it could mean breaking changes, which drops the chance of being merged. Taylor is not fond of implementing Framework solution for edge-cases such as this one.
On the other hand, I can see that the argument is not so bad: "If I explicitly bind something, respect it no matter what". The binding being explicit, it is a good argument that the Framework shouldn't trigger auto-wire.
I would suggest you go for it since it feels like it's a small change that won't require a lot of work. But it's likely that you'll have to target 5.7.
Side note: changes to the Application Container almost never gets merged without automation tests.

[ragboyjr]

Awesome @deleugpn, thanks for that info and for taking this time to reply!

[laurencei]

Hi there,

Welcome to Laravel and we are glad to have you as part of the community.

Unfortunately this GitHub area is not for ideas, suggestions etc. This is only for issues/bugs with the framework code itself.

I will be closing your ticket here. You are able to open a ticket at https://github.com/laravel/ideas

Alternatively you are able to open a PR using the Contributions guidelines: https://laravel.com/docs/5.7/contributions#which-branch

If you feel I've closed this issue in error, please provide more information about how this is a framework issue, and I'll reopen the ticket.

Thanks in advance.