Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

tyage
Copy link
Contributor

@tyage tyage commented Jun 10, 2016

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.

@cmb69
Copy link
Member

cmb69 commented Jun 28, 2016

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);
Copy link
Member

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

@tyage
Copy link
Contributor Author

tyage commented Jul 8, 2016

Yes, this PR is a BC break.
(So, should I re-create the PR to be merged in PHP-7.1.0?)

I found that if #1430 is merged, ZipArchive::addGlob called with remove_path option (without add_path option) will produce the bug.
But this PR also resolves that issue.

@cmb69
Copy link
Member

cmb69 commented Jul 13, 2016

(So, should I re-create the PR to be merged in PHP-7.1.0?)

No, master would be fine.

I found that if #1430 is merged, […]
But this PR also resolves that issue.

I just ran bug70103.phpt with this patch, and the test failed:

002+ string(47) "/vagrant/php-src/ext/zip/tests/bug70103/foo.txt"
002- string(7) "foo.txt"

Can you please double-check that.

@cmb69
Copy link
Member

cmb69 commented Jul 13, 2016

BTW: the failing check is unrelated to this patch.

@tyage
Copy link
Contributor Author

tyage commented Jul 15, 2016

I just ran bug70103.phpt with this patch, and the test failed:

I just want to say if PR #1430 is merged, add_path option is not need to reporoduce the bug #72374.
So, the test file ext/zip/tests/bug72374.phpt can be modified:

before:

https://github.com/tyage/php-src/blob/f15eb7934c927d2d6c0662c4aa1a8a673b2279fd/ext/zip/tests/bug72374.phpt

after:

https://gist.github.com/tyage/173cac00c035289bd4b55beada67d55d

And this PR passes such test file.
Sorry for my bad English.

@cmb69
Copy link
Member

cmb69 commented Jul 15, 2016

Ah, I understand now. Thanks.

@smalyshev smalyshev added the Bug label Sep 5, 2016
@krakjoe
Copy link
Member

krakjoe commented Jan 3, 2017

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.

@tyage
Copy link
Contributor Author

tyage commented Jan 17, 2017

Thank you for the heads up @krakjoe . I will try to do that.

@tyage
Copy link
Contributor Author

tyage commented May 26, 2017

I posted a mail but there is no response.
http://news.php.net/php.internals/98422

Is there any idea to start the discussion?

@krakjoe
Copy link
Member

krakjoe commented Jan 30, 2019

@cmb69 what would you like to do here, internals seemed uninterested ?

@cmb69
Copy link
Member

cmb69 commented Jan 30, 2019

It seems to me that this could target PHP 8, but if in doubt, @remicollet should decide this.

@krakjoe
Copy link
Member

krakjoe commented Oct 2, 2019

@cmb69 please can you make a move on this ... if 8 still seems reasonable to you, nobody objected, so ship it ...

@cmb69
Copy link
Member

cmb69 commented Oct 2, 2019

Thanks for the reminder @krakjoe. And sorry that it took so long to address this PR @tyage!

Anyhow, thanks for the PR! Applied as 4d6f88e.

@cmb69 cmb69 closed this Oct 2, 2019
@tyage tyage deleted the PHP-7.0 branch October 2, 2019 14:16
@tyage
Copy link
Contributor Author

tyage commented Oct 2, 2019

thank you so much!

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

Successfully merging this pull request may close these issues.

5 participants