-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Don't merge PersistentCollection orderBy with criteria in matching() #7850
Don't merge PersistentCollection orderBy with criteria in matching() #7850
Conversation
dc452c4
to
cfb357f
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.
@nlx-lars thank you for this well-crafted changeset! Not merging arrays is indeed a more sensible option since it removes potential WTFs (thanks @SenseException).
I just have this tiny request 👍
|
||
$children = $parent->getChildren()->matching(Criteria::create()); | ||
|
||
self::assertEquals(100, $children[0]->position); |
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.
Please use self::assertSame()
to be a bit more strict (yay strictness 🤘)
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.
done!
If no orderings are given to PersistentCollection::matching(), the orderBy annotation will be used if present. If the criteria contains orderings, those will be used without merging them with the orderBy. See doctrine#7836
cfb357f
to
79a7ecc
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.
Perfect, thanks!
If no orderings are given to PersistentCollection::matching(), the
orderBy annotation will be used if present. If the criteria contains
orderings, those will be used without merging them with the orderBy.
See #7836