Distinct in the QueryBuilder#3340
Conversation
morozov
left a comment
There was a problem hiding this comment.
Besides the comments below, looks good to me.
| * | ||
| * @return $this This QueryBuilder instance. | ||
| */ | ||
| public function distinct($isDistinct = true) |
There was a problem hiding this comment.
The API would be cleaner if instead of distinct(false), a developer could call nonDistinct(). Otherwise, it looks sort of asymmetric: distinct() ↔ distinct(false).
There was a problem hiding this comment.
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.
We can make it better and improve the ORM’s builder later.
There was a problem hiding this comment.
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 of if ($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 to distinct(bool $flag), particularly inflated by the fact that is the ORM's behaviour.
There was a problem hiding this comment.
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() and assertNotEquals(), we'd see something like assertIfEquals(bool $equals) in xUnit frameworks.
Summary
Add functionality to generate distinct queries using the DBAL query builder