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

Make addStage Aggregation Builder method public #1974

Closed
wants to merge 1 commit into from
Closed

Make addStage Aggregation Builder method public #1974

wants to merge 1 commit into from

Conversation

Steveb-p
Copy link
Contributor

Q A
Type feature
BC Break no
Fixed issues

Summary

Makes addStage method public to allow easier usage of "custom" Stage class objects.

Following discussion in https://doctrine.slack.com/archives/CAAE6T66B/p1552300833003900 / https://doctrine.slack.com/messages/CAAE6T66B/convo/CAAE6T66B-1552300833.003900/

@Steveb-p Steveb-p changed the base branch from master to 1.3.x March 11, 2019 14:41
@alcaeus alcaeus self-assigned this Mar 11, 2019
@alcaeus alcaeus added this to the 1.3.0 milestone Mar 11, 2019
@alcaeus
Copy link
Member

alcaeus commented Apr 11, 2019

Technically, I could merge this, but while taking another look I noticed that this only partially helps. Any stage method in the builder class returns the newly added stage, which works because the base Stage class implements all methods also implemented by the builder. This means that you can simply chain all method calls. However, since the base class doesn't offer a public addStage method, this is not chain able like other methods:

$builder
    ->match()
        ->field('foo')->equals('bar')
    ->addStage(new Match()) // <- fatal error because the match stage returned above does not have an addStage method
    ->group()
    // ...
;

I'm not sure this is a good idea, as it may be quite unexpected and confusing. I'm not sure this warrants another minor release of doctrine/mongodb that makes these methods public. With this in mind, I would suggest moving this back to 2.0 where we are in complete control of the API and can add the missing methods. What do you think @Steveb-p?

@alcaeus
Copy link
Member

alcaeus commented Apr 28, 2019

Closing in favour of #2020, we can't really add this in 1.3 without making the fluent API of aggregation builder confusing, as addStage would only be available on the builder, not on stages.

@alcaeus alcaeus closed this Apr 28, 2019
@alcaeus alcaeus removed this from the 1.3.0 milestone Apr 28, 2019
@Steveb-p Steveb-p deleted the make-add-stage-public branch April 28, 2019 18:57
@Steveb-p
Copy link
Contributor Author

@alcaeus Sorry I totally missed that comment in my mailbox :/ Just wanted to tell you I really appreciate your work :) Thanks for merging this to 2.0 :)

@alcaeus
Copy link
Member

alcaeus commented Apr 28, 2019

Thank you for making the initial PR - sorry we couldn't get it into 1.3. 🍻

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.

2 participants