Skip to content

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Aug 8, 2025

@jmikola jmikola requested a review from a team as a code owner August 8, 2025 18:04
@jmikola jmikola requested review from GromNaN and alcaeus August 8, 2025 18:04
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

The fix looks good, I have some comments on the tests.

$client = static::createTestClient();

// Ensure the key vault collection is dropped before each test
$collection = $client->selectCollection('keyvault', 'datakeys', ['writeConcern' => new WriteConcern(WriteConcern::MAJORITY)]);
Copy link
Member

Choose a reason for hiding this comment

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

You'll get used to the new name of this method.

Suggested change
$collection = $client->selectCollection('keyvault', 'datakeys', ['writeConcern' => new WriteConcern(WriteConcern::MAJORITY)]);
$collection = $client->getCollection('keyvault', 'datakeys', ['writeConcern' => new WriteConcern(WriteConcern::MAJORITY)]);

Why do you need to set the writeConcern, as that doesn't guarantee that all replica are up to date.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm borrowing that from the CSFLE spec tests, which use w:majority for all ops targeting the key vault collection. I reckon it makes little difference for tests, but in a production environment this would ensure that any modifications will persist through a replica set election.

{
protected ClientEncryption $clientEncryption;

public function setUp(): void
Copy link
Member

Choose a reason for hiding this comment

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

Both test classes has the same setUp method. They should probably be a single test class with 2 test methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Converted this to a single test file in a new PR.

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

Changes LGTM, but should this change target v1.21 for a patch release?

@jmikola
Copy link
Member Author

jmikola commented Aug 12, 2025

Superseded by #1745

@jmikola jmikola closed this Aug 12, 2025
@jmikola jmikola deleted the phplib-1702 branch August 12, 2025 18:33
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