Visitar URL original
add missing hasOneof method to check presence of oneof fields by anight · Pull Request #8003 · protocolbuffers/protobuf · GitHub
Skip to content

add missing hasOneof method to check presence of oneof fields - #8003

Merged
haberman merged 2 commits into
protocolbuffers:masterfrom
anight:php_extension_add_has_oneof_field_method
Nov 4, 2020
Merged

add missing hasOneof method to check presence of oneof fields#8003
haberman merged 2 commits into
protocolbuffers:masterfrom
anight:php_extension_add_has_oneof_field_method

Conversation

@anight

@anight anight commented Nov 3, 2020

Copy link
Copy Markdown
Contributor

Possible solution for #8001 and #7961

test.proto:

syntax = "proto3";
package Test;
message Test {
        oneof O {
                int32 A = 1;
                int32 B = 2;
        }
}

test.php:

<?php
        dl('protobuf.so');
        require_once './vendor/autoload.php';
        $t = new Test\Test();
        print_r($t->hasB() === false);
?>

Output:

$ php ./test.php
Fatal error: Uncaught Error: Call to undefined method Test\Test::hasOneof() ...

Expected output:

$ php ./test.php
1

@google-cla google-cla Bot added the cla: yes label Nov 3, 2020
@anight anight changed the title add missing has<Field>() method for oneof fields add missing hasOneof method to check presence of oneof fields Nov 3, 2020
haberman
haberman reviewed Nov 3, 2020
View reviewed changes

@haberman haberman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks! Just a few small comments.

Comment thread php/ext/google/protobuf/message.c Resolved
Comment thread php/ext/google/protobuf/message.c
*
* @return boolean
*/
PHP_METHOD(Message, hasOneof) {

@haberman haberman Nov 3, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test for this?

@haberman haberman Nov 3, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test looks great, but it might be better to move it into one of the existing test files (I think GeneratedClassTest.php would be a good option). Adding new files is a bit more involved because then the new file has to get added to the Makefile:

https://github.com/protocolbuffers/protobuf/blob/master/Makefile.am#L767-L961

@haberman

haberman commented Nov 3, 2020

Copy link
Copy Markdown
Member

I kicked off the tests. If you move your new test into an existing file, it should fix the "distcheck" test failure.

@haberman

haberman commented Nov 4, 2020

Copy link
Copy Markdown
Member

We'd like to cut a branch for our upcoming release today and this fix should definitely go in. Are you planning to push again in the next few hours? If not I may need to adopt this change in a different PR to get it merged.

@haberman

haberman commented Nov 4, 2020

Copy link
Copy Markdown
Member

Actually let me merge this now and the send a follow-up PR to fix the test failures.

@haberman
haberman merged commit 957a0bf into protocolbuffers:master Nov 4, 2020
@haberman haberman mentioned this pull request Nov 4, 2020
Merged
@haberman haberman mentioned this pull request Jan 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haberman haberman haberman left review comments

Assignees

No one assigned

Labels

cla: yes php

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@anight @haberman @protobuf-kokoro