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.

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

Get realpath for ZipArchive #11636

Merged
merged 3 commits into from
Sep 13, 2023
Merged

Conversation

jasonmccreary
Copy link
Contributor

I came across a few open issues, notably #10099, and the fix commented by @Seldaek resolved the issue. However, it has yet to be PR'd since Satis is now replaced by Private Packagist.

Nonetheless, here is the PR to resolve the issue where the archive paths are incorrect due to ZipArchive::archive not using the realpath(). Notably on MacOS where sys_get_temp_dir() returns a symlinked path. This change is inline with PharArchive::archive as it uses realpath() already.

There are other instances in the codebase which use sys_get_temp_dir(). However, since they are only used for storage, and not pathing information, I don't believe they require changes.

@jasonmccreary jasonmccreary changed the title Get realpath for archive Get realpath for ZipArchive Sep 12, 2023
@jasonmccreary
Copy link
Contributor Author

Not sure the appropriate fix here for PHPStan as checking for false requires some kind of action, e.g. throwing an exception or using $sources as is.

@stof
Copy link
Contributor

stof commented Sep 12, 2023

I suggest using realpath($sources) ?: $sources. If the resolution of the realpath fails, we should keep using the original one.

@jasonmccreary
Copy link
Contributor Author

Haha, looks like PHPStan doesn't like short ternary.

@Seldaek Seldaek added this to the 2.6 milestone Sep 13, 2023
@Seldaek Seldaek added the Bug label Sep 13, 2023
@Seldaek Seldaek merged commit 1e4966c into composer:main Sep 13, 2023
19 of 20 checks passed
@Seldaek
Copy link
Member

Seldaek commented Sep 13, 2023

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants