-
Notifications
You must be signed in to change notification settings - Fork 15.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PHP] allow encoding enums as integers for json #12707
base: main
Are you sure you want to change the base?
Conversation
The php extension already had an implementation for encoding enums as integers, but it was not exposed, so add params to Message::serializeToJsonString() to enable the behaviour. Since "preserve proto fieldnames" was already an (undocumented) boolean parameter to serializeToJsonString, I've continued that pattern by adding a second boolean param. Add the corresponding functionality to the native library, and add a test to verify the behavior.
upb_Status status; | ||
|
||
if (zend_parse_parameters(ZEND_NUM_ARGS(), "|b", | ||
&preserve_proto_fieldnames) == FAILURE) { | ||
if (zend_parse_parameters(ZEND_NUM_ARGS(), "|bb", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This uses two boolean parameters, since there was a single (undocumented and ineffective) boolean parameter already there in the extension. We could consider using a bitmask depending on whether it's considered safe to do so, which might make future extension easier?
eg $message->serializeToJsonString(JSON_ENCODE_FORMAT_ENUMS_AS_INTEGERS & JSON_ENCODE_PRESERVE_PROTO_FILENAMES);
Can I get any update on how this is progressing? I believe that it's been moved into google's internal systems and possibly being looked at there, but from the outside there's no sign of any activity. |
We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please add a comment. This PR is labeled |
Hi. This is still active, and awaiting work/feedback/something from a maintainer. |
We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please add a comment. This PR is labeled |
Still active. Please notice me. |
We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please add a comment. This PR is labeled |
Still active/required/wanted. Pretty please notice me. |
@anandolee can I get an update on where this PR is at, since there's no visibility of what's happening behind the scenes? Thanks! |
The php extension already had an implementation for encoding enums as integers, but it was not exposed, so add params to Message::serializeToJsonString() to enable the behaviour.
Since "preserve proto fieldnames" was already an (undocumented) boolean parameter of serializeToJsonString, I've continued that pattern by adding a second boolean param. Add the corresponding functionality to the native library, and add a test to verify the behavior.