-
Notifications
You must be signed in to change notification settings - Fork 15.6k
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 Enum methods for converting to/from strings #5342
PHP: Add Enum methods for converting to/from strings #5342
Conversation
It seems some tests are broken. Could you help take a look? |
@TeBoring updated to not use EnumTrait, PTAL |
It seems the test is still broken. Basically, GPBUtil should also be avoided in generated code. |
Doesn't simply expand the GPBUtil method in the generated code work? |
Yes, it does. I thought that using GPBUtil would work, but it seems not (I'll look into why my local testing caught the earlier error with EnumTrait, but not this one GPBUtil). |
Updated, PTAL |
Still some broken tests. Please take a look. |
php/tests/well_known_test.php
Outdated
return [ | ||
['\Google\Protobuf\Field\Cardinality'], | ||
['\Google\Protobuf\Field\Kind'], | ||
['\Google\Protobuf\Internal\FieldDescriptorProto\Label'], |
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.
These messages under Google\Protobuf\Internal are internal only and don't have implementation in c extension.
We don't need to test them.
@@ -4,6 +4,8 @@ | |||
|
|||
namespace Google\Protobuf; | |||
|
|||
use UnexpectedValueException; |
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.
I got warning: The use statement with non-compound name 'UnexpectedValueException' has no effect
Remove that?
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.
I found that I got this warning only for the no-namespace enum. Is that what you found? I have added a commit to remove the use statement from the no-namespace case.
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.
yes. For NoNamespace enum
Implemented c extension in the last commit. |
@michaelbausor Do you want to take another look? |
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.
LGTM
Hooray! nice work guys! |
@michaelbausor et al – any update on when this will make it into the PHP protobuf library? Tried using this today and there is no method |
3.7.0rc2 should already have this
…On Sat, Feb 16, 2019 at 14:39 Rebecca Taylor ***@***.***> wrote:
@michaelbausor <https://github.com/michaelbausor> et al – any update on
when this will make it into the PHP protobuf library?
Tried using this today and there is no method ::name on my enum class,
presuming I'm calling it correctly.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#5342 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AE9H5Tf7R-P_FRyN0U78gBlaXKBLp9Jdks5vOIivgaJpZM4Yam3j>
.
|
This PR migrates the changes in #4534 and rebases them onto master branch. cc @TeBoring @fiboknacky