Skip to content
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

Update PHP descriptors #3391

Merged
merged 12 commits into from Aug 4, 2017
Merged

Conversation

@michaelbausor
Copy link
Contributor

@michaelbausor michaelbausor commented Jul 18, 2017

cc @TeBoring

@grpc-kokoro
Copy link

@grpc-kokoro grpc-kokoro commented Jul 18, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

1 similar comment
@grpc-kokoro
Copy link

@grpc-kokoro grpc-kokoro commented Jul 18, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@bazel-io
Copy link

@bazel-io bazel-io commented Jul 18, 2017

Can one of the admins verify this patch?

@michaelbausor
Copy link
Contributor Author

@michaelbausor michaelbausor commented Jul 24, 2017

@TeBoring Added PHP implementation of descriptors in Google\Protobuf namespace, PTAL

@TeBoring
Copy link
Contributor

@TeBoring TeBoring commented Jul 25, 2017

ok to test

require_once('test_util.php');

use Google\Protobuf\DescriptorPool;
use Google\Protobuf\Internal\GPBLabel;
Copy link
Contributor

@TeBoring TeBoring Jul 27, 2017

Choose a reason for hiding this comment

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

No GPBLabel in c extension. Can we use integer instead?

Copy link
Contributor Author

@michaelbausor michaelbausor Jul 27, 2017

Choose a reason for hiding this comment

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

Yes, updated.

$this->assertInstanceOf('\Google\Protobuf\FieldDescriptor', $desc->getField(0));
$this->assertSame(7, $desc->getFieldCount());

$this->assertInstanceOf('\Google\Protobuf\Descriptor', $desc->getNestedType(0));
Copy link
Contributor

@TeBoring TeBoring Jul 27, 2017

Choose a reason for hiding this comment

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

c extension cannot provide info about nested type.

Copy link
Contributor Author

@michaelbausor michaelbausor Jul 27, 2017

Choose a reason for hiding this comment

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

Sorry, I forgot about this one - removed now.

$this->assertSame('optional_int32', $fieldDesc->getName());
$this->assertSame(1, $fieldDesc->getNumber());
$this->assertSame(self::GPBLABEL_OPTIONAL, $fieldDesc->getLabel());
$this->assertSame(GPBType::INT32, $fieldDesc->getType());
Copy link
Contributor

@TeBoring TeBoring Jul 28, 2017

Choose a reason for hiding this comment

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

Please also avoid using GPBType since it's internal. Otherwise, the compatibility test may be broken if we remove GPBType in future.

Copy link
Contributor Author

@michaelbausor michaelbausor Jul 31, 2017

Choose a reason for hiding this comment

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

Fixed

$this->assertInstanceOf('\Google\Protobuf\FieldDescriptor', $desc->getField(0));
$this->assertSame(7, $desc->getFieldCount());

$this->assertInstanceOf('\Google\Protobuf\EnumDescriptor', $desc->getEnumType(0));
Copy link
Contributor

@TeBoring TeBoring Jul 28, 2017

Choose a reason for hiding this comment

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

No information about nested enum in c extension.

Copy link
Contributor Author

@michaelbausor michaelbausor Jul 31, 2017

Choose a reason for hiding this comment

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

Fixed

@TeBoring
Copy link
Contributor

@TeBoring TeBoring commented Aug 2, 2017

#3422 has been merged.

@TeBoring
Copy link
Contributor

@TeBoring TeBoring commented Aug 3, 2017

It seems you need to add new file into Makefile.am

@TeBoring
Copy link
Contributor

@TeBoring TeBoring commented Aug 4, 2017

The error for jenkins is because the composer dependency used by jenkins is different after this change. Need to submit this change first and update the commit id in Dockerfile.

@michaelbausor michaelbausor changed the title WIP: Update PHP descriptors Update PHP descriptors Aug 4, 2017
@TeBoring TeBoring merged commit 21b0e55 into protocolbuffers:master Aug 4, 2017
3 of 5 checks passed
TeBoring added a commit to TeBoring/protobuf that referenced this issue Aug 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants