Skip to content

[5.8] Track abstracts being resolved, fire resolving events once #26112

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

[5.8] Track abstracts being resolved, fire resolving events once #26112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d62acd7
Test resolving() callbacks being fired once for implementations
tomlankhorst Mar 26, 2018
dcc48f0
Track abstract resolve stack and inhibit resolving callback when is …
tomlankhorst Mar 26, 2018
0dcb68a
StyleCI
tomlankhorst Mar 26, 2018
7089b10
Missing array_pop
tomlankhorst Mar 26, 2018
3dad949
Check if resolve is pending when firing callbacks
tomlankhorst Mar 26, 2018
d50b2b3
More extensive test
tomlankhorst Mar 26, 2018
823677c
Performance improvement
tomlankhorst Oct 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion src/Illuminate/Container/Container.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,13 @@ class Container implements ArrayAccess, ContainerContract
*/
protected $buildStack = [];

/**
* The stack of de-aliased abstractions currently being resolved.
*
* @var array
*/
protected $resolveStack = [];

/**
* The parameter override stack.
*
Expand Down Expand Up @@ -636,6 +643,8 @@ protected function resolve($abstract, $parameters = [])
{
$abstract = $this->getAlias($abstract);

$this->resolveStack[] = $abstract;

$needsContextualBuild = ! empty($parameters) || ! is_null(
$this->getContextualConcrete($abstract)
);
Expand All @@ -644,6 +653,8 @@ protected function resolve($abstract, $parameters = [])
// just return an existing instance instead of instantiating new instances
// so the developer can keep using the same objects instance every time.
if (isset($this->instances[$abstract]) && ! $needsContextualBuild) {
array_pop($this->resolveStack);

return $this->instances[$abstract];
}

Expand Down Expand Up @@ -682,6 +693,7 @@ protected function resolve($abstract, $parameters = [])
$this->resolved[$abstract] = true;

array_pop($this->with);
array_pop($this->resolveStack);

return $object;
}
Expand Down Expand Up @@ -1048,7 +1060,7 @@ protected function getCallbacksForType($abstract, $object, array $callbacksPerTy
$results = [];

foreach ($callbacksPerType as $type => $callbacks) {
if ($type === $abstract || $object instanceof $type) {
if (($type === $abstract || $object instanceof $type) && ! $this->pendingInResolveStack($object)) {
$results = array_merge($results, $callbacks);
}
}
Expand All @@ -1070,6 +1082,24 @@ protected function fireCallbackArray($object, array $callbacks)
}
}

/**
* Check if object or a generalisation is pending in the resolve stack.
*
* @param object $object
*
* @return bool
*/
protected function pendingInResolveStack($object)
{
foreach ($this->resolveStack as $resolving) {
Copy link
Contributor

@lucasmichot lucasmichot Oct 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure we have to resolve the last element on each loop. I would make sense to extract it to a variable before.
Or even slicing the resolveStack.

Copy link
Contributor

@lucasmichot lucasmichot Oct 13, 2018

Choose a reason for hiding this comment

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

This would also work:

return collect($this->resolveStack)->slice(0, -1)->contains(function ($resolving) {
    $object instanceof $resolving;
});

Copy link
Contributor Author

@tomlankhorst tomlankhorst Oct 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It does, but it's a big performance hit.

phpbench run tests/Container/ContainerTest.php --revs=100
+-------------------------------------------------------+------------------+-------------------+
| subject                                               | tag:collect:mean | tag:original:mean |
+-------------------------------------------------------+------------------+-------------------+
| testResolvingCallbacksAreCalledForSpecificAbstracts   | 5,917.960μs      | 4,046.610μs       |
| testResolvingCallbacksAreCalledOnceForImplementations | 23,971.640μs     | 9,072.640μs       |
| testResolvingCallbacksAreCalledForNestedDependencies  | 99,831.230μs     | 61,386.800μs      |
+-------------------------------------------------------+------------------+-------------------+

original is this PR, collect is your alternative.

Note that the loop breaks through return as soon as a match is found.

if ($resolving !== end($this->resolveStack) && $object instanceof $resolving) {
return true;
}
}

return false;
}

/**
* Get the container's bindings.
*
Expand Down
93 changes: 93 additions & 0 deletions tests/Container/ContainerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,64 @@ public function testResolvingCallbacksAreCalledForType()
$this->assertEquals('taylor', $instance->name);
}

public function testResolvingCallbacksAreCalledOnceForImplementations()
{
$container = new Container;
$resolving_contract_invocations = 0;
$after_resolving_contract_invocations = 0;
$resolving_implementation_invocations = 0;
$after_resolving_implementation_invocations = 0;

$container->resolving(IContainerContractStub::class, function () use (&$resolving_contract_invocations) {
$resolving_contract_invocations++;
});
$container->afterResolving(IContainerContractStub::class, function () use (&$after_resolving_contract_invocations) {
$after_resolving_contract_invocations++;
});
$container->resolving(ContainerImplementationStub::class, function () use (&$resolving_implementation_invocations) {
$resolving_implementation_invocations++;
});
$container->afterResolving(ContainerImplementationStub::class, function () use (&$after_resolving_implementation_invocations) {
$after_resolving_implementation_invocations++;
});
$container->bind(IContainerContractStub::class, ContainerImplementationStub::class);
$container->make(IContainerContractStub::class);

$this->assertEquals(1, $resolving_contract_invocations);
$this->assertEquals(1, $after_resolving_contract_invocations);
$this->assertEquals(1, $resolving_implementation_invocations);
$this->assertEquals(1, $after_resolving_implementation_invocations);
}

public function testResolvingCallbacksAreCalledForNestedDependencies()
{
$container = new Container;
$resolving_dependency_interface_invocations = 0;
$resolving_dependency_implementation_invocations = 0;
$resolving_dependent_invocations = 0;

$container->bind(IContainerContractStub::class, ContainerImplementationStub::class);

$container->resolving(IContainerContractStub::class, function () use (&$resolving_dependency_interface_invocations) {
$resolving_dependency_interface_invocations++;
});

$container->resolving(ContainerImplementationStub::class, function () use (&$resolving_dependency_implementation_invocations) {
$resolving_dependency_implementation_invocations++;
});

$container->resolving(ContainerNestedDependentStubTwo::class, function () use (&$resolving_dependent_invocations) {
$resolving_dependent_invocations++;
});

$container->make(ContainerNestedDependentStubTwo::class);
$container->make(ContainerNestedDependentStubTwo::class);

$this->assertEquals(4, $resolving_dependency_interface_invocations);
$this->assertEquals(4, $resolving_dependency_implementation_invocations);
$this->assertEquals(2, $resolving_dependent_invocations);
}

public function testUnsetRemoveBoundInstances()
{
$container = new Container;
Expand Down Expand Up @@ -951,6 +1009,19 @@ public function testResolvingCallbacksShouldBeFiredWhenCalledWithAliases()
$this->assertEquals('taylor', $instance->name);
}

public function testInterfaceResolvingCallbacksShouldBeFiredWhenCalledWithAliases()
{
$container = new Container;
$container->alias(IContainerContractStub::class, 'foo');
$container->resolving(IContainerContractStub::class, function ($object) {
return $object->name = 'taylor';
});
$container->bind('foo', ContainerImplementationStub::class);
$instance = $container->make('foo');

$this->assertEquals('taylor', $instance->name);
}

public function testMakeWithMethodIsAnAliasForMakeMethod()
{
$mock = $this->getMockBuilder(Container::class)
Expand Down Expand Up @@ -1126,6 +1197,18 @@ public function __construct(IContainerContractStub $impl)
}
}

class ContainerDependentStubTwo
{
public $implA;
public $implB;

public function __construct(IContainerContractStub $implA, IContainerContractStub $implB)
{
$this->implA = $implA;
$this->implB = $implB;
}
}

class ContainerNestedDependentStub
{
public $inner;
Expand All @@ -1136,6 +1219,16 @@ public function __construct(ContainerDependentStub $inner)
}
}

class ContainerNestedDependentStubTwo
{
public $inner;

public function __construct(ContainerDependentStubTwo $inner)
{
$this->inner = $inner;
}
}

class ContainerDefaultValueStub
{
public $stub;
Expand Down