PHPLIB-1227 Use void return types for operations without meaningful result document #1468

GromNaN · 2024-09-26T13:36:22Z

This introduces major breaking changes that we did not deprecate.

GromNaN · 2024-09-26T13:37:30Z

src/Database.php

@@ -314,17 +310,13 @@ public function createCollection(string $collectionName, array $options = []): a
     * getPrevious() and getEncryptedFields() methods, respectively.
     *
     * @see CreateCollection::__construct() for supported options
-     * @return array A tuple containing the command result document from creating the collection and the modified "encryptedFields" option
+     * @return array The modified "encryptedFields" option


Instead of a tuple, we return the 2nd element (the encryptedFields), but the return type is not modified.

GromNaN · 2024-09-26T13:39:07Z

tests/FunctionalTestCase.php

@@ -189,6 +196,45 @@ protected function assertCollectionExists(string $collectionName, ?string $datab
        }
    }

+    /** @param callable():void $closure */
+    protected static function commandResult(callable $closure): object


Read command result from the events. That allows to keep the assertCommandSucceeded. But it might not be necessary if the operations throw an exception when the command fails.

I concur. There should be no reason to maintain assertCommandSucceeded(), although removing it entirely might leave some tests without assertions (easy fix if so).

I removed all that assertions. assertCommandSucceeded is still used when running raw commands.

GromNaN · 2024-09-26T13:40:05Z

src/Operation/CreateCollection.php

-     *  * typeMap (array): Type map for BSON deserialization. This will only be
-     *    used for the returned command result document.
-     *


The typeMap option can be removed since we don't return the command results.

Thinking of your comment below on createDataKeys(), I noted you didn't suggest deprecating the typeMap option in 1.x. I also don't think that's necessary and feel we can just rely on upgrade notes for 2.0.

I agree with not needing a deprecation notice for removing typeMap, as we won't throw an error when the option is present, it just won't do anything.

I would point out however that a typeMap option here is a pretty good indication that the caller intends to use the result, so it could be a way to notify users of the changing return type. Since it doesn't catch all cases, I'm not sure it's warranted though.

Rebased on #1473

GromNaN · 2024-09-26T13:55:00Z

src/Operation/CreateEncryptedCollection.php

     * @throws DriverRuntimeException for errors creating a data key
     */
-    public function createDataKeys(ClientEncryption $clientEncryption, string $kmsProvider, ?array $masterKey, ?array &$encryptedFields = null): void
+    public function createDataKeys(ClientEncryption $clientEncryption, string $kmsProvider, ?array $masterKey): array


The function return null, we don't need to have a by-reference parameter to output the data keys. Using the returned value is more common.
This change can be backported to 1.x, with a deprecation message if $encryptedFields parameter is passed.

This change can be backported to 1.x

FWIW, I'm OK skipping that and just communicating this in the upgrade notes. I think it's very unlikely that anyone was actually using this API.

Defer to you, though.

If we consider the operation (and thus the API) internal in 1.x, I agree with not adding a deprecation layer in 1.x.

jmikola · 2024-09-27T00:39:07Z

src/Operation/CreateEncryptedCollection.php

     * @throws DriverRuntimeException for errors creating a data key
     */
-    public function createDataKeys(ClientEncryption $clientEncryption, string $kmsProvider, ?array $masterKey, ?array &$encryptedFields = null): void
+    public function createDataKeys(ClientEncryption $clientEncryption, string $kmsProvider, ?array $masterKey): array


This change can be backported to 1.x

FWIW, I'm OK skipping that and just communicating this in the upgrade notes. I think it's very unlikely that anyone was actually using this API.

Defer to you, though.

src/Operation/DropCollection.php

jmikola · 2024-09-27T00:41:52Z

src/Operation/CreateCollection.php

-     *  * typeMap (array): Type map for BSON deserialization. This will only be
-     *    used for the returned command result document.
-     *


Thinking of your comment below on createDataKeys(), I noted you didn't suggest deprecating the typeMap option in 1.x. I also don't think that's necessary and feel we can just rely on upgrade notes for 2.0.

jmikola · 2024-09-27T00:43:55Z

tests/FunctionalTestCase.php

@@ -189,6 +196,45 @@ protected function assertCollectionExists(string $collectionName, ?string $datab
        }
    }

+    /** @param callable():void $closure */
+    protected static function commandResult(callable $closure): object


I concur. There should be no reason to maintain assertCommandSucceeded(), although removing it entirely might leave some tests without assertions (easy fix if so).

alcaeus

Left a few comments, and I agree with the list of operations that no longer return a result. I'll defer to @jmikola for the detailed review.

UPGRADE-2.0.md

src/Client.php

alcaeus · 2024-09-30T07:36:56Z

src/Operation/CreateCollection.php

-     *  * typeMap (array): Type map for BSON deserialization. This will only be
-     *    used for the returned command result document.
-     *


I agree with not needing a deprecation notice for removing typeMap, as we won't throw an error when the option is present, it just won't do anything.

I would point out however that a typeMap option here is a pretty good indication that the caller intends to use the result, so it could be a way to notify users of the changing return type. Since it doesn't catch all cases, I'm not sure it's warranted though.

alcaeus · 2024-09-30T07:40:14Z

src/Operation/CreateEncryptedCollection.php

     * @throws DriverRuntimeException for errors creating a data key
     */
-    public function createDataKeys(ClientEncryption $clientEncryption, string $kmsProvider, ?array $masterKey, ?array &$encryptedFields = null): void
+    public function createDataKeys(ClientEncryption $clientEncryption, string $kmsProvider, ?array $masterKey): array


If we consider the operation (and thus the API) internal in 1.x, I agree with not adding a deprecation layer in 1.x.

src/Operation/DropCollection.php

…sult document

GromNaN · 2024-09-30T14:08:31Z

Rebased. Ready for review @jmikola @alcaeus

jmikola

Two suggestions (your call). LGTM with whatever you decide.

UPGRADE-2.0.md

jmikola · 2024-09-30T14:11:43Z

tests/Operation/ModifyCollectionFunctionalTest.php

@@ -29,9 +33,23 @@ public function testCollMod(): void
            ['index' => ['keyPattern' => ['lastAccess' => 1], 'expireAfterSeconds' => 1000]],
            ['typeMap' => ['root' => 'array', 'document' => 'array']],
        );
-        $result = $modifyCollection->execute($this->getPrimaryServer());
+        $modifyCollection->execute($this->getPrimaryServer());


I'm not sure that the return value of collMod is entirely irrelevant, as the server docs include a note about being able to infer whether a value actually changed server-side by inspecting the command result:

If the operation is successful but the hidden value has not changed (specifically, hiding an already hidden index or unhiding an already unhidden index), the command omits the hidden_old and hidden_new fields from the output.

I didn't consider that until seeing this test change.

If you agree, I'd suggest just reverting the changes to ModifyCollection. Otherwise, feel free to leave as-is.

I reverted the change on this operation. This was missing from #1473, so nothing to revert on v1.x

* v2.x: PHPLIB-1546 and PHPLIB-1159: Remove CreateCollection flags and autoIndexId options (#1478) PHPLIB-1227 Use void return types for operations without meaningful result document (#1468) Remove deprecated functionality (#1439) PHPLIB-1518 `WriteResult::get*Count()` always return an int on acknowledged result (#1454) PHPLIB-954: Add return types to all methods (#1391) PHPLIB-797: Remove unused methods in UnsupportedException (#1436) Revert "Add final annotations to non-internal Operation classes (#1410)" PHPLIB-953 Make internal classes and Operation classes final (#1392) PHPLIB-1218 Remove deprecated fields from GridFS files (#1398)

* v2.x: Regenerate evergreen configuration (#1503) PHPLIB-1546 and PHPLIB-1159: Remove CreateCollection flags and autoIndexId options (#1478) PHPLIB-1227 Use void return types for operations without meaningful result document (#1468) Remove deprecated functionality (#1439) PHPLIB-1518 `WriteResult::get*Count()` always return an int on acknowledged result (#1454) PHPLIB-954: Add return types to all methods (#1391) PHPLIB-797: Remove unused methods in UnsupportedException (#1436) Revert "Add final annotations to non-internal Operation classes (#1410)" PHPLIB-953 Make internal classes and Operation classes final (#1392) PHPLIB-1218 Remove deprecated fields from GridFS files (#1398)

GromNaN commented Sep 26, 2024

View reviewed changes

GromNaN force-pushed the PHPLIB-1227 branch from 244030e to 6f2085c Compare September 26, 2024 13:46

GromNaN commented Sep 26, 2024

View reviewed changes

GromNaN force-pushed the PHPLIB-1227 branch from e15bc11 to c6772de Compare September 26, 2024 13:55

jmikola reviewed Sep 27, 2024

View reviewed changes

GromNaN force-pushed the PHPLIB-1227 branch 3 times, most recently from 96d6c8a to a901218 Compare September 27, 2024 13:28

GromNaN marked this pull request as ready for review September 27, 2024 13:57

GromNaN requested a review from a team as a code owner September 27, 2024 13:57

GromNaN requested review from alcaeus and jmikola September 27, 2024 13:57

alcaeus reviewed Sep 30, 2024

View reviewed changes

GromNaN commented Sep 30, 2024

View reviewed changes

src/Operation/DropCollection.php Outdated Show resolved Hide resolved

GromNaN mentioned this pull request Sep 30, 2024

PHPLIB-1538 Deprecate typeMap on operations without meaningful result #1473

Merged

GromNaN force-pushed the PHPLIB-1227 branch 2 times, most recently from 378073a to eea3fbe Compare September 30, 2024 12:24

GromNaN requested a review from alcaeus September 30, 2024 12:24

PHPLIB-1227 Use void return type for operations without meaningful re…

260c266

…sult document

GromNaN force-pushed the PHPLIB-1227 branch from eea3fbe to 260c266 Compare September 30, 2024 14:06

jmikola approved these changes Sep 30, 2024

View reviewed changes

alcaeus approved these changes Sep 30, 2024

View reviewed changes

GromNaN force-pushed the PHPLIB-1227 branch 2 times, most recently from 72bf1b8 to 9b89a1b Compare October 1, 2024 10:07

Revert change on modifyCollection

5ff7621

GromNaN force-pushed the PHPLIB-1227 branch from 9b89a1b to 5ff7621 Compare October 1, 2024 10:08

GromNaN enabled auto-merge (squash) October 1, 2024 10:11

GromNaN merged commit f4962c9 into mongodb:v2.x Oct 1, 2024
28 checks passed

GromNaN deleted the PHPLIB-1227 branch October 1, 2024 11:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPLIB-1227 Use void return types for operations without meaningful result document #1468

PHPLIB-1227 Use void return types for operations without meaningful result document #1468

GromNaN commented Sep 26, 2024

GromNaN Sep 26, 2024 •

edited

Loading

GromNaN Sep 26, 2024

jmikola Sep 27, 2024

GromNaN Sep 27, 2024 •

edited

Loading

GromNaN Sep 26, 2024

jmikola Sep 27, 2024

alcaeus Sep 30, 2024

GromNaN Sep 30, 2024 •

edited

Loading

GromNaN Sep 26, 2024

jmikola Sep 27, 2024

alcaeus Sep 30, 2024

jmikola Sep 27, 2024

jmikola Sep 27, 2024

jmikola Sep 27, 2024

alcaeus left a comment

alcaeus Sep 30, 2024

alcaeus Sep 30, 2024

GromNaN commented Sep 30, 2024

jmikola left a comment

jmikola Sep 30, 2024

GromNaN Oct 1, 2024 •

edited

Loading

PHPLIB-1227 Use void return types for operations without meaningful result document #1468

PHPLIB-1227 Use void return types for operations without meaningful result document #1468

Conversation

GromNaN commented Sep 26, 2024

GromNaN Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN commented Sep 30, 2024

jmikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

GromNaN Sep 26, 2024 •

edited

Loading

GromNaN Sep 27, 2024 •

edited

Loading

GromNaN Sep 30, 2024 •

edited

Loading

GromNaN Oct 1, 2024 •

edited

Loading