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

Introduce aggregation API to replicate query API #2211

Merged
merged 4 commits into from
Jul 24, 2020

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Jul 22, 2020

Q A
Type feature
BC Break no
Fixed issues

Summary

This introduces a new Aggregation class, that can be created from an aggregation builder using the getAggregation method. The execute method on the aggregation builder has been deprecated in favour of the new API.

Rationale

The query builder returns a query instance that can be passed around without executing the query, but also disallowing changes to the query. The aggregation builder on the other hand only returns an iterator that may or may not have already executed something on the database. Passing the aggregation builder around opens it up to modification that may not be intended.

@alcaeus alcaeus added this to the 2.2.0 milestone Jul 22, 2020
@alcaeus alcaeus requested a review from malarzm July 22, 2020 11:12
@alcaeus alcaeus self-assigned this Jul 22, 2020
Copy link
Member

@malarzm malarzm left a comment

Choose a reason for hiding this comment

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

Please add UPGRADE-2.2.md already so we won't forget about it later :)

@alcaeus alcaeus force-pushed the aggregation-instance branch from fb5835e to 222334b Compare July 23, 2020 13:10
@alcaeus alcaeus requested a review from malarzm July 23, 2020 13:11
@alcaeus
Copy link
Member Author

alcaeus commented Jul 23, 2020

@malarzm updated UPGRADE documents and updated documentation. Mind giving it another look please?

Copy link
Member

@malarzm malarzm left a comment

Choose a reason for hiding this comment

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

Thanks, that's even more that I've asked for! 🎉

UPGRADE-3.0.md Outdated
comparable to the `Query` class.

The `Doctrine\ODM\MongoDB\Aggregation\Builder::execute()` method was deprecated
and will be removed in ODM 3.0.
Copy link
Member

Choose a reason for hiding this comment

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

In context of 3.0 this (and other sentences) should be written with the past tense I think :) But I have no problem with doing so in another PR

Copy link
Member Author

Choose a reason for hiding this comment

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

That was the original intention, but I goofed up. It's fixed now.

@alcaeus alcaeus force-pushed the aggregation-instance branch from e777dcc to dc52f23 Compare July 24, 2020 06:16
@malarzm
Copy link
Member

malarzm commented Jul 24, 2020

Thanks @alcaeus!

@malarzm malarzm merged commit 6bf710b into doctrine:master Jul 24, 2020
@alcaeus alcaeus deleted the aggregation-instance branch July 24, 2020 08:57
DavideTriso added a commit to DavideTriso/mongodb-odm that referenced this pull request Sep 21, 2020
…tance"

This reverts commit 6bf710b, reversing
changes made to 9b8c4d5.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants