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.

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

Do not pass &$parts as reference to resolvePharParentPath #67

Closed
wants to merge 3 commits into from
+10 −2
Closed

Do not pass &$parts as reference to resolvePharParentPath #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fee16ec
Add property in Zend_Loader_ClassMapAutoloaderTest
Nov 30, 2020
6bcaae7
Do not pass &$parts as reference to resolvePharParentPath
Nov 30, 2020
8377126
Changed call in ClassMapAutoloader to preserve original behaviour
Dec 3, 2020
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
7 changes: 5 additions & 2 deletions packages/zend-loader/library/Zend/Loader/ClassMapAutoloader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,10 @@ public static function realPharPath($path)
$prependSlash = $parts && $parts[0] === '' ? '/' : '';
$parts = array_values(array_filter($parts, array(__CLASS__, 'concatPharParts')));

array_walk($parts, array(__CLASS__, 'resolvePharParentPath'), $parts);
foreach ($parts as $key => $value) {
self::resolvePharParentPath($value, $key, $parts);
}

if (file_exists($realPath = 'phar://' . $prependSlash . implode('/', $parts))) {
return $realPath;
}
Expand All @@ -233,7 +236,7 @@ public static function concatPharParts($part)
* @param array $parts
* @return void
*/
public static function resolvePharParentPath($value, $key, &$parts)
public static function resolvePharParentPath($value, $key, $parts)
{
if ($value !== '...') {
Comment on lines +239 to 241
Copy link
Member

@falkenhawk falkenhawk Mar 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@falkenhawk but ths pr did not modify line including if ($value !== '...') {

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it removed reference from $parts which made the method obsolete, but I tried to figure out the original purpose of the method...

Copy link
Member

Choose a reason for hiding this comment

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

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 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved everything but this change to separate PRs, so they won't suffer the delay by code review.

return;
Expand Down
5 changes: 5 additions & 0 deletions tests/Zend/Loader/ClassMapAutoloaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@
*/
class Zend_Loader_ClassMapAutoloaderTest extends PHPUnit_Framework_TestCase
{
/**
* @var Zend_Loader_ClassMapAutoloader
*/
private $loader;

public static function main()
{
$suite = new PHPUnit_Framework_TestSuite(__CLASS__);
Expand Down