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

[5.4] Fix contextual bindings when instances and aliases are involved #17031

Merged
merged 6 commits into from Jan 4, 2017

Conversation

thecrypticace
Copy link
Contributor

@thecrypticace thecrypticace commented Dec 28, 2016

A resubmit of #15637 directed at 5.4.

Ping @taylorotwell, addressing your last comment from the PR:

So if something needs to be contextually built can it ever be bound as an instance after this PR? For example if I want a controller method to have SomeLogger implementation of the Logger interface, and SomeLogger is bound as an instance, will that be respected or will the container create a new instance every time?

Yes. It does respect it. I've added a test to ensure that only one instance of SomeLogger (or in the case of the test ContainerTestContextInjectInstantiations) is created and used during contextual binding if there is an instance available.

I couldn't come up with a better name for ContainerTestContextInjectInstantiations that followed the conventions of the other classes in the test case. Feel free to change it. :)

This doesn’t get reset before the test so every time `testExtendIsLazyInitialized` runs after the first time it fails.
If there is an instance in the container that is involved in contextual binding the binding would not be honored and the same instance would be always returned. If the instance was removed it would be. This fixes that.
@taylorotwell
Copy link
Member

Didn't I just fix this? Did my commit not fix this problem? I confirmed with your original logger example it was fixed.

@thecrypticace
Copy link
Contributor Author

I ran the all the tests I wrote a while ago (same ones in the PR) to confirm the problem was fixed and these did not pass:

testContextualBindingWorksForExistingInstancedBindings
testContextualBindingWorksForNewlyInstancedBindings
testContextualBindingWorksOnExistingAliasedInstances
testContextualBindingWorksOnNewAliasedInstances
testContextualBindingWorksOnNewAliasedBindings
testContextualBindingDoesntOverrideNonContextualResolution

@thecrypticace
Copy link
Contributor Author

The main thing that it needs is skipping the instance short circuit in make()

If I have a contextual binding for IContainerContractStub and IContainerContractStub exists as an instance it immediately returns the instance in the container and not the one given by the contextual binding.

Basically, with the way it works now, a container instance overrides all resolution and returns that one object for every request regardless of the registered contextual bindings.

Does this explanation help at all?

@thecrypticace
Copy link
Contributor Author

Furthermore, the reason the log code works with what you provided has to do with some of the refactoring.

On 5.3: https://github.com/laravel/framework/blob/5.3/src/Illuminate/Foundation/Bootstrap/ConfigureLogging.php#L41

On 5.4: https://github.com/laravel/framework/blob/master/src/Illuminate/Log/LogServiceProvider.php#L17

On 5.3 it is using instance (which this PR fixes contextual bindings for). On 5.4 it is using singleton (which your commit fixed contextual bindings for). So the problem still exists just not for the logger because the setup changed.

@@ -53,6 +53,7 @@ class Container implements ArrayAccess, ContainerContract
* @var array
*/
protected $aliases = [];
protected $aliasAbstracts = [];
Copy link
Member

Choose a reason for hiding this comment

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

Please add phpdoc.

@@ -351,6 +353,23 @@ public function instance($abstract, $instance)
}
}

private function removeAbstractAlias($searched)
Copy link
Member

Choose a reason for hiding this comment

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

Please make this protected, and add phpdoc.


class ContainerTestContextInjectInstantiations implements IContainerContractStub
{
public static $instantiations = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Does this work properly if you use the flag on phpunit that lets you run the tests multiple times?

Copy link
Member

Choose a reason for hiding this comment

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

If so, why does this variable get initialized here?

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 does not work. It's a left over from before I set it in the test itself. Thanks for catching this.

@taylorotwell
Copy link
Member

OK Thanks for feedback.

@GrahamCampbell GrahamCampbell changed the base branch from master to 5.4 December 29, 2016 22:18
@taylorotwell taylorotwell merged commit 96af646 into laravel:5.4 Jan 4, 2017
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