Fix alias resolution #221

[fhein]

Can you try and open a new PR just with the test and the proposed fix?

Of course. This PR provides tests and a bug fix for mapAliasesToTargets(). The fix does not cost performance. :)

[michalbundyra]

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

$stack = [];

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

[fhein]

unset is faster.

[michalbundyra]

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

[fhein]

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.

[michalbundyra]

@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

[fhein]

@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

[michalbundyra]

@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.

[fhein]

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

[michalbundyra]

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

foreach ($stack as $alias) {

[fhein]

'$_ => $alias' is faster.

[froschdesign]

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

[fhein]

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

[fhein]

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

[fhein]

Not addressed by the coding standards, it seems.

[froschdesign]

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.

[michalbundyra]

@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 😄

[fhein]

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.

[weierophinney]

Thanks, @fhein; merged to develop.