-
Notifications
You must be signed in to change notification settings - Fork 264
PHPLIB-1419 Encode Agg builder objects in Collection methods #1383
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f8213b2
to
da71fcc
Compare
da71fcc
to
0ba9938
Compare
0ba9938
to
5949533
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests failing only in evergreen "test-mongodb-4.0-replicaset-noauth-nossl"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We missed a BC break while planning this, so we'll have to think about how we can accomplish this.
The changes made to aggregate
and watch
will also have to be applied to the Client::watch
, Database::watch
, and Database::aggregate
methods.
src/Collection.php
Outdated
@@ -221,8 +222,9 @@ public function __toString() | |||
* @throws InvalidArgumentException for parameter/option parsing errors | |||
* @throws DriverRuntimeException for other driver errors (e.g. connection errors) | |||
*/ | |||
public function aggregate(array $pipeline, array $options = []) | |||
public function aggregate(array|Pipeline $pipeline, array $options = []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how I missed this while we were planning things, but this is a BC break, as any class extending Collection
and overriding aggregate
will be incompatible due to a narrower parameter type. I don't know if there's a way to solve this in 1.x, as I'd hate to defer this feature to 2.x.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compromise we can make for the moment is to accept an array of stages.
$pipeline = new Pipeline(...);
$results = $collection->aggregate(iterator_to_array($pipeline));
Ignore static analysis issues PHPLIB-1419 Fix BC break
acc21d4
to
7b4886b
Compare
7b4886b
to
f3b4961
Compare
7f8b5c4
to
f9539b6
Compare
|
||
if ($this->isShardedCluster()) { | ||
$this->markTestSkipped('Test does not apply on sharded clusters: need more than a single getMore call on the change stream.'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this was copied from testChangeStreamExample_1_4
in DocumentationExamplesTest. I think this skip was only done for documentation examples to avoid complicating the code snippets.
32bade4 also introduced advanceCursorUntilValid()
to fix change stream iteration for sharded clusters in our other tests.
I won't push for changing this, but wanted to provide some context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the context. In fact, I've simplified the documentation example you pointed. Given what's being tested, I don't think it's essential for the test to run on all server typologies.
* v1.x: (87 commits) Performance: Keep collections and indexes between GridFS tests (#1421) Add final annotations to non-internal Operation classes (#1410) Fix types accepted by $round (#1401) Replace arrayHasKey with assertArrayHasKey in tests (#1403) PHPLIB-1514 Make data providers static (#1404) PHPLIB-1515 Replace assertObjectHasAttribute with assertObjectHasProperty (#1405) Restore Prose22_RangeExplicitEncryptionTest (#1400) Remove Prose22_RangeExplicitEncryptionTest that requires ext-mongodb < 1.20 (#1394) Use `match` instead of `switch` when a simple value is returned (#1393) Remove PHPUnit functions polyfill (#1395) Update branch names for GHA workflows (#1390) PHPLIB-1419 Encode Agg builder objects in Collection methods (#1383) PHPLIB-1420 Integrate query builder for non-aggregation APIs (#1385) Fix optional parameter declared before required parameter (#1384) PHPLIB-1505 Add driver option "builderEncoder" (#1382) Exclude rector.php from the artifact Check generated files are up-to-date Replace composer package mongodb/builder Skip Pedentry method sort for generated files Remove composer constraints already imposed by the main package ...
* v1.x: (90 commits) Merge v1.20 into v1.x (#1447) PHPLIB-1525 Removes dependency to Symfony PHPUnit bridge (#1413) Change deprecated assertObjectHasAttribute to assertObjectHasProperty (#1432) Performance: Keep collections and indexes between GridFS tests (#1421) Add final annotations to non-internal Operation classes (#1410) Fix types accepted by $round (#1401) Replace arrayHasKey with assertArrayHasKey in tests (#1403) PHPLIB-1514 Make data providers static (#1404) PHPLIB-1515 Replace assertObjectHasAttribute with assertObjectHasProperty (#1405) Restore Prose22_RangeExplicitEncryptionTest (#1400) Remove Prose22_RangeExplicitEncryptionTest that requires ext-mongodb < 1.20 (#1394) Use `match` instead of `switch` when a simple value is returned (#1393) Remove PHPUnit functions polyfill (#1395) Update branch names for GHA workflows (#1390) PHPLIB-1419 Encode Agg builder objects in Collection methods (#1383) PHPLIB-1420 Integrate query builder for non-aggregation APIs (#1385) Fix optional parameter declared before required parameter (#1384) PHPLIB-1505 Add driver option "builderEncoder" (#1382) Exclude rector.php from the artifact Check generated files are up-to-date ...
* v1.x: (90 commits) Merge v1.20 into v1.x (#1447) PHPLIB-1525 Removes dependency to Symfony PHPUnit bridge (#1413) Change deprecated assertObjectHasAttribute to assertObjectHasProperty (#1432) Performance: Keep collections and indexes between GridFS tests (#1421) Add final annotations to non-internal Operation classes (#1410) Fix types accepted by $round (#1401) Replace arrayHasKey with assertArrayHasKey in tests (#1403) PHPLIB-1514 Make data providers static (#1404) PHPLIB-1515 Replace assertObjectHasAttribute with assertObjectHasProperty (#1405) Restore Prose22_RangeExplicitEncryptionTest (#1400) Remove Prose22_RangeExplicitEncryptionTest that requires ext-mongodb < 1.20 (#1394) Use `match` instead of `switch` when a simple value is returned (#1393) Remove PHPUnit functions polyfill (#1395) Update branch names for GHA workflows (#1390) PHPLIB-1419 Encode Agg builder objects in Collection methods (#1383) PHPLIB-1420 Integrate query builder for non-aggregation APIs (#1385) Fix optional parameter declared before required parameter (#1384) PHPLIB-1505 Add driver option "builderEncoder" (#1382) Exclude rector.php from the artifact Check generated files are up-to-date ...
* v1.x: (95 commits) PHPLIB-1369 Upgrade to PHPUnit 10 (#1412) Higher phpunit version required (#1463) Fix deprecations in tests (#1458) Deprecate functionality to be removed (#1441) Expect BulkWriteException (#1455) Merge v1.20 into v1.x (#1447) PHPLIB-1525 Removes dependency to Symfony PHPUnit bridge (#1413) Change deprecated assertObjectHasAttribute to assertObjectHasProperty (#1432) Performance: Keep collections and indexes between GridFS tests (#1421) Add final annotations to non-internal Operation classes (#1410) Fix types accepted by $round (#1401) Replace arrayHasKey with assertArrayHasKey in tests (#1403) PHPLIB-1514 Make data providers static (#1404) PHPLIB-1515 Replace assertObjectHasAttribute with assertObjectHasProperty (#1405) Restore Prose22_RangeExplicitEncryptionTest (#1400) Remove Prose22_RangeExplicitEncryptionTest that requires ext-mongodb < 1.20 (#1394) Use `match` instead of `switch` when a simple value is returned (#1393) Remove PHPUnit functions polyfill (#1395) Update branch names for GHA workflows (#1390) PHPLIB-1419 Encode Agg builder objects in Collection methods (#1383) ...
Fix PHPLIB-1419
Requires #1382The encoding is done in the
Collection
methods, not in the variousOperation
classes:Only the
BulkWrite
operation has the option, because the processing of each operation is already organised in this operation class.The Collection methods does not accept the
builderEncoder
option for now. If a specific encoder is necessary for an operation, if can be called just before the operation: