feat!: [Datastore] Upgrade to V2#7179
Conversation
1f14746 to
237531f
Compare
2eab49d to
cee1539
Compare
chore(Datastore): Enhance readability of RunQuery and RunAggregationQuery APIs
|
I'll give this a more thorough review later today, but don't forget we need to remove all V1 GAPIC client classes. For Datastore V1, that would be Also the PR title should be renamed to better describe what the PR is doing. For PubSub, we said "remove connection classes". I don't mind using V2 in the title (since we are bumping to a V2 version), but we do not need to include PHP in the title. |
bshaffer
left a comment
There was a problem hiding this comment.
Looks good! A few suggestions with testing and some other minor things. The bigger question I have is with the Operations class - it looks like we are doing a LOT of converting of parameters. Why is so much of this necessary? Since we are doing a major version break, we have an opportunity to clean this up. Parameters which need to be changed should fall in two categories:
- We think the current (non-standard) interface is better. This would include things like accepting the string values of constants instead of their int versions. And since we know which parameters are enums, we could even automate this for all parameters (something to consider, but maybe not a good idea)
- We think the GAPIC interface is better. These should then be deprecated and either removed entirely (if they are not heavily used parameters) or renamed/converted to their new parameter/type.
| } | ||
|
|
||
| return $filter; | ||
| } |
There was a problem hiding this comment.
Shouldn't this logic be in a Serializer class? This makes me a bit nervous, because changes to these parameters will break the API...
Is there a generic way we can do this logic? For instance, Message::serializeToJsonString will handle string enums (e.g. "AND" and "OR")
feat(Datastore): Datastore V2 PR Iteration
| } | ||
| ]); | ||
|
|
||
| $this->requestHandler = new RequestHandler( |
There was a problem hiding this comment.
Because there is only one GAPIC client in this library, there's no reason to use a RequestHandler like we have here. Just create the datastoreClient directly and use that.
| * @return mixed | ||
| */ | ||
| public function refreshOperation($stub, ConnectionInterface $connection, array $options = []) | ||
| public function refreshOperation($stub, RequestHandler $requestHandler, array $options = []) |
There was a problem hiding this comment.
Change this to just use DatastoreClient directly, instead of the RequestHandler
|
superceded by #8609 |
Main PR for all the Datastore PHP v2 upgrade changes as mentioned below:
Google\Cloud\Datastore\DatastoreClient7215b2e$requestHandlerprophecy andserializerbe2533c^1.55bf2c50bOperationclass as internal d6e2823BREAKING_CHANGE_REASON=RC-1 for the version 2 of the Datastore library.