Migrate to new protobuf implementation #512

michaelbausor · 2017-05-22T17:04:17Z

This PR still needs some cleaning up, but I wanted to start getting review sooner rather than later. Some points:

The most significant change is to logging, pubsub and spanner, because they rely on GAPICs. In particular, the PhpArray codec is replaced by a Serializer object.
The huge number of files is because this PR adds all of the generated protobuf types - these don't need review individually, but we do want to consider what will be the best repo structure for including these types (if indeed we decide to go ahead with that plan), considering our release process.

dwsupplee

This is looking great, just a few early notes!

src/Logging/V2/ConfigServiceV2Client.php

@@ -534,7 +544,7 @@ public function createSink($parent, $sink, $optionalArgs = [])
    /**
     * Updates a sink. If the named sink doesn't exist, then this method is
     * identical to
-     * [sinks.create](https://cloud.google.com/logging/docs/api/reference/rest/v2/projects.sinks/create).
+     * [sinks.create](/logging/docs/api/reference/rest/v2/projects.sinks/create).


src/proto/Google/Cloud/Language/V1beta2/AnalyzeEntitiesRequest.php

+     * Input document.
+     * </pre>
+     *
+     * <code>.google.cloud.language.v1beta2.Document document = 1;</code>


src/google-cloud-php.iml

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="UTF-8"?>


dwsupplee · 2017-05-22T18:37:21Z

There are some types that still live in proto-client-php, for example:

https://github.com/googleapis/proto-client-php/tree/proto3/src/Google/Logging/Type

Should these be a part of this PR as well?

src/Core/Serializer.php

+                }
+                $v = $arr;
+            }
+            elseif ($field->isRepeated()) {


dwsupplee · 2017-05-22T18:58:36Z

If we do keep the protos in this repository, what do you think of the following?

Instead of Google\Logging\V2\CreateSinkRequest - dir: src/proto/Logging/V2

We Use Google\Cloud\Logging\V2\CreateSinkRequest - dir: src/Logging/V2

A major downside here, is that the protos will live right next to the GAPIC clients - making it hard to distinguish which to use if looking directly at the source. An option then would be to use Google\Cloud\Logging\V2\Proto\CreateSinkRequest - dir: src/Logging/V2/Proto.

On the plus side, we stick to our core namespace of Google\Cloud and managing releases will be easier.

michaelbausor · 2017-05-22T19:03:44Z

@dwsupplee That seems possible, but one problem is that the proto types do not all use the Google\Cloud namespace (e.g. the logging types are under Google\Logging rather than Google\Cloud\Logging). This is something that we could perhaps push for, by adding support for an option to specify the namespace in the proto file similar to C#: https://github.com/googleapis/googleapis/blob/6ae2d424245deeb34cf73c4f7aba31f1079bcc40/google/logging/v2/logging.proto#L28

dwsupplee · 2017-05-22T19:17:42Z

That sounds like a great solution to me

michaelbausor · 2017-05-22T20:52:49Z

Given that, I think it might be best if we make the migration to the new protobuf implementation without including the generated types in google-cloud-php, and instead leaving them in proto-client-php. Then once we have that protobuf feature available, we can change the namespaces and move the generated classes. This has the downside of involving two disruptive changes, but the upside of not blocking on having that protobuf feature available. WDYT?

dwsupplee · 2017-05-22T21:02:50Z

Is it correct then that for packages we are currently GA on we would end up bumping them to 3.0 in a relatively short time frame? I have some definite reservations there - do you have an idea as to when it may be possible to get the update to change the namespaces?

michaelbausor · 2017-05-22T21:35:30Z

No, we should not be bumping them beyond 1.0. The affected GA API is Logging, but in that case we have gone GA on the manual layer but not on the GAPIC client or protobuf types, because we knew that these breaking changes would be required (and we documented that the GAPIC layer is still experimental).

michaelbausor · 2017-05-24T21:21:27Z

An update after some discussions with @bshaffer, @tmatsuo and @TeBoring:

We are now looking to implement the proto option to set the PHP namespace (issue filed here: protocolbuffers/protobuf#3122) before we make the migration over to the new protobuf implementation. This will allow us to move all of the generated protobuf types into the Google\Cloud namespace.

There is also a new issue in protobuf that I discovered today while running Spanner integration tests, which we will need to resolve before we can complete the migration: protocolbuffers/protobuf#3139

michaelbausor · 2017-06-02T17:46:16Z

I have updated the PR to include:

Protobuf types are now in src/Google/Cloud directories and have correct namespaces (generated from Add new file option php_namespace. protocolbuffers/protobuf#3162). I think that the clutter, while not ideal, it not too bad - README samples can direct users to the client class, and there is a single place to look if you want to find a class to use.
The protobuf metadata classes are in src/GPBMetadata. I think this is the best place for them - they all reside under the GPBMetadata\\ namespace, and should never be used directly, so having them in the src/Google/Cloud directories doesn't seem right.
Moved the Serializer class into GAX. I think that the functionality will be useful in autogen code as well as google-cloud-php. WDYT?

This PR is awful to review because of the proto files. Since it is not passing test yet anyway, WDYT if I remove the proto types for now?

michaelbausor · 2017-06-22T23:43:36Z

After much struggling with HHVM, I have come up against a tricky problem. The reason it is failing is this code in protobuf. This is problematic because (1) this code is a PARSE-time error in HHVM, not a runtime error, and (2) this code does NOT follow PSR-4, meaning it is not autoloaded, but always loaded. Therefore, just skipping the gRPC tests is not sufficient - it is necessary to conditionally remove the dependencies on google/protobuf, google/gax and google/proto-client-php for HHVM.

After doing that, we then hit the problem on the snippet parser, which also tries to load all the classes and errors when it can't.

I think this can be solved by adding an exclude list for the snippet parser, which checks for HHVM and conditionally does not check the Spanner files for snippets. This would also allow use to remove the stanley-cheung/protobuf-php dependency.

This reverts commit 398ad11.

This reverts commit 531ea97.

This reverts commit 707a910.

dwsupplee · 2017-06-23T00:42:02Z

That sounds like a solid solution for now, and hopefully we can get HHVM support sooner rather than later.

michaelbausor · 2017-06-23T17:23:07Z

HHVM tests are now passing. I did not need to use an exclusion list for grpc snippets in the end, but having implemented it, I have retained it to remove the DrSlump dev dependency.

I believe this is ready for final review, pending releases of GAX and proto-client-php, and updating their docs to link externally instead of to code.

dwsupplee

Very nice work. 👍

Just a few comments, overall LGTM!

composer.json

-        "google/gax": "^0.10",
+        "google/proto-client": "^0.20.0",
+        "google/gax": "^0.20.0",
+        "google/protobuf": "3.3.2",


composer.json

        "symfony/lock": "3.3.x-dev#1ba6ac9"
    },
    "suggest": {
        "google/gax": "Required to support gRPC",
-        "google/proto-client-php": "Required to support gRPC",


dev/src/Snippet/Coverage/Coverage.php

@@ -17,8 +17,14 @@

 namespace Google\Cloud\Dev\Snippet\Coverage;

+use Google\Cloud\Dev\CheckGrpcTrait;


docs/external-classes.json

 }, {
  "name": "Google\\GAX\\",
-  "uri": "https://github.com/googleapis/gax-php/tree/master/src/%s.php"
+  "uri": "https://googleapis.github.io/gax-php/master/Google/GAX/%s.html"


src/Spanner/composer.json

-        "google/cloud-core": "^1.4",
-        "google/gax": "^0.10",
-        "google/proto-client-php": "^0.13"
+        "google/cloud-core": "^1.6",


dwsupplee · 2017-06-26T11:22:36Z

Did we need to revert the snippet changes (9c4b04b) (a518c92)?

michaelbausor

Did we need to revert the snippet changes (9c4b04b) (a518c92)?

We didn't need to, but because we aren't including the protobuf classes, there wasn't a reason to change them, and I wanted to minimize the changes in this PR. If you think they are a worthwhile change in their own right (or just worth adding as preparation) and can add them back in.

composer.json

-        "google/gax": "^0.10",
+        "google/proto-client": "^0.20.0",
+        "google/gax": "^0.20.0",
+        "google/protobuf": "3.3.2",


composer.json

        "symfony/lock": "3.3.x-dev#1ba6ac9"
    },
    "suggest": {
        "google/gax": "Required to support gRPC",
-        "google/proto-client-php": "Required to support gRPC",


dev/src/Snippet/Coverage/Coverage.php

@@ -17,8 +17,14 @@

 namespace Google\Cloud\Dev\Snippet\Coverage;

+use Google\Cloud\Dev\CheckGrpcTrait;


docs/external-classes.json

 }, {
  "name": "Google\\GAX\\",
-  "uri": "https://github.com/googleapis/gax-php/tree/master/src/%s.php"
+  "uri": "https://googleapis.github.io/gax-php/master/Google/GAX/%s.html"


src/Spanner/composer.json

-        "google/cloud-core": "^1.4",
-        "google/gax": "^0.10",
-        "google/proto-client-php": "^0.13"
+        "google/cloud-core": "^1.6",


dwsupplee · 2017-06-26T16:28:33Z

We didn't need to, but because we aren't including the protobuf classes, there wasn't a reason to change them, and I wanted to minimize the changes in this PR. If you think they are a worthwhile change in their own right (or just worth adding as preparation) and can add them back in.

I wasn't sure if lines like this may cause issues, but after taking a quick look at the docs it appears we should be okay. Given that, I'm happy to keep the changes in this PR minimal, that works for me!

michaelbausor · 2017-06-28T17:49:43Z

Great! I believe I have addressed all changes, so this should be ready to merge - PTAL

michaelbausor added 2 commits May 19, 2017 16:38

Update for new proto3 types

6be2e07

Fixes for spanner

4dc99ee

michaelbausor added gapic do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels May 22, 2017

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 22, 2017

dwsupplee reviewed May 22, 2017

View reviewed changes

src/google-cloud-php.iml Outdated

@@ -0,0 +1,9 @@

<?xml version="1.0" encoding="UTF-8"?>

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

jdpedrie reviewed May 22, 2017

View reviewed changes

src/Core/Serializer.php Outdated

}

$v = $arr;

}

elseif ($field->isRepeated()) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Address PR comments

42b04d1

michaelbausor added 6 commits May 23, 2017 07:44

Update tests

d261bbb

Fix Mutation Write

7398560

Update spanner serialization

20efc87

Merge branch 'master' into proto3-pr

8f748c8

Support message type transformers

cf5d921

Update spanner unpacking

f0909c2

michaelbausor added 6 commits May 31, 2017 16:31

Merge branch 'master' into proto3-pr

1924e27

Update to match latest gax

6aa7ff1

Remove proto dir

ec2fa7c

Update GAPIC clients and tests

51b1724

Add protobuf types

1f12e7e

Update psr-4

fd386cf

michaelbausor added 2 commits June 22, 2017 15:26

Remove protobuf deps when running hhvm

f8d50cc

Skip Spanner tests that use gRPC

5921244

michaelbausor added 4 commits June 22, 2017 17:29

Remove test code

707a910

Revert "Update Core to ensure changes are not breaking"

df68e90

This reverts commit 398ad11.

Revert "Update composer, use GPBGrpc classes from Core"

82b7021

This reverts commit 531ea97.

Revert "Remove test code"

87532e9

This reverts commit 707a910.

michaelbausor added 5 commits June 22, 2017 17:48

Reintroduce namespace changes

8776d4c

Update snippet tests

e11d574

Check for external classes in Google\Cloud namespace

c271202

Remove debug and unused code

ab8fc4c

Rename SpannerClient imports

cfe71c2

michaelbausor added 3 commits June 23, 2017 16:49

Update gax and proto-client versions

cabb852

Update links to GAX and proto-clien docs

0517090

Update proto-client in travis

4f1a9ee

dwsupplee reviewed Jun 26, 2017

View reviewed changes

michaelbausor commented Jun 26, 2017

View reviewed changes

Address PR comments

c3368d2

dwsupplee approved these changes Jun 30, 2017

View reviewed changes

dwsupplee merged commit 100569e into googleapis:master Jun 30, 2017

michaelbausor mentioned this pull request Jun 30, 2017

Regenerate GAPICs with updated documentation #539

Closed

dwsupplee mentioned this pull request Jun 30, 2017

Define a better process for testing side by side installation of components #555

Closed

dwsupplee removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 30, 2017

jdpedrie mentioned this pull request Jun 30, 2017

Prepare v0.33.0 #557

Merged

michaelbausor deleted the proto3-pr branch August 21, 2017 15:56

		@@ -17,8 +17,14 @@

		namespace Google\Cloud\Dev\Snippet\Coverage;

		use Google\Cloud\Dev\CheckGrpcTrait;

Migrate to new protobuf implementation #512

Migrate to new protobuf implementation #512

Conversation

michaelbausor commented May 22, 2017

dwsupplee left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

dwsupplee commented May 22, 2017

This comment was marked as spam.

This comment was marked as spam.

dwsupplee commented May 22, 2017 • edited Loading

michaelbausor commented May 22, 2017

dwsupplee commented May 22, 2017

michaelbausor commented May 22, 2017

dwsupplee commented May 22, 2017

michaelbausor commented May 22, 2017

michaelbausor commented May 24, 2017

michaelbausor commented Jun 2, 2017

michaelbausor commented Jun 22, 2017

dwsupplee commented Jun 23, 2017

michaelbausor commented Jun 23, 2017

dwsupplee left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

dwsupplee commented Jun 26, 2017

michaelbausor left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

dwsupplee commented Jun 26, 2017

michaelbausor commented Jun 28, 2017

dwsupplee commented May 22, 2017 •

edited

Loading