Add ZipArchive::closeString()

[tstarling]

• 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]

Imo, either closeString() or saveToString().

[tstarling]

@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]

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

[tstarling]

Imo, either closeString() or saveToString().

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

[DanielEScherzer]

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]

if obj->out_str is not set, we should ensure that callers do not try to use the out_str, suggest adding

Suggested change

	}
	} else {
	ZEND_ASSERT(!out_str);
	}

[tstarling]

Done.

[tstarling]

I had to revert this, since the assertion fails when closeString() is called but something goes wrong with saving the archive. The caller is already guarding for that, and false is returned to userspace. There was already a test, which failed due to the assertion.