[5.4] [WIP] Fix container

[laurencei]

This is an attempt to fix #19175

The current getClosure() does the following call:

return $container->$method($concrete, $parameters);

But $method can only ever be make($abstract) or build($concrete). Both of these only take a single parameter as of ff993b8 - yet we are trying to pass in 2 parameters.

The problem is if you originally called makeWith() - and you end up down this path due to an interface being used, you end up with $parameters dropping off and not being passed along when make() is called here.

The limitation we have to work with is there cant be an interface change for 5.4. So we cant change make() to accept the second $parameters (which is probably the best solution here).

This PR switches the getClosure() to use the makeWith(), allowing the parameters to happily continue along until they are needed.

But - switching to makeWith() we create another issue: Application.php extends the make() function, to load any deferred providers. We need to also allow that to occur for makeWith()?

I've labelled this WIP - because although all the tests are green - I'm worried about unintended side effects here. Specifically would anyone have a custom implementation of Application that does not extend the Framework version? What would occur to their deferred providers in that instance?

I think we should get some people to test in real applications and see before we commit this.

The alternative is we leave the behavior as is for 5.4, and just fix it in 5.5 with a contract change, which will allow for a better and safer refactor, rather than doing this on a point release?

[taylorotwell]

Thanks

[mfn]

@laurencei for this fix, we were just hit with the bug that resolving with parameters did not load the deferred service provider.

PS: we verified that this change fixes it for us

@taylorotwell any chance cutting a release? 😄

[sebastiaanluca]

Could this somehow be related to the container discarding passed parameters in 5.5? Getting a BindingResolutionException because the container can't resolve the (primitive) values I'm passing and it's not getting.

app(ApiService::class, ['myrandomid']);

class ApiService {
    public function __construct(string $key)
    {
        //
    }
}

Using the new keyword is not possible since it's a dynamic call and some of those classes need to have their dependencies resolved automatically.

See also #17556 (comment). Was fixed in #18271 (discussion: laravel/ideas#391).

[laurencei]

@sebastiaanluca does the issue occur in the latest version of 5.4? I dont know why it would stop in 5.5 if it was working in 5.4?

[sebastiaanluca]

Verified this does not work in a clean 5.4 and 5.5 installation, even without type hints.

[Illuminate\Contracts\Container\BindingResolutionException]
Unresolvable dependency resolving [Parameter #0 [ <required> string $clientId ]] in class App\ApiService

Not sure why it stopped working, don't know what exactly changed in the mean time. I drilled down the chain of execution and noticed it never receives any parameters. They don't get explicitly passed (as you've mentioned in your opening post, the make and build methods only take one parameter and makeWith uses those) and retrieving the parameter using the getLastParameter and other methods returns no values.

I'll create a new issue, but I doubt I'll get an answer anytime soon. Lots of issues and little time unfortunately.

[deleugpn]

@sebastiaanluca Does it work if you remove the primitive type-hint?

[sebastiaanluca]

No, all passed parameters are discarded. See #21147.