[PHP] Serialize message to json add include default values and preserve fields name
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
View this failed invocation of the CLA check for more information.
For the most up to date status, view the checks section at the bottom of the pull request.
Also please rebase, the testing infrastructure has changed a lot in the last few weeks. Sorry for the delay on this PR.
Piotrze @s2x any updates on this?
Piotrze @s2x any updates on this?
Hello @knobik , I finally managed to find some time to finish it. Here is a list of fixes that have been added since the last commit:
Warning for boolean parameter in serializeToJsonString
Added a warning to users informing them about incorrect use of the boolean parameter in the serializeToJsonString function.
Unify preserve_proto_field_names and json_name
Unified behavior of preserve_proto_field_names and json_name options between php-ext and php, eliminating ambiguities. If json_name was provided in the proto file, it will always be used.
Adding the JsonSerializeOptions class to php-ext A new class, JsonSerializeOptions, has been introduced in php-ext, allowing more advanced configuration of JSON serialization options. Previously it was only in the composer package.
Test extension Unit tests have been expanded with new scenarios, covering the changes introduced, in order to more fully check their correctness.
This PR makes several changes to the semantics of JSON serialization. We have to be careful, because we have to keep these semantics the same across languages.
None of our other languages currently serialize default values when serializing to JSON. So I don't think we can make that change in PHP.
This PR also is changing the behavior of the upb JSON serializer, so that JSON name is always used if it is explicitly provided, even if a user asked for proto field names. We cannot change this behavior, as it would cause divergence from what other languages do.
For example, here is the C++ logic for preserve_proto_field_names. Note that it always uses the proto field name if the option is set; it doesn't look to see if a json_name was set: https://github.com/protocolbuffers/protobuf/blob/671b61b523dbec8bf8e41a61d5a8f689c3e2bb9f/src/google/protobuf/json/internal/unparser.cc#L409-L410
Hey, thanks for the quick reply. I read your comment, but I'm not sure if I understood it correctly.
This PR makes several changes to the semantics of JSON serialization. We have to be careful, because we have to keep these semantics the same across languages.
None of our other languages currently serialize default values when serializing to JSON. So I don't think we can make that change in PHP.
Unfortunately, I don't really understand what the problem is. To make the default values serialized, you must provide the parameter JsonSerializeOptions::EMIT_DEFAULTS in serializeToJsonString function. Without this parameter, only non-empty fields are serialized. It appears that this behavior is already implemented in other languages
- java: https://github.com/protocolbuffers/protobuf/blob/671b61b523dbec8bf8e41a61d5a8f689c3e2bb9f/java/util/src/test/java/com/google/protobuf/util/JsonFormatTest.java#L1486
- ruby: ???https://github.com/protocolbuffers/protobuf/blob/671b61b523dbec8bf8e41a61d5a8f689c3e2bb9f/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java#L703
- python: https://github.com/protocolbuffers/protobuf/blob/671b61b523dbec8bf8e41a61d5a8f689c3e2bb9f/python/google/protobuf/internal/json_format_test.py#L301
This PR also is changing the behavior of the upb JSON serializer, so that JSON name is always used if it is explicitly provided, even if a user asked for proto field names. We cannot change this behavior, as it would cause divergence from what other languages do.
For example, here is the C++ logic for
preserve_proto_field_names. Note that it always uses the proto field name if the option is set; it doesn't look to see if ajson_namewas set:https://github.com/protocolbuffers/protobuf/blob/671b61b523dbec8bf8e41a61d5a8f689c3e2bb9f/src/google/protobuf/json/internal/unparser.cc#L409-L410
Okay, I didn't know. I will change the code to be compatible with C++ and keep preserve_proto_field_names priority over json_names
It appears that this behavior is already implemented in other languages
You are right, I misremembered. If this is implementing the behavior already supported by the upb_JsonEncode_EmitDefaults flag in upb, that should be fine.
Hi @haberman, I hope you had a great weekend. I've been busy with a few tasks lately, but today I finally made the necessary changes.
I've incorporated your suggestions in the latest commit, ensuring that PHP code is now compatible with C++ code. Now the priority is to keep preserve_proto_field_names instead of json_names as you mentioned earlier.
If you could take a moment from your schedule, I would really appreciate it if you could review my latest commit.
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 inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.
We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active or becomes active again, please reopen it.
This PR was closed and archived because there has been no new activity in the 14 days since the inactive label was added.