-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix #72374: ZipArchive::addGlob remove_path option strips first char of filename #1939
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
Conversation
FWIW: the failing check doesn't seem to be related to this commit. |
@@ -24,7 +24,7 @@ if (!$zip->open($file)) { | |||
exit('failed'); | |||
} | |||
$dir = realpath($dirname); | |||
$options = array('add_path' => 'baz/', 'remove_path' => $dir); | |||
$options = array('add_path' => 'baz', 'remove_path' => $dir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This necessary change hints at a potential BC break, so the PR should probably go to PHP 7.1 only (or maybe 8.0?). After all, this PHPT shows, that add_path and remove_path can be used together (albeit add_path needs the trailing slash, and remove_path must not have it).
Yes, this PR is a BC break. I found that if #1430 is merged, ZipArchive::addGlob called with |
No, master would be fine.
I just ran bug70103.phpt with this patch, and the test failed:
Can you please double-check that. |
BTW: the failing check is unrelated to this patch. |
I just want to say if PR #1430 is merged, before: after: https://gist.github.com/tyage/173cac00c035289bd4b55beada67d55d And this PR passes such test file. |
Ah, I understand now. Thanks. |
None of us can merge this while it has potential BC implications. Can I request that you start an internals discussion to try to gather consensus regarding targeting master with these changes. The discussion may lead to the creation of an RFC if no clear consensus emerges. |
Thank you for the heads up @krakjoe . I will try to do that. |
I posted a mail but there is no response. Is there any idea to start the discussion? |
@cmb69 what would you like to do here, internals seemed uninterested ? |
It seems to me that this could target PHP 8, but if in doubt, @remicollet should decide this. |
@cmb69 please can you make a move on this ... if 8 still seems reasonable to you, nobody objected, so ship it ... |
thank you so much! |
https://bugs.php.net/bug.php?id=72374
After adding a file by addGlob using add_path and remove_path options, the first character of the filename is stripped in the archive.
This patch fixed the issue.