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

[8.x] Resolving non-instantiables corrupts Container::$with #36212

Merged
merged 3 commits into from
Feb 10, 2021

Conversation

piurafunk
Copy link
Contributor

I ran into this issue when a constructor dependency is non-instantiable (such as an interface) and a following dependency is provided as a contextual property. My use case was when using an OpenAPI generated library. The class is essentially:

class OpenApi {
	public function __construct(ClientInterface $client = null, Configuration $config = null, HeaderSelector $selector = null, $host_index = 0)
	{
		$this->client = $client ?: new Client();
		$this->config = $config ?: new Configuration();
		$this->headerSelector = $selector ?: new HeaderSelector();
		$this->hostIndex = $host_index;
	}
}

This needs a Guzzlehttp client, a configuration object for request settings (HTTP host, auth data, etc), and a few other parameters. I didn't need to customize the Guzzlehttp client, however I was using the container's extend function to configure the Configuration object:

$this->app->extend(Configuration::class, function (Configuration $configuration) {
	return $configuration->setHost('...');
});

This all worked as expected, so far.

In my application, I have another class that consumes a 3rd party API via the OpenApi class, essentially:

class Consumer {
	public function __construct(OpenApi $api, Repository $repository)
	{
		$this->api = $api;
		$this->repository = $repository;
	}
}

When resolving the Consumer class, I usually allow the container to resolve the Repository class for me, but there are some instances that I need to provide it a certain setup. I do this like so:

$repository = new Repository();
// Customize the $repository as needed
$consumer = app()->make(Consumer::class, ['repository' => $repository]);

This is where I found the issue. When resolving the OpenApi class, the container correctly lists out ClientInterface, Configuration, HeaderSelector, and $host_index as dependencies, and it resolves them. However, because ClientInterface is not instantiable (\ReflectionClass::isInstantiable() is used on src/Illuminate/Container/Container.php:838) it throws an \Illuminate\Contracts\Container\BindingResolutionException, which is handled at src/Illuminate/Container/Container.php:986. Because the ClientInterface has a default value of null, the container returns that value and continues on its merry way.

The problem is that it does not clean up the Container::$with property correctly. When a dependency properly resolves, it eventually uses array_pop($this->with); as it climbs up the recursion. When the \Illuminate\Contracts\Container\BindingResolutionException occurs, it doesn't clean up the $with property. Then, when resolving subsequent parameters of the constructor (in this example, trying to resolve Repository for Consumer), the $with property has the wrong data and it fails to use the provided $repository object.

This PR provides a fix as well as a test that passes with the fix and fails without it.

As a side-effect, it also leaves 1 additional element in the $with array when all resolution is complete every time this occurs (probably not very frequently in the life-cycle of a request). And as far as I can tell, this doesn't impact any future resolutions, since each resolution just appends to the end of the $with array rather than reading anything already there.

@driesvints driesvints changed the title Resolving non-instantiables corrupts Container::$with [8.x] Resolving non-instantiables corrupts Container::$with Feb 10, 2021
@taylorotwell taylorotwell merged commit c086fc5 into laravel:8.x Feb 10, 2021
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.

2 participants