Skip to content

Conversation

@Hectorhammett
Copy link
Collaborator

@Hectorhammett Hectorhammett commented Sep 16, 2025

This is an effort to update the Datastore library to start using a standardized way. This consists in removing the ConnectionInterface and replacing it for our generated client.

b/338660654

BREAKING_CHANGE_REASON=removing connection classes in favor of generated client, which will be breaking. We are incrementing a new major version

Copy link
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.

Second pass (all minor stuff)

Copy link
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 great! Just a few suggestions for options validation (not required), and also please update the PHPDoc for $options in Operation::allocateIds to list the supported options

@Hectorhammett Hectorhammett force-pushed the datastore-modernization branch from c7618b4 to 9ebfdc5 Compare September 29, 2025 21:30
@Hectorhammett Hectorhammett force-pushed the datastore-modernization branch from 1af2b1d to 9b1f5a2 Compare September 30, 2025 22:37
@Hectorhammett Hectorhammett force-pushed the datastore-modernization branch 2 times, most recently from e4518f0 to 57040d2 Compare October 2, 2025 02:06
@Hectorhammett Hectorhammett marked this pull request as ready for review October 2, 2025 17:49
@Hectorhammett Hectorhammett requested review from a team as code owners October 2, 2025 17:49
@Hectorhammett Hectorhammett requested review from a team as code owners October 8, 2025 20:10
Copy link
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.

I think we should go through each handwritten class file in src and promote the properties to the __construct constructor and give them types.

Comment on lines +449 to +457
$readOptions = $this->createReadOptions($this->pluckArray(
[
'readConsistency',
'transaction',
'readTime'
],
$options,
false
));
Copy link
Contributor

Choose a reason for hiding this comment

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

the third option here doesn't exist! That's only required for pluck, not pluckArray.
also, we can format this to be a bit less verbose (IMHO)

Suggested change
$readOptions = $this->createReadOptions($this->pluckArray(
[
'readConsistency',
'transaction',
'readTime'
],
$options,
false
));
$readOptions = $this->createReadOptions($this->pluckArray(
['readConsistency', 'transaction', 'readTime'],
$options,
));

$encode,
$returnInt64AsObject,
$connectionType = 'grpc'
$returnInt64AsObject
Copy link
Contributor

Choose a reason for hiding this comment

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

For these we could use inline constructor properties:

    public function __construct(
        private string $projectId,
        private bool $encode,
        private bool $returnInt64AsObject,
    ) {

$projectId,
$namespaceId,
EntityMapper $entityMapper,
$databaseId = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

here as well - all of these arguments can be defined as properties in the constructor:

    public function __construct(
        private DatastoreClient $gapicClient,
        private string $projectId,
        private string $namespaceId,
        private EntityMapper $entityMapper,
        private string $databaseId = ''
    ) {

Copy link
Contributor

Choose a reason for hiding this comment

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

both $entityMapper and $query can be given types and defined in the constructor

Copy link
Contributor

Choose a reason for hiding this comment

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

properties $operation, $transactionId, and $projectId should be typed and defined in the constructor

@Hectorhammett Hectorhammett force-pushed the datastore-modernization branch from 688a592 to 103ba8d Compare October 8, 2025 23:14
@bshaffer bshaffer merged commit 95b34bb into main Oct 9, 2025
59 of 62 checks passed
@bshaffer bshaffer deleted the datastore-modernization branch October 9, 2025 17:22
@bshaffer bshaffer mentioned this pull request Oct 9, 2025
13 tasks
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.

2 participants