ext/zip: add ZipArchive::openBuffer method #14078
Conversation
shyim
force-pushed
the
add-ziparchive-openbuffer
branch
from
023e619 to
b9495a6
Compare
|
Hi php-team! It's possible to push this new feature into PHP-8.4 ? Thanks for reply! |
shyim
force-pushed
the
add-ziparchive-openbuffer
branch
from
ccd3914 to
db22fee
Compare
Signed-off-by: Soner Sayakci <[email protected]>
shyim
force-pushed
the
add-ziparchive-openbuffer
branch
from
db22fee to
87f690a
Compare
|
Hello, Appreciate your time! |
ext/zip/php_zip.c
Outdated
| zip_source_t * zip_source = zip_source_buffer_create(ZSTR_VAL(buffer), ZSTR_LEN(buffer), 0, &err); | ||
|
|
||
| if (!zip_source) { | ||
| zend_throw_error(NULL, "Cannot open zip: %s", zip_error_strerror(&err)); |
There was a problem hiding this comment.
converted to zend_value_error - hope this was the correct change to do.
ext/zip/php_zip.c
Outdated
| intern = zip_open_from_source(zip_source, ZIP_RDONLY, &err); | ||
| if (!intern) { | ||
| zip_source_free(zip_source); | ||
| zend_throw_error(NULL, "Cannot open zip: %s", zip_error_strerror(&err)); |
| bool(false) | ||
| bool(false) |
There was a problem hiding this comment.
Wouldn't be helpful to throw exceptions here instead of silent false?
There was a problem hiding this comment.
I'm not sure we should update the behaviour of addFromString/addEmptyDir to throw exceptions 🤔
There was a problem hiding this comment.
that would be bc and require an rfc i think, but @TimWolla can maybe say here more 🙈
There was a problem hiding this comment.
No, this will change the behavior of already existing methods
|
I'm not a big fan of this addition, which may raise huge memory needs (archives are usually big) I don't like the exception (value error), which makes this method inconsistent with the standard open I don't see what warranty the buffer will be available after the method call ? Also, if accepted, need to be applied to https://github.com/pierrejoye/php_zip |
The memory footprint here is indeed an issue, but I think there is still merit in having such functionality. In our usecase, we process small archives that we already have in memory but the current interface forces us to write them into disk then reread. In my opinion, we can try to mitigate this with few things:
I could align the returns to be consistent and not throw errors.
I will see what I can do to protect the buffer lifetime.
If you agree with these changes, I’ll go ahead and prepare them. |
There was a problem hiding this comment.
Since RM review was requested: we feel that this is not ready yet to include in 8.5 given the existing discussion, and trying to get it in before Tuesday would rush things. Marking as request changes to make it clear this should not merge until after 8.5 is branched
I was missing exactly that functionality around the time when the PR was opened. Functionality can always be used wrongly – that is in the hand of the developer. But, without this functionality, the only alternative to create in-memory archives is to utilize a ramdisk and/or hassle with file system accesses, adding unnecessary complexity, just to create an archive of 2 MB in size or whatever on machines which have hundreds of GB of RAM. In my opinion there is no loss in adding this functionality, but there are cases where exactly that functionality is the best solution. |
9 participants
Allow opening read-only zips by string.
An example use-case would be to download a zip file and without saving it to disk, to open it and get one file out of it.
See also #11594
It's my first PR, I run the test with also Valgrind. Did I miss something 🤔?