Add ZipArchive::closeString() by tstarling · Pull Request #21497 · php/php-src

tstarling · 2026-03-23T05:33:58Z

Add a $flags parameter to ZipArchive::openString(), by analogy with ZipArchive::open(). This allows the string to be opened read/write.
Have the $data parameter to ZipArchive::openString() default to an empty string, for convenience of callers that want to create an empty archive. This works on all versions of libzip since the change in 1.6.0 only applied to files, it's opt-in for generic sources.
Add ZipArchive::closeString() which closes the archive and returns the resulting string. For consistency with openString(), return an empty string if the archive is empty.
Wrap strings passed to libzip with zip_source_function_create() instead of using zip_source_buffer_create(). This allows us to make the string writable, and simplifies memory management.

alecpl · 2026-03-23T07:23:25Z

Imo, either closeString() or saveToString().

tstarling · 2026-03-24T04:09:09Z

@ndossche It would be good to have your review on this.

@remicollet I considered making an openString() PR for the PECL module, but it doesn't merge cleanly and I'm touching the same lines of code again here, so I decided to do this PR first.

@alecpl I think closeString() would be fine, i.e. referring to the data model, instead of referring to what it returns. The new method can only be called when the underlying data is a string. But I'll give it a day to see if there are any other comments about this.

ndossche · 2026-03-28T11:50:52Z

@tstarling I'm sorry but I'm taking a step back from PHP, so someone else will need to review this.

tstarling · 2026-03-30T00:08:22Z

Imo, either closeString() or saveToString().

@alecpl I renamed it from closeToString() to closeString().

DanielEScherzer · 2026-04-02T16:13:21Z

-ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_class_ZipArchive_openString, 0, 1, MAY_BE_BOOL|MAY_BE_LONG)
-	ZEND_ARG_TYPE_INFO(0, data, IS_STRING, 0)
+ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_class_ZipArchive_openString, 0, 0, MAY_BE_BOOL|MAY_BE_LONG)
+	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, data, IS_STRING, 0, "\'\'")


hmm, @kocsismate does this need to be escaped in the arginfo? Not an actual suggestion to change in this patch, but just to document my thinking, why couldn't this be

Suggested change

ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, data, IS_STRING, 0, "\'\'")

ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, data, IS_STRING, 0, "''")

? (Just using this as an example since I'm not sure I've seen it before - since this is automatically generated please don't actually apply this suggestion @tstarling)

DanielEScherzer

generally looks good, I'll take another look after #21659 merges so it is easier to see what is changing in this patch

DanielEScherzer

thanks for splitting out #21659, makes this so much easier to review
a few suggestions, code mostly looks good

DanielEScherzer

generally looks good, I'll plan to test this a bit tomorrow and then hopefully merge it - thanks for bearing with me since I wasn't familiar with the extension

DanielEScherzer

Came back to this, looks ready to merge modulo the requested comment

- Add a $flags parameter to ZipArchive::openString(), by analogy with ZipArchive::open(). This allows the string to be opened read/write. - Have the $data parameter to ZipArchive::openString() default to an empty string, for convenience of callers that want to create an empty archive. This works on all versions of libzip since the change in 1.6.0 only applied to files, it's opt-in for generic sources. - Add ZipArchive::closeString() which closes the archive and returns the resulting string. For consistency with openString(), return an empty string if the archive is empty.

DanielEScherzer

Looks good to me - since I'm not too familiar with ext/zip I'll want to do another round of review with fresh eyes before merging but I don't see anything that needs to be changed

tstarling requested a review from kocsismate as a code owner March 23, 2026 05:33

github-actions Bot added Category: Build System Extension: zip labels Mar 23, 2026

tstarling force-pushed the ZipArchive-closeToString branch 2 times, most recently from 6ad9c14 to f8b8959 Compare March 23, 2026 06:12

tstarling force-pushed the ZipArchive-closeToString branch 2 times, most recently from 2c11df3 to a266aa3 Compare March 24, 2026 03:11

tstarling force-pushed the ZipArchive-closeToString branch 2 times, most recently from 09a25f1 to 283c75c Compare March 30, 2026 00:07

tstarling changed the title ~~Add ZipArchive::closeToString()~~ Add ZipArchive::closeString() Mar 30, 2026

DanielEScherzer reviewed Apr 1, 2026

View reviewed changes

tstarling force-pushed the ZipArchive-closeToString branch from 283c75c to 5eae17f Compare April 2, 2026 04:24

DanielEScherzer reviewed Apr 2, 2026

View reviewed changes

tstarling mentioned this pull request Apr 6, 2026

Wrap strings passed to libzip with zip_source_function_create() #21659

Merged

tstarling force-pushed the ZipArchive-closeToString branch from 5eae17f to de15d91 Compare April 7, 2026 00:14

github-actions Bot added the Category: CI label Apr 7, 2026

Ayesh removed the Category: CI label Apr 7, 2026

DanielEScherzer reviewed Apr 9, 2026

View reviewed changes

Comment thread ext/zip/php_zip.c Outdated

DanielEScherzer mentioned this pull request Apr 10, 2026

Fix GH-21682: Add NOT_SERIALIZABLE to ZipArchive, XMLWriter, XMLReader, SNMP, tidy, tidyNode #21694

Open

iliaal mentioned this pull request Apr 10, 2026

Fix GH-21682: Add NOT_SERIALIZABLE to ZipArchive #21708

Open

tstarling force-pushed the ZipArchive-closeToString branch from de15d91 to 248c02c Compare April 15, 2026 06:19

DanielEScherzer reviewed Apr 19, 2026

View reviewed changes

tstarling force-pushed the ZipArchive-closeToString branch 3 times, most recently from 59e42cd to 26e6159 Compare May 21, 2026 22:39

DanielEScherzer reviewed May 24, 2026

View reviewed changes

Comment thread ext/zip/php_zip.c

DanielEScherzer approved these changes May 27, 2026

View reviewed changes

tstarling force-pushed the ZipArchive-closeToString branch from 26e6159 to db5b845 Compare May 28, 2026 05:16

DanielEScherzer approved these changes May 29, 2026

View reviewed changes

	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, data, IS_STRING, 0, "\'\'")
	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, data, IS_STRING, 0, "''")

Conversation

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

Uh oh!

Uh oh!

alecpl commented Mar 23, 2026

Uh oh!

tstarling commented Mar 24, 2026

Uh oh!

ndossche commented Mar 28, 2026

Uh oh!

tstarling commented Mar 30, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanielEScherzer Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

DanielEScherzer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DanielEScherzer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanielEScherzer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DanielEScherzer left a comment

Choose a reason for hiding this comment

Uh oh!

DanielEScherzer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tstarling commented Mar 23, 2026 •

edited

Loading