Skip to content
This repository has been archived by the owner on Feb 6, 2020. It is now read-only.

Fix alias resolution #221 #230

Merged
merged 5 commits into from
Jan 22, 2018
Merged

Fix alias resolution #221 #230

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f0f2cd3
Merge pull request #1 from zendframework/develop
fhein Jan 17, 2018
2413112
Tests and fix for mapAliasesToTargets as of #221.
fhein Jan 18, 2018
7bf7b8a
Missing word in source doc.
fhein Jan 18, 2018
a359426
Removed $_.
fhein Jan 18, 2018
886b6f9
Switched mapAliasesToTargets from isset() to empty() mechanics.
fhein Jan 19, 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
35 changes: 32 additions & 3 deletions src/ServiceManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,12 @@ private function mapAliasToTarget($alias, $target)
*
* This function maps $this->aliases in place.
*
* This algorithm is an adaptated version of Tarjans Strongly
* Connected Components. Instead of returning the strongly
* connected components (i.e. cycles in our case), we throw.
* If nodes are not strongly connected (i.e. resolvable in
* our case), they get resolved.
*
* This algorithm is fast for mass updates through configure().
* It is not appropriate if just a single alias is added.
*
Expand All @@ -881,17 +887,40 @@ private function mapAliasesToTargets()
if (isset($tagged[$alias])) {
continue;
}

$tCursor = $this->aliases[$alias];
$aCursor = $alias;
if ($aCursor === $tCursor) {
throw CyclicAliasException::fromCyclicAlias($alias, $this->aliases);
}
if (! isset($this->aliases[$tCursor])) {
continue;
}

while (isset($this->aliases[$tCursor])) {
$tagged[$aCursor] = true;
$this->aliases[$aCursor] = $this->aliases[$tCursor];
$stack[] = $aCursor;
if ($aCursor === $this->aliases[$tCursor]) {
throw CyclicAliasException::fromCyclicAlias($alias, $this->aliases);
}
$aCursor = $tCursor;
$tCursor = $this->aliases[$tCursor];
if ($aCursor === $tCursor) {
}

$tagged[$aCursor] = true;

if (! isset($stack)) {
Copy link
Member

Choose a reason for hiding this comment

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

I would set empty array to $stack before while loop:

$stack = [];

and then we can remove this condition and also unset in line 923.

Copy link
Author

Choose a reason for hiding this comment

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

unset is faster.

Copy link
Member

Choose a reason for hiding this comment

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

@fhein so you'd like to say that isset + unset is faster than setting empty array and foreach loop on empty array?

Copy link
Author

@fhein fhein Jan 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. empty($var) is an equivalent for !isset($var) || $var == false. isset is slightly faster than empty.

$stack = [] would be done on every loop, while unset is done only if $stack was used.

Copy link
Member

Choose a reason for hiding this comment

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

@fhein not sure if you understand me correctly. I know what is empty and what is the difference between empty and isset. My suggestion was to have:

$stack = []; // <-- add this line
while (isset($this->aliases[$tCursor])) {
  // ... 
}

$tagged[$aCursor] = true;

foreach ($stack as $alias) {
  // ...
}

instead of:

while (isset($this->aliases[$tCursor])) {
  // ... 
}
 
$tagged[$aCursor] = true;

if (! isset($stack)) { // <-- remove this if statement
  continue;
}

foreach ($stack as $alias) {
  // ...
}

unset($stack); // <-- remove this line

Copy link
Author

@fhein fhein Jan 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@fhein not sure if you understand me correctly. I know what is empty and what is the difference between empty and isset. My suggestion was to have:

I understand you correctly. See me annotations to your sample.

$stack = []; // <-- add this line  <- would be done on every iteration
while (isset($this->aliases[$tCursor])) {
      // ... 
}

$tagged[$aCursor] = true;

foreach ($stack as $alias) {
      // ...
}
instead of:

while (isset($this->aliases[$tCursor])) {
    // ... 
}

$tagged[$aCursor] = true;

foreach ($stack as $alias) {
   // ...
}
unset($stack); // <-- remove this line

Copy link
Member

Choose a reason for hiding this comment

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

@fhein

if (! isset($stack)) { // <-- must be replaced with if ( empty($stack)

no, can be removed at all, because then we will have foreach on an empty array, and unset will be no needed.

Copy link
Author

Choose a reason for hiding this comment

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

No performance impact following your suggestion on PHP 7.x. I'll do.

continue;
}

foreach ($stack as $_ => $alias) {
Copy link
Member

Choose a reason for hiding this comment

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

If we are not using indexes from the array we can skip them in the foreach:

foreach ($stack as $alias) {

Copy link
Author

Choose a reason for hiding this comment

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

'$_ => $alias' is faster.

Copy link
Member

Choose a reason for hiding this comment

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

@weierophinney and @webimpress
$_ looks weird and it has no meaning. Is this allowed in our coding standard?

Copy link
Author

Choose a reason for hiding this comment

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

At least as of PHP 5.x foreach with explicit $key => $value is always faster.

Copy link
Author

Choose a reason for hiding this comment

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

As of 7.x it doesn't seem to make sifnificant difference.

Copy link
Author

Choose a reason for hiding this comment

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

Not addressed by the coding standards, it seems.

Copy link
Member

Choose a reason for hiding this comment

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

We are humans and we want / must read the code. This type of variable name says nothing and there is also no comment to explain the current usage.
I'm not against a performance boost, but I'm against useless variable names.

Copy link
Member

Choose a reason for hiding this comment

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

@froschdesign

$_ looks weird and it has no meaning. Is this allowed in our coding standard?

with new zend-coding-standard we will get an error:

Variable _ is not in valid camel caps format

Glad we got rid of this useless variable 😄

Copy link
Author

Choose a reason for hiding this comment

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

And what about foreach( $key as $_) ?

The current usage of $_ is to indicate that this variable will not get accessed in the loop.

Unused, yes. Useless, no.

if ($alias === $tCursor) {
throw CyclicAliasException::fromCyclicAlias($alias, $this->aliases);
}
$this->aliases[$alias] = $tCursor;
$tagged[$alias] = true;
}

unset($stack);
}
}

Expand Down
24 changes: 24 additions & 0 deletions test/CommonServiceLocatorBehaviorsTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,4 +855,28 @@ public function testCrashesOnCyclicAliases()
],
]);
}

public function testMinimalCyclicAliasDefinitionShouldThrow()
{
$sm = $this->createContainer([]);

$this->expectException(CyclicAliasException::class);
$sm->setAlias('alias', 'alias');
}

public function testCoverageDepthFirstTaggingOnRecursiveAliasDefinitions()
{
$sm = $this->createContainer([
'factories' => [
stdClass::class => InvokableFactory::class,
],
'aliases' => [
'alias1' => 'alias2',
'alias2' => 'alias3',
'alias3' => stdClass::class,
],
]);
$this->assertSame($sm->get('alias1'), $sm->get('alias2'));
$this->assertSame($sm->get(stdClass::class), $sm->get('alias1'));
}
}