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: Add support for primitive types in setters #5126

Merged
merged 21 commits into from Oct 7, 2018

Conversation

Projects
None yet
5 participants
@michaelbausor
Contributor

michaelbausor commented Sep 7, 2018

This PR adds support for automatic conversion from PHP native types to protobuf wrapper types in message setters. Still pending:

  • Support for repeated fields
  • PHPDoc updates

@TeBoring PTAL and let me know if this is the approach you had in mind

@googlebot googlebot added the cla: yes label Sep 7, 2018

@TeBoring TeBoring self-assigned this Sep 7, 2018

@TeBoring TeBoring added the php label Sep 7, 2018

@fiboknacky

This comment has been minimized.

Show comment
Hide comment
@fiboknacky

fiboknacky Sep 9, 2018

Will there be a support for getters (e.g., via other names) too?

fiboknacky commented Sep 9, 2018

Will there be a support for getters (e.g., via other names) too?

@michaelbausor

This comment has been minimized.

Show comment
Hide comment
@michaelbausor

michaelbausor Sep 11, 2018

Contributor

@TeBoring @fiboknacky PTAL
Regarding offline discussion of boxing/unboxing - I have put the boxing logic for setters into a method in a new class, GPBWrapperUtils. This would be my personal preference instead of generating this code inline in every setter, since it is not completely trivial like the unboxing logic. But let me know if you prefer to have this generated inline as well, and I can do that instead.

Contributor

michaelbausor commented Sep 11, 2018

@TeBoring @fiboknacky PTAL
Regarding offline discussion of boxing/unboxing - I have put the boxing logic for setters into a method in a new class, GPBWrapperUtils. This would be my personal preference instead of generating this code inline in every setter, since it is not completely trivial like the unboxing logic. But let me know if you prefer to have this generated inline as well, and I can do that instead.

@TeBoring

This comment has been minimized.

Show comment
Hide comment
@TeBoring

TeBoring Sep 12, 2018

Contributor

For setter, can we also create setXXXValue? So that the original setter doesn't need to be complicated. The new setter will also looks cleaner.

Contributor

TeBoring commented Sep 12, 2018

For setter, can we also create setXXXValue? So that the original setter doesn't need to be complicated. The new setter will also looks cleaner.

@fiboknacky

This comment has been minimized.

Show comment
Hide comment
@fiboknacky

fiboknacky Sep 12, 2018

@TeBoring @fiboknacky PTAL
Regarding offline discussion of boxing/unboxing - I have put the boxing logic for setters into a method in a new class, GPBWrapperUtils. This would be my personal preference instead of generating this code inline in every setter, since it is not completely trivial like the unboxing logic. But let me know if you prefer to have this generated inline as well, and I can do that instead.

I'm not quite familiar with this part of the lib. Will it affect how we can use setXXXValue and getXXXValue?
If not, then SGTM.

fiboknacky commented Sep 12, 2018

@TeBoring @fiboknacky PTAL
Regarding offline discussion of boxing/unboxing - I have put the boxing logic for setters into a method in a new class, GPBWrapperUtils. This would be my personal preference instead of generating this code inline in every setter, since it is not completely trivial like the unboxing logic. But let me know if you prefer to have this generated inline as well, and I can do that instead.

I'm not quite familiar with this part of the lib. Will it affect how we can use setXXXValue and getXXXValue?
If not, then SGTM.

@michaelbausor

This comment has been minimized.

Show comment
Hide comment
@michaelbausor

michaelbausor Sep 12, 2018

Contributor

For setter, can we also create setXXXValue? So that the original setter doesn't need to be complicated. The new setter will also looks cleaner.

I am not necessarily opposed to a setXXXValue method, but on reflection I think there are some downsides we should consider: null handling, and constructor arguments.

For null handling, we have two options: either setXXXValue(null) can be an error, or setXXXValue(null) will be the same as calling setXXX(null).
I think having setXXXValue(null) be an error would be a really bad user experience, because then users have to know to switch back and forth between the setXXX method and setXXXValue method based on whether they have a null value or not.
The alternative of having setXXXValue(null) just call setXXX(null) is better, but might be confusing because it is subtly different from the case of a non-null value. For a non-null value, setXXXValue($x) will construct a wrapper type and set its value to $x, so users might assume that setXXXValue(null) will construct a wrapper type and set its value to null, but this is not true and in fact is impossible (which is why wrapper types exist in the first place).

For constructor arguments, we would like to allow this usage:

// Old usage
$myMessage = new MyMessage(['double_value' => new DoubleValue(['value' => 1.1])]);
// New usage
$myMessage = new MyMessage(['double_value' => 1.1]);

With the current implementation allowing the setXXX method to accept both the wrapper message type and the primitive type, then passing primitive values into the constructor "just works" - I have added tests for this in the latest commit.

This is not an argument against adding the setXXXValue method, but if we don't support accepting the primitive value in the existing setXXX setter then we will also need to make other changes to support the usage in the constructor, e.g. in Message::mergeFromArray, and it is not clear how we could do that without extra complexity.

I'm not quite familiar with this part of the lib. Will it affect how we can use setXXXValue and getXXXValue?
If not, then SGTM.

This should only affect the implementation, not how it is used.

Contributor

michaelbausor commented Sep 12, 2018

For setter, can we also create setXXXValue? So that the original setter doesn't need to be complicated. The new setter will also looks cleaner.

I am not necessarily opposed to a setXXXValue method, but on reflection I think there are some downsides we should consider: null handling, and constructor arguments.

For null handling, we have two options: either setXXXValue(null) can be an error, or setXXXValue(null) will be the same as calling setXXX(null).
I think having setXXXValue(null) be an error would be a really bad user experience, because then users have to know to switch back and forth between the setXXX method and setXXXValue method based on whether they have a null value or not.
The alternative of having setXXXValue(null) just call setXXX(null) is better, but might be confusing because it is subtly different from the case of a non-null value. For a non-null value, setXXXValue($x) will construct a wrapper type and set its value to $x, so users might assume that setXXXValue(null) will construct a wrapper type and set its value to null, but this is not true and in fact is impossible (which is why wrapper types exist in the first place).

For constructor arguments, we would like to allow this usage:

// Old usage
$myMessage = new MyMessage(['double_value' => new DoubleValue(['value' => 1.1])]);
// New usage
$myMessage = new MyMessage(['double_value' => 1.1]);

With the current implementation allowing the setXXX method to accept both the wrapper message type and the primitive type, then passing primitive values into the constructor "just works" - I have added tests for this in the latest commit.

This is not an argument against adding the setXXXValue method, but if we don't support accepting the primitive value in the existing setXXX setter then we will also need to make other changes to support the usage in the constructor, e.g. in Message::mergeFromArray, and it is not clear how we could do that without extra complexity.

I'm not quite familiar with this part of the lib. Will it affect how we can use setXXXValue and getXXXValue?
If not, then SGTM.

This should only affect the implementation, not how it is used.

@TeBoring

This comment has been minimized.

Show comment
Hide comment
@TeBoring

TeBoring Sep 12, 2018

Contributor
Contributor

TeBoring commented Sep 12, 2018

@TeBoring

This comment has been minimized.

Show comment
Hide comment
@TeBoring

TeBoring Sep 12, 2018

Contributor
Contributor

TeBoring commented Sep 12, 2018

@TeBoring

This comment has been minimized.

Show comment
Hide comment
@TeBoring

TeBoring Sep 12, 2018

Contributor
Contributor

TeBoring commented Sep 12, 2018

@michaelbausor

This comment has been minimized.

Show comment
Hide comment
@michaelbausor

michaelbausor Sep 12, 2018

Contributor

If we do add a setXXXValue method, WDYT about whether or not to support accepting the primitive type in setXXX? If we don't, do you have a suggestion about how to implement the changes for the constructor argument? I haven't looked into what I think the best approach will be.

I think setXXX already report error when given null

This is true for primitive types, but not for message types such as DoubleValue and other wrapper types, where null is a valid value.

Contributor

michaelbausor commented Sep 12, 2018

If we do add a setXXXValue method, WDYT about whether or not to support accepting the primitive type in setXXX? If we don't, do you have a suggestion about how to implement the changes for the constructor argument? I haven't looked into what I think the best approach will be.

I think setXXX already report error when given null

This is true for primitive types, but not for message types such as DoubleValue and other wrapper types, where null is a valid value.

@TeBoring

This comment has been minimized.

Show comment
Hide comment
@TeBoring

TeBoring Sep 12, 2018

Contributor

https://github.com/protocolbuffers/protobuf/blob/master/php/src/Google/Protobuf/Internal/Message.php#L990
I think we can add a try catch here. If an exception is thrown, we check whether the field to be set is a wrapper message. If so, we do the setXXXValue.

Contributor

TeBoring commented Sep 12, 2018

https://github.com/protocolbuffers/protobuf/blob/master/php/src/Google/Protobuf/Internal/Message.php#L990
I think we can add a try catch here. If an exception is thrown, we check whether the field to be set is a wrapper message. If so, we do the setXXXValue.

@michaelbausor

This comment has been minimized.

Show comment
Hide comment
@michaelbausor

michaelbausor Sep 12, 2018

Contributor

cc-ing @AnashOommen for comment

Contributor

michaelbausor commented Sep 12, 2018

cc-ing @AnashOommen for comment

@fiboknacky

This comment has been minimized.

Show comment
Hide comment
@fiboknacky

fiboknacky Sep 13, 2018

Is there any way to:

  • Support setting primitive type in constructor argument $myMessage = new MyMessage(['double_value' => 1.1]);
  • Add both setXXXValue and getXXXValue and separate their usages from setXXX and getXXX

I see your concerns about using null with setXXXValue now. IMHO, it would be better not to support using null with setXXXValue at all.
It'd be misleading if setXXX(null) is equal to setXXXValue(null).

As for supporting primitive value in setXXX(), I'm quite neutral to this with a bit inclination to not support it.
The reason is that it'd be clearer to separate the use of setXXX and setXXXValue.
(Needing to type one more word "Value" after the original method names doesn't seem bad to me)

If we sometimes allow some kinds of arguments in both methods, but in other times, disallow other kinds of arguments in one methods, it'd be pretty confusing.

fiboknacky commented Sep 13, 2018

Is there any way to:

  • Support setting primitive type in constructor argument $myMessage = new MyMessage(['double_value' => 1.1]);
  • Add both setXXXValue and getXXXValue and separate their usages from setXXX and getXXX

I see your concerns about using null with setXXXValue now. IMHO, it would be better not to support using null with setXXXValue at all.
It'd be misleading if setXXX(null) is equal to setXXXValue(null).

As for supporting primitive value in setXXX(), I'm quite neutral to this with a bit inclination to not support it.
The reason is that it'd be clearer to separate the use of setXXX and setXXXValue.
(Needing to type one more word "Value" after the original method names doesn't seem bad to me)

If we sometimes allow some kinds of arguments in both methods, but in other times, disallow other kinds of arguments in one methods, it'd be pretty confusing.

@michaelbausor

This comment has been minimized.

Show comment
Hide comment
@michaelbausor

michaelbausor Sep 27, 2018

Contributor

I have pulled in the changes and made fixes, PTAL.

Contributor

michaelbausor commented Sep 27, 2018

I have pulled in the changes and made fixes, PTAL.

@michaelbausor

This comment has been minimized.

Show comment
Hide comment
@michaelbausor

michaelbausor Sep 27, 2018

Contributor

(Fixes in my code based on PR comments, not fixes in the C extension changes)

Contributor

michaelbausor commented Sep 27, 2018

(Fixes in my code based on PR comments, not fixes in the C extension changes)

@michaelbausor

This comment has been minimized.

Show comment
Hide comment
@michaelbausor

michaelbausor Oct 2, 2018

Contributor

Synced, PTAL

Contributor

michaelbausor commented Oct 2, 2018

Synced, PTAL

@TeBoring TeBoring added the kokoro:run label Oct 3, 2018

@kokoro-team kokoro-team removed the kokoro:run label Oct 3, 2018

@TeBoring

This comment has been minimized.

Show comment
Hide comment
@TeBoring

TeBoring Oct 3, 2018

Contributor

Sorry, I forgot to add include path in tests.sh. I have send the fix. Please sync again.

Contributor

TeBoring commented Oct 3, 2018

Sorry, I forgot to add include path in tests.sh. I have send the fix. Please sync again.

@michaelbausor

This comment has been minimized.

Show comment
Hide comment
@michaelbausor

michaelbausor Oct 3, 2018

Contributor

Synced, PTAL

Contributor

michaelbausor commented Oct 3, 2018

Synced, PTAL

@TeBoring TeBoring added the kokoro:run label Oct 4, 2018

@kokoro-team kokoro-team removed the kokoro:run label Oct 4, 2018

TeBoring added some commits Oct 4, 2018

@TeBoring

This comment has been minimized.

Show comment
Hide comment
@TeBoring

TeBoring Oct 4, 2018

Contributor

I am really sorry that there were other problems. This time I have tested in #5222. Could you please sync again?

Contributor

TeBoring commented Oct 4, 2018

I am really sorry that there were other problems. This time I have tested in #5222. Could you please sync again?

@michaelbausor

This comment has been minimized.

Show comment
Hide comment
@michaelbausor

michaelbausor Oct 4, 2018

Contributor

No worries! Synced, PTAL.

Contributor

michaelbausor commented Oct 4, 2018

No worries! Synced, PTAL.

@TeBoring TeBoring added the kokoro:run label Oct 5, 2018

@kokoro-team kokoro-team removed the kokoro:run label Oct 5, 2018

@TeBoring TeBoring added the kokoro:run label Oct 6, 2018

@TeBoring

This comment has been minimized.

Show comment
Hide comment
@TeBoring

TeBoring Oct 7, 2018

Contributor

The broken javascript test should not be related to this change.

Contributor

TeBoring commented Oct 7, 2018

The broken javascript test should not be related to this change.

@TeBoring TeBoring merged commit 6a51c03 into protocolbuffers:master Oct 7, 2018

26 of 28 checks passed

Linux JavaScript Kokoro build failed
Details
Linux Ruby Kokoro build failed
Details
Bazel Kokoro build successful
Details
Linux 32-bit Kokoro build successful
Details
Linux C# Kokoro build successful
Details
Linux C++ Distcheck Kokoro build successful
Details
Linux Golang Kokoro build successful
Details
Linux Java Compatibility Kokoro build successful
Details
Linux Java JDK 7 Kokoro build successful
Details
Linux Java Oracle 7 Kokoro build successful
Details
Linux PHP Kokoro build successful
Details
Linux Python Kokoro build successful
Details
Linux Python C++ Kokoro build successful
Details
Linux Python Compatibility Kokoro build successful
Details
MacOS C++ Kokoro build successful
Details
MacOS C++ Distcheck Kokoro build successful
Details
MacOS JavaScript Kokoro build successful
Details
MacOS Obj-C CocoaPods Integration Kokoro build successful
Details
MacOS Obj-C OS X Kokoro build successful
Details
MacOS Obj-C iOS Debug (Allowed Failure) Kokoro build successful
Details
MacOS Obj-C iOS Release (Allowed Failure) Kokoro build successful
Details
MacOS PHP5.6 Kokoro build successful
Details
MacOS PHP7.0 Kokoro build successful
Details
MacOS Python Kokoro build successful
Details
MacOS Python C++ (Allowed Failure) Kokoro build successful
Details
MacOS Ruby 2.1 Kokoro build successful
Details
MacOS Ruby 2.2 (Allowed Failure) Kokoro build successful
Details
cla/google All necessary CLAs are signed
@TeBoring

This comment has been minimized.

Show comment
Hide comment
@TeBoring

TeBoring Oct 7, 2018

Contributor

@BSBandme could you help take a look of the broken javascript test?

Contributor

TeBoring commented Oct 7, 2018

@BSBandme could you help take a look of the broken javascript test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment