Do not pass &$parts as reference to resolvePharParentPath #67
Conversation
|
offtopic question: I can see lot of PRs from your side, which is awesome. it seems you are planning to add php8 support to zf1? |
|
@staabm yes. it's the details all in mr body. click the link there and open the pandora box. |
|
php 5.4 flakyness: |
| public static function resolvePharParentPath($value, $key, $parts) | ||
| { | ||
| if ($value !== '...') { |
There was a problem hiding this comment.
I looked at this method and the logic does not make sense to me. The purpose of it was to remove current and previous segment in a path, where current segment was ...? Is triple-dots even ever used in phar paths? Moreover this method would be buggy if ... was at the beginning of a path.
If the original intention was to have .. there instead of ... then it would maybe make more sense... but the logic here to mutate $parts array passed by reference looks awkward and is forbidden in php8 while using array_walk. The change that $parts is no longer a reference is even worse now, because the method would basically do nothing in result in all cases incl. ... value.
It looks like a workaround for a super-rare edge case to me. The case with ... is also not tested. We could try to refactor it in a loop or something, but I think I'd get rid of this method completely.
There was a problem hiding this comment.
@falkenhawk but ths pr did not modify line including if ($value !== '...') {
There was a problem hiding this comment.
Yes, it removed reference from $parts which made the method obsolete, but I tried to figure out the original purpose of the method...
There was a problem hiding this comment.
Just noticed this #74 - it actually addresses the same issue.
But, I still wonder, why there is a condition for ... inside resolvePharParentPath - it just looks off to me.
Btw, when I change that to .. in #74 to actually allow the $parts array to be modified in tests, ...it crashes in tests for me locally on php 7.3 (win) with a weird error.
composer test tests/Zend/Loader/ClassMapAutoloaderTest.php
> phpunit "tests/Zend/Loader/ClassMapAutoloaderTest.php"
PHPUnit 3.7.38 by Sebastian Bergmann.
Configuration read from D:\projects\ovosdev\zf1s\phpunit.xml.dist
...........Script phpunit handling the test event returned with error code -1073741819
Per https://www.php.net/manual/en/function.array-walk.php :
Only the values of the array may potentially be changed; its structure cannot be altered, i.e., the programmer cannot add, unset or reorder elements. If the callback does not respect this requirement, the behavior of this function is undefined, and unpredictable.
The logic here is definitely not very safe 🙈
There was a problem hiding this comment.
I've moved everything but this change to separate PRs, so they won't suffer the delay by code review.
glensc
changed the title
Signed-off-by: Alexander Wozniak <wozniak@oktopos.com> Signed-off-by: Elan Ruusamäe <glen@pld-linux.org>
In PHP8 the third param of array_walk ($userdata) is automatically passed by value thus calling a function that expects pass by reference as the third param is not possible. Simply changing the array_walk to a foreach loop allows us to fulfill the pass by reference signature.
|
superseded by #111 |
Extracted change made by @Megatherium from #32