ext/zip: add ZipArchive::openString method by tstarling · Pull Request #21205 · php/php-src

tstarling · 2026-02-12T04:34:38Z

Add ZipArchive::openString().
For consistency with ::open(), return int|bool, don't throw an exception on error. Provide error information via existing properties and accessors.
Share buffer handling with ZipArchive::addFromString().
If there is an error from zip_close() during a call to ZipArchive::open(), emit a warning but proceed to open the archive, don't return early. Add test.
When buffers are saved by ZipArchive::addFromString(), release them in ZipArchive::close() and ::open(), don't accumulate buffers until the free_obj handler is called.
Increase width of integers buffers_cnt and filename_len

Co-authored-by: @shyim
Forked from #14078

ext/zip/php_zip.stub.php

ext/zip/php_zip.c

iluuu1994 · 2026-02-13T13:00:45Z

Just a heads-up: ext/zip doesn't currently have any active primary maintainers. I'm not sure who would be most qualified to review this. Maybe @devnexen or @ndossche? (Though note that @ndossche is a volunteer and busy with uni).

tstarling · 2026-02-13T22:24:53Z

Just a heads-up: ext/zip doesn't currently have any active primary maintainers. I'm not sure who would be most qualified to review this. Maybe @devnexen or @ndossche? (Though note that @ndossche is a volunteer and busy with uni).

@ndossche is definitely the most qualified and it would be great to have his review. My work here carries on from his ZipArchive::addFromString() commit in November.

Regarding this commit and future directions: the read-only restriction is somewhat arbitrary -- we could provide an interface allowing a zip archive to be modified in memory, although I'm not sure exactly what that would look like. Maybe there would be a getter for the modified string. Then openString() could take a $flags argument, like open(). If we want to do that, $flags should be introduced here, since ZipArchive::open() defaults to read/write mode, and it would make sense for openString to do that too.

tstarling · 2026-02-13T22:32:23Z

Since zip_close() does the writing, there's no point providing an accessor for the updated zip archive string before that is called. So maybe we could have ZipArchive::closeToString(), which closes the archive and returns it as a string.

We could then have a default for the first parameter of ZipArchive::openString to open an empty archive in read/write mode.

Girgias · 2026-02-15T13:22:03Z

ext/zip/php_zip.c

+	php_zipobj_add_buffer(ze_obj, buffer);
+	ze_obj->za = intern;
+	zip_error_fini(&err);
+	RETURN_TRUE;


Why not just return 0? Having a return type of int|true is incredibly weird.

For consistency with ZipArchive::open. #14078 proposed to throw exceptions instead, but @remicollet complained that it was inconsistent with ZipArchive::open, so here I am proposing to make it consistent. ZipArchive::open has a couple of error cases which can return false, but they don't apply here.

If you're prepared to take responsibility for this PR and hit the merge button when it meets your requirements, I'll make it return whatever you want.

For forwards compatibility and consistency with ZipArchive::open, it would be better if the method was declared as returning int|bool, not int|true. Callers should check for a false return in case some future version returns false. But I don't care enough to delay the merge. Daniel asked for int|true, so it returns int|true.

If there is a reason to use int|bool, i.e. because you want to be able to return false in some future version, I have no objections - I just wanted to make sure that if there wasn't a reason to have bool in the type, that it would be narrowed, so that people wouldn't check for false values unnecessarily - future compatibility is a valid reason

OK, I changed it to bool|int.

Signed-off-by: Soner Sayakci <s.sayakci@shopware.com>

Fixes to php#14078: * Rename ZipArchive::openBuffer() to ::openString(). * For consistency with ::open(), return int|bool, don't throw an exception on error. Provide error information via existing properties and accessors. * Fix memory leak when ::openString() is called but ::close() is not called. Add test. * Fix memory leak when a call to ::open() is followed by a call to ::openString(). Add test. * Let libzip own the source, don't call zip_source_keep(). * Share buffer handling with ZipArchive::addFromString(). Elsewhere: * If there is an error from zip_close() during a call to ZipArchive::open(), emit a warning but proceed to open the archive, don't return early. Add test. * When buffers are saved by ZipArchive::addFromString(), release them in ZipArchive::close() and ::open(), don't accumulate buffers until the free_obj handler is called. * Factor out buffer handling and reuse it in ZipArchive::openString()

ndossche · 2026-02-24T20:06:03Z

Pushed on my todo stack for wednesday.

ext/zip/tests/ZipArchive_open_with_close_fail.phpt

ext/zip/tests/ZipArchive_openString_leak.phpt

ext/zip/php_zip.c

ndossche · 2026-02-25T19:26:17Z

ext/zip/php_zip.c

+		RETURN_THROWS();
+	}
+
+	ze_zip_object *ze_obj = Z_ZIP_P(self);


I'd use Z_ZIP_P(ZEND_THIS) instead of storing ZEND_THIS in a variable.

Every method in this class uses this pattern. I can clean it up in a separate PR if you like. Maybe it makes sense for performance if ZEND_THIS is accessed more than once, but the original author decided to be consistent about it regardless of how many times it's accessed.

ext/zip/php_zip.c

tstarling requested a review from kocsismate as a code owner February 12, 2026 04:34

github-actions bot added the Extension: zip label Feb 12, 2026

tstarling mentioned this pull request Feb 12, 2026

Remove ifdef guards for ZIP_RDONLY #21195

Merged

DanielEScherzer reviewed Feb 12, 2026

View reviewed changes

ext/zip/php_zip.stub.php Show resolved Hide resolved

ext/zip/php_zip.c Outdated Show resolved Hide resolved

Girgias reviewed Feb 15, 2026

View reviewed changes

shyim and others added 4 commits February 16, 2026 12:54

ext/zip: add ZipArchive::openBuffer method

ab299ba

Signed-off-by: Soner Sayakci <s.sayakci@shopware.com>

throw value error instead of Error

8991220

Narrow return type to true|int

1d8a567

tstarling force-pushed the add-ziparchive-openbuffer branch from efbdd09 to 1d8a567 Compare February 16, 2026 01:56

ndossche self-requested a review February 24, 2026 20:04

ndossche requested changes Feb 25, 2026

View reviewed changes

tstarling added 2 commits February 26, 2026 09:16

Code review responses

5a83207

Fix safe_erealloc() call

d684590

devnexen reviewed Feb 26, 2026

View reviewed changes

ext/zip/php_zip.c Outdated Show resolved Hide resolved

Fix signedness mismatch and return bool|int

4073b1a

Conversation

tstarling commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iluuu1994 commented Feb 13, 2026

Uh oh!

tstarling commented Feb 13, 2026

Uh oh!

tstarling commented Feb 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndossche commented Feb 24, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

tstarling commented Feb 12, 2026 •

edited

Loading