Fix #70322: ZipArchive::close() doesn't indicate errors#1481
Fix #70322: ZipArchive::close() doesn't indicate errors#1481cmb69 wants to merge 2 commits intophp:PHP-5.6from
Conversation
If an archive can't be written, ZipArchive::close() nonetheless returns TRUE. We fix the return value to properly return success, and additionally raise a warning on failure.
|
Wrt. changing oo_delete.phpt: the error message would only occur on Windows: Rather interesting to see that some platform dependent programming errors are hidden by suppressing zip_close() failures. 😄 |
|
The failing check seems unrelated to this PR. |
|
It looks good. While I wish we had use exceptions :) For the phpt, any reason to silent the warning? |
IMHO, that would be too much of a BC break, at least for PHP 5.6.
For oo_delete.phpt ZipArchive::close() fails on Windows only. It seems to me that it's not worth to have platform dependent tests, and actually changing the test doesn't seem to be appropriate for this PR. |
|
On Sat, Aug 22, 2015 at 9:15 AM, Christoph M. Becker <
Pierre |
The issue is that libzip 0.11.2 calls remove() when an empty archive is closed, even though the file handle may still be open (it gets closed only in zip_discard()). Windows doesn't allow that operation, but Linux does. As of libzip 1.0.1 (probably already as of 1.0.0) this issue has been fixed, and so oo_delete.php passes also without the silence operator. Likewise, with libzip 1.0.1 the new test (bug70233.phpt) fails (Windows and Linux). I'll replace this test with a more portable and reliable one. |
|
Comment on behalf of cmb at php.net: |
3 participants
If an archive can't be written, ZipArchive::close() nonetheless returns TRUE.
We fix the return value to properly return success, and additionally raise a
warning on failure.
@pierrejoye @remicollet Okay to merge? Or should this be applied to pecl/zip first?