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

[6.x] Fix loading deferred providers for binding interfaces and implementations #31629

Conversation

lprzybylek
Copy link
Contributor

This PR is the effect of discussion in #31588

It brings back behaviour from Laravel 5.7 when you could use deferred provides to bind an interface to implementation even when implementation had its own deferred provider. Changing resolve method to make results in calling Illuminate\Foundation\Application::make method which loads deferred providers. Calling resolve method calls Illuminate\Container\Container::resolve which does not load deferred providers.

I'm targeting 6.x as it is stated in contribution guide, but in my opinion it may also be good to apply it to 5.8 for the sake of compatibility when migrating gradually from 5.7 to 5.8 and then to 6.x.

This commit brings back behaviour from Laravel 5.7 when you could use deferred provides to bind an interface to implementation even when implementation had its own deferred provider. Changing `resolve` method to `make` results in calling `Illuminate\Foundation\Application::make` method which loads deferred providers. Calling `resolve` method calls `Illuminate\Container\Container::resolve` which does not load deferred providers.
@@ -261,7 +261,7 @@ protected function getClosure($abstract, $concrete)
return $container->build($concrete);
}

return $container->resolve(
return $container->make(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The make method is just an alias of resolve. This can't solve anything really.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It solves the issue. Try checking out previous commit where test was added. Resolve is an alias of make in Illuminate\Container\Container, but not in Illuminate\Foundation\Application.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@driesvints it actually could in the context of an application because the Foundation Application overrides make to do the deferred provider stuff.

* @return mixed
*
* @throws \Illuminate\Contracts\Container\BindingResolutionException
*/
public function make($abstract, array $parameters = [])
public function make($abstract, array $parameters = [], $raiseEvents = true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the method signature here is unfortunately a breaking change.

@taylorotwell
Copy link
Member

Why all the raiseEvents stuff? Is that necessary?

@driesvints
Copy link
Member

The problem here really is that the things happening in Application::make should probably also happen in Application::resolve. From a container perspective there shouldn't be a difference between Container::make and Container::resolve.

@lprzybylek
Copy link
Contributor Author

Why all the raiseEvents stuff? Is that necessary?

To be honest I don't know how $raiseEvents affects the application. getClosure was using $raiseEvents = false and I didn't see any other way to run it with the same value than changing a signature and passing it over. Do you have any other ideas?

@taylorotwell
Copy link
Member

taylorotwell commented Feb 27, 2020

One solution may be to make the extra stuff in Application make to a new Application resolve method which calls that same code and then parent::resolve like @driesvints said. That would probably let you do this without any breaking changes?

It allows resolving object by interface when using separate deferred providers for interface and implementation.
@lprzybylek
Copy link
Contributor Author

Good idea, I've updated the PR. Tests are OK and signatures are intact.

@taylorotwell taylorotwell merged commit cba2b53 into laravel:6.x Feb 28, 2020
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.

None yet

3 participants