Add support for DISTINCT statement#3695
Add support for DISTINCT statement#3695bingo-soft wants to merge 4 commits intodoctrine:developfrom bingo-soft:develop
Conversation
|
That functionality is already available by using Also, is |
|
@greg0ire. There is a number of arguments for providing this functionality. First of all, in SQL Secondly, In short, yes, the point is to make it conditional easily. |
greg0ire
left a comment
There was a problem hiding this comment.
Also, is develop the correct target branch when there are no BC breaks?
Please address this
| */ | ||
| public function distinct(bool $flag = true) | ||
| { | ||
| $this->sqlParts['distinct'] = (bool) $flag; |
There was a problem hiding this comment.
I don't think this cast is needed.
| $this->sqlParts['distinct'] = (bool) $flag; | |
| $this->sqlParts['distinct'] = $flag; |
| * | ||
| * @return $this This QueryBuilder instance. | ||
| */ | ||
| public function distinct(bool $flag = true) |
There was a problem hiding this comment.
| public function distinct(bool $flag = true) | |
| final public function distinct(bool $flag = true) : self |
There was a problem hiding this comment.
Agree with self part, but not sure of final specificator. It would be the only one final method in QueryBuilder.
There was a problem hiding this comment.
Yeah but it is one of the most recent ones, right? We can wait for the advice of another maintainer if you want, I will not merge this without the approval of some other maintainers anyway.
| private function getSQLForSelect() : string | ||
| { | ||
| $query = 'SELECT ' . implode(', ', $this->sqlParts['select']); | ||
| $query = 'SELECT ' . ($this->sqlParts['distinct'] === true ? 'DISTINCT ' : '') . |
There was a problem hiding this comment.
This is already supposed to be a boolean, and if happens not to be, the bug might still go undetected. If you feel unsafe about it being a boolean, then check that and throw if it isn't, or introduce a private accessor to do the type check. If you feel safe, then simply do this:
| $query = 'SELECT ' . ($this->sqlParts['distinct'] === true ? 'DISTINCT ' : '') . | |
| $query = 'SELECT ' . ($this->sqlParts['distinct'] ? 'DISTINCT ' : '') . |
| * @var array<string, mixed> | ||
| */ | ||
| private $sqlParts = [ | ||
| 'distinct' => false, |
There was a problem hiding this comment.
I'd move it one line below, it looks like there this array is ordered with the same order in which you would find these keywords in an SQL query.
There was a problem hiding this comment.
Looks like that. You are right
| * ->from('users', 'u'); | ||
| * </code> | ||
| * | ||
| * @param bool $flag If true, then DISTINCT is added to the query |
There was a problem hiding this comment.
In what scenario is that parameter going to be useful?
There was a problem hiding this comment.
Do you mean, that there is no sense to mention this parameter in the docstring, since it is self evident? And by the way, as for develop branch, should I make a new merge request to the master branch to fix this?
There was a problem hiding this comment.
No I mean, maybe we should remove the parameter entirely?
And by the way, as for develop branch, should I make a new merge request to the master branch to fix this?
You can edit your PR to target master, and rebase your branch on it: git rebase --onto master origin/develop develop (you should probably not have named your local branch develop BTW, it makes this a bit confusing :P)
There was a problem hiding this comment.
Ups, I've never done that magic yet and made another fork for another PR. So, I think I will close this one and start another PR, if you don't mind. And I will take into account all you suggestions, probably except final key word
|
You should probably add something to https://github.com/doctrine/dbal/blob/master/docs/en/reference/query-builder.rst |
Summary
The same functionality as in Doctrine/ORM. New method
distinct($flag = true)inQueryBuilderclass and new testtestSimpleSelectWithDistinctinQueryBuilderTestare provided.