Skip to content

feat!: [Datastore] Upgrade to V2#7179

Closed
yash30201 wants to merge 55 commits intogoogleapis:mainfrom
yash30201:datastore-php-v2
Closed

feat!: [Datastore] Upgrade to V2#7179
yash30201 wants to merge 55 commits intogoogleapis:mainfrom
yash30201:datastore-php-v2

Conversation

@yash30201
Copy link
Copy Markdown
Contributor

@yash30201 yash30201 commented Mar 26, 2024

Main PR for all the Datastore PHP v2 upgrade changes as mentioned below:

  • Enable direct Gapic Client instantiation in Google\Cloud\Datastore\DatastoreClient 7215b2e
  • Add credentials wrapper support in DatastoreClient 237531f
  • Instantiate serializer and Request Handler in DatastoreClient. cac76f1
  • Update Resource classes to add serializer and requesthandler in their constructor. f39189f
  • Update all the Snippet and Units tests to add $requestHandler prophecy and serializer be2533c
  • Update Client + ResourceClass methods to use request handler for everything, correspondingly, update their unit tests and snippet tests.
  • Remove all references of connection classes from everywhere 4c32fc9
  • Update System tests [Not needed as none of the tests broke🎉]
  • Add MIGRATING.md file e098d66
  • Update DatastoreClient's documentation 2196aa2
  • Remove old GAPIC files (class aliases, etc) 0596f27
  • Increase minimum core to ^1.55 bf2c50b
  • Mark Operation class as internal d6e2823

BREAKING_CHANGE_REASON=RC-1 for the version 2 of the Datastore library.

Comment thread Datastore/src/DatastoreClient.php Outdated
Comment thread Datastore/tests/Unit/TransactionTest.php
chore(Datastore): Enhance readability of RunQuery and RunAggregationQuery APIs
@bshaffer
Copy link
Copy Markdown
Contributor

bshaffer commented May 7, 2024

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 Datastore/src/V1/DatastoreClient.php and Datastore/src/V1/DatastoreGrpcClient.php.

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.

Copy link
Copy Markdown
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

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:

  1. 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)
  2. 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.

Comment thread Core/src/Testing/DatastoreOperationRefreshTrait.php Outdated
Comment thread Core/src/Testing/DatastoreOperationRefreshTrait.php Outdated
Comment thread Datastore/tests/Snippet/EntityTest.php Outdated
Comment thread Datastore/tests/Snippet/ReadOnlyTransactionTest.php Outdated
Comment thread Datastore/tests/Snippet/ReadOnlyTransactionTest.php Outdated
Comment thread Datastore/src/Operation.php Outdated
Comment thread Datastore/src/Operation.php Outdated
}

return $filter;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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")

Comment thread Datastore/src/Operation.php
Comment thread Datastore/src/Operation.php
@yash30201 yash30201 changed the title feat!: [Datastore] Upgrade to PHP V2 feat!: [Datastore] Upgrade to V2 May 13, 2024
@bshaffer bshaffer self-assigned this Jun 12, 2024
}
]);

$this->requestHandler = new RequestHandler(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@Hectorhammett

* @return mixed
*/
public function refreshOperation($stub, ConnectionInterface $connection, array $options = [])
public function refreshOperation($stub, RequestHandler $requestHandler, array $options = [])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change this to just use DatastoreClient directly, instead of the RequestHandler

@bshaffer
Copy link
Copy Markdown
Contributor

bshaffer commented Oct 9, 2025

superceded by #8609

@bshaffer bshaffer closed this Oct 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants