Implement GH-15711: Allow SoapClient to use the backing value during response serialization #15803
Conversation
| @@ -830,33 +831,34 @@ static zval *to_zval_hexbin(zval *ret, encodeTypePtr type, xmlNodePtr data) | |||
| return ret; | |||
| } | |||
|
|
|||
| static zend_string *get_serialization_string_from_zval(zval *data) | |||
There was a problem hiding this comment.
Should this have a "SOAP" prefix? I don't remember if SOAP uses them or not
There was a problem hiding this comment.
The other methods here don't do that, most methods in soap actually don't 🤷
|
I updated one commit to remove a dead break statement, and I added a commit adding support for int-backed enums as per #15711 (comment) |
I don't know, I think this is closer to a feature than a bugfix. |
…tring() For now this new function only returns a copy of the string, but its functionality will be expanded by later commits. to_xml_string() now uses this function and the memory management is simplified as well.
…ng response serialization (string enums)
…ng response serialization (int enums)
|
Had to rebase to make CI green again due to master failure |
| } | ||
| ZEND_FALLTHROUGH; | ||
| default: | ||
| return zval_get_long(data); |
There was a problem hiding this comment.
Is this guaranteed to be an integer string?
There was a problem hiding this comment.
No. It'll throw if that's not the case.
I would appreciate backport to 8.3. But if not, it's not a big deal. I'll just add TODOs to remove workarounds when 8.4 releases |
|
Afaik today is the last day we can still add features to 8.4. The question is whether this is a feature or a bugfix. Cc @SakiTakamachi @ericmann @NattyNarwhal |
|
@nielsdos @NattyNarwhal |
|
Unfortunately I just cut the release already. It might not be too late to get a feature in though, from the sounds of this comment, it might be possible to get it after beta but before RC, but I could be misreading it though. That said, I am sympathetic to call this a bug fix based on the original issue's description, since it's an inconsistency between places where you'd want to serialize values. |
|
I'll wait for Eric to see if he sees this too as a bugfix, and in that case there's no issue at all. Thanks for the feedback!
I am reading https://wiki.php.net/rfc/release_cycle_update and getting confused: One the one hand it states:
But it also states (under "Revised timeline"):
So this seems contradictory? |
I don't have a strong feeling either way, but this does feel like a bugfix. I'd like to see it land in both 8.3 and 8.4 though ... |
|
Thanks for the feedback, I'll reclassify the issue report and get this into 8.3 & master. |
See individual commits, some code had to be adapted first.