-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Distinct in the QueryBuilder #3340
Closed
Closed
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
efd2795
Add functionality to generate distinct queries using the DBAL query b…
michaelcullum 32674ae
Fix CS violations
michaelcullum 118662d
Add tests for distinct functionality
michaelcullum 6105a00
Remove extraneous space for CS
michaelcullum 40a39aa
Add flag to enable setting distinct back to false again
michaelcullum 9b644e1
Add return type hint
michaelcullum 257d5bf
Fix CS
michaelcullum File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The API would be cleaner if instead of
distinct(false)
, a developer could callnonDistinct()
. Otherwise, it looks sort of asymmetric:distinct()
↔distinct(false)
.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.
I think he is trying to make it consistent with Doctrine ORM's QueryBuilder: https://github.com/doctrine/doctrine2/blob/6dc46e78ccf4fe75c80884e7abe41b811f2204b6/lib/Doctrine/ORM/QueryBuilder.php#L765
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.
We can make it better and improve the ORM’s builder later.
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.
I'm inclined to disagree, the main reason was for consistency with the ORM but I think the ORM behaviour makes sense.
The method is essentially a boolean setter and the most common thing it's being set to is true so the fact it defaults to true is just to make it nicer to use (
$qb->distinct()
instead of a regularly superfluous true). Assuming you don't rely on the default added for DX it is symmetric:distinct(true)
↔distinct(false)
It also means you could pass a statement straight into the argument e.g.
$qb->distinct($this->myBooleanFlag)
should you so wish instead ofif ($this->myBooleanFlag) { $qb->distinct(); } else { $qb->nonDistinct(); }
.Finally, DX again, a method called
nonDistinct()
is much less likely to actually be found by a developer compared todistinct(bool $flag)
, particularly inflated by the fact that is the ORM's behaviour.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.
It's not a setter. It's a set of builder commands which you will call or not call depending on the flag and the previous state. If this justification was valid, then instead of
assertEquals()
andassertNotEquals()
, we'd see something likeassertIfEquals(bool $equals)
in xUnit frameworks.