Skip to content
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

PHPLIB-1617 Accept a Pipeline instance in aggregate and watch methods #1580

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Feb 7, 2025

Fix PHPLIB-1617

In v1.x, we could not change the signature of some methods to accept a Pipeline instance, so in #1383 I added a workaround.

But for v2.0.0, we allow ourselves this breaking change to simplify the API.

@GromNaN GromNaN requested a review from a team as a code owner February 7, 2025 09:33
@GromNaN GromNaN requested a review from jmikola February 7, 2025 09:33
@GromNaN GromNaN enabled auto-merge (squash) February 10, 2025 15:13
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

I'll defer to you about using a list of stages as the default code path for tests and then only conditionally converting that into a Pipeline object (presumably with the ... operator, just as is done in the helper methods).


if ($pipelineAsArray) {
$pipeline = iterator_to_array($pipeline);
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't disagree that this tests the underlying behavior, but it seems a bit backwards that this operates by converting the Pipeline back into an array, instead of creating a list of stages and then conditionally converting that to a pipeline.

The latter approach seems more straightforward.

Copy link
Member Author

@GromNaN GromNaN Feb 10, 2025

Choose a reason for hiding this comment

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

You're right, I hadn't thought of it like that. #1596

@GromNaN GromNaN merged commit 0bdbf47 into mongodb:v2.x Feb 10, 2025
30 checks passed
alcaeus added a commit that referenced this pull request Feb 28, 2025
* v2.x:
  Add return type hint for Encoder::encode() implementation
  Reverse pipeline init from an array (#1596)
  PHPLIB-1617 Accept a Pipeline instance in aggregate and watch methods (#1580)
  Fix CS
  Require latest python version (#1564) (#1565)
  Update src/Operation/Find.php
  Ignore `disableMD5` option as `md5` field is removed from the spec (#1502)
  Remove obsolete baseline entries
  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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants