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 · 2026-03-05T22:01:55Z

Are there any remaining issues blocking the merge of this PR?

ndossche · 2026-03-05T22:15:46Z

I'll have another look this weekend, I've just been busy with other things and not being super motivated to look after PHP.
You might want to shoot an email to internal in case this hasn't happened yet to see if there's any objections or concerns. If we had a maintainer for this extension then they could in principle decide on their own; but we don't have a dedicated person assigned for this.

tstarling · 2026-03-06T00:18:58Z

I sent an email to internals: https://www.mail-archive.com/internals@lists.php.net/msg122329.html

remicollet · 2026-03-06T06:48:39Z

If we had a maintainer for this extension

I'm terribly sad to read this.
AFAIK I'm still in the CREDITS and EXTENSIONS files

For memory, most zip features were developed in https://github.com/pierrejoye/php_zip, especially keeping libzip and ext/zip in peace.
This repository was also designed to make downstream packaging easier (updating ext in sync with libzip, rather than with php)
Also remind this extension is one with its own version (not using PHP one)

But because of lack of respect for this work, I'm alone to take care of this. And I'm tired, exhausted...

ndossche · 2026-03-06T08:05:59Z

If we had a maintainer for this extension

I'm terribly sad to read this. AFAIK I'm still in the CREDITS and EXTENSIONS files

(...)

But because of lack of respect for this work, I'm alone to take care of this. And I'm tired, exhausted...

This wasn't meant as a disrespect or anything like that. We recently talked internally how the EXTENSIONS file is terribly outdated and how it cannot be really used, some even suggesting dropping it. I see you're listed in there but only until 2020, so that's what I go off by. I think most of us mostly look at the CODEOWNERS file to determine active maintenance.
I know you maintain the external repository, and I do respect that.

tstarling · 2026-03-06T09:37:55Z

For memory, most zip features were developed in https://github.com/pierrejoye/php_zip, especially keeping libzip and ext/zip in peace. This repository was also designed to make downstream packaging easier (updating ext in sync with libzip, rather than with php) Also remind this extension is one with its own version (not using PHP one)

I can make a PR against that repo instead, assuming you can push it back to php-src when it's merged. Although I admit I don't understand the technical rationale for it. I see you're updating the Fedora package as we speak. Wikimedia is a Debian shop, I haven't touched Fedora packaging since 2004, so I can't meaningfully comment on it.

remicollet · 2026-03-06T10:27:33Z

Although I admit I don't understand the technical rationale for it

Such new feature will only be available to PHP version 8.6 with ext/zip users
But to all pecl/zip extension users, whatever PHP version

I can make a PR against that repo instead

Having review here is fine.
But PR in other repo welcome (after merge)

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

ndossche self-requested a review March 5, 2026 22:15

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!

tstarling commented Mar 5, 2026

Uh oh!

ndossche commented Mar 5, 2026

Uh oh!

tstarling commented Mar 6, 2026

Uh oh!

remicollet commented Mar 6, 2026

Uh oh!

ndossche commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tstarling commented Mar 6, 2026

Uh oh!

remicollet commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

tstarling commented Feb 12, 2026 •

edited

Loading

ndossche commented Mar 6, 2026 •

edited

Loading

remicollet commented Mar 6, 2026 •

edited

Loading