Distinct in the QueryBuilder

[michaelcullum]

Q	A
Type	feature
BC Break	no

Summary

Add functionality to generate distinct queries using the DBAL query builder

[morozov]

Besides the comments below, looks good to me.

[morozov]

The API would be cleaner if instead of distinct(false), a developer could call nonDistinct(). Otherwise, it looks sort of asymmetric: distinct() ↔ distinct(false).

[IonBazan]

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

[morozov]

We can make it better and improve the ORM’s builder later.

[michaelcullum]

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.

[morozov]

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.