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

Change returned type references in query builder documentation #2091

Merged
merged 2 commits into from
Oct 26, 2019

Conversation

sarcher
Copy link
Contributor

@sarcher sarcher commented Oct 24, 2019

Q A
Type improvement
BC Break no
Fixed issues n/a

Summary

Looks like something that was missed in the 2.x changes; execute() no longer returns a cursor but an Iterator, and distinct() returns a plain array.

I do wonder if there should be an additional note for distinct() calling out that the returned type is different, since that can be a bit jarring (I was migrating a project and it took me a minute to understand exactly what was different here).

@alcaeus
Copy link
Member

alcaeus commented Oct 25, 2019

Ah, yes. Those are the kind of ugly APIs that get missed when checking for BC breaks with automated tools. Thanks for fixing the docs, and I agree that we want to add a note to this in the UPGRADE document. Can you please add that to UPGRADE-2.0.md, and change the base of this PR to be the 2.0.x branch so this is fixed in the current documentation as well? Thanks!

@alcaeus alcaeus added this to the 2.0.2 milestone Oct 25, 2019
@alcaeus alcaeus added this to 2.0 (bug fixes only) in Roadmap Oct 25, 2019
@sarcher
Copy link
Contributor Author

sarcher commented Oct 25, 2019

Should be all set; let me know if anything else is needed. Thanks!

@greg0ire greg0ire changed the base branch from master to 2.0.x October 26, 2019 07:32
@greg0ire
Copy link
Member

change the base of this PR to be the 2.0.x branch

I did it for you.

@malarzm malarzm merged commit a3d518f into doctrine:2.0.x Oct 26, 2019
@malarzm
Copy link
Member

malarzm commented Oct 26, 2019

Thank you @sarcher!

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.

None yet

4 participants