Skip to content
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.

Sign up for GitHub

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

Add support for DISTINCT statement #3695

Closed
wants to merge 4 commits into from
Closed

Add support for DISTINCT statement #3695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8495d05
Add support for DISTINCT statement
bingo-soft Oct 11, 2019
eff22d6
Fixed code style errors in Query Builder
bingo-soft Oct 11, 2019
cd03fe2
Fixed distinct method docstring
bingo-soft Oct 11, 2019
4956402
Remove unnecessary type casting in QueryBuilder distinct method
bingo-soft Oct 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion lib/Doctrine/DBAL/Query/QueryBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class QueryBuilder
* @var array<string, mixed>
*/
private $sqlParts = [
'distinct' => false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like that. You are right

'select' => [],
'from' => [],
'join' => [],
Expand Down Expand Up @@ -465,6 +466,27 @@ public function select($select = null) : self
return $this->add('select', $selects);
}

/**
* Adds a DISTINCT flag to this query.
*
* <code>
* $qb = $conn->createQueryBuilder()
* ->select('u.id')
* ->distinct()
* ->from('users', 'u');
* </code>
*
* @param bool $flag If true, then DISTINCT is added to the query
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what scenario is that parameter going to be useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*
* @return $this This QueryBuilder instance.
*/
public function distinct(bool $flag = true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function distinct(bool $flag = true)
final public function distinct(bool $flag = true) : self

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with self part, but not sure of final specificator. It would be the only one final method in QueryBuilder.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
$this->sqlParts['distinct'] = $flag;

return $this;
}

/**
* Adds an item that is to be returned in the query result.
*
Expand Down Expand Up @@ -1088,7 +1110,8 @@ public function resetQueryPart(string $queryPartName) : self
*/
private function getSQLForSelect() : string
{
$query = 'SELECT ' . implode(', ', $this->sqlParts['select']);
$query = 'SELECT ' . ($this->sqlParts['distinct'] === true ? 'DISTINCT ' : '') .
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
$query = 'SELECT ' . ($this->sqlParts['distinct'] === true ? 'DISTINCT ' : '') .
$query = 'SELECT ' . ($this->sqlParts['distinct'] ? 'DISTINCT ' : '') .

implode(', ', $this->sqlParts['select']);

$query .= ($this->sqlParts['from'] ? ' FROM ' . implode(', ', $this->getFromClauses()) : '')
. ($this->sqlParts['where'] !== null ? ' WHERE ' . ((string) $this->sqlParts['where']) : '')
Expand Down
11 changes: 11 additions & 0 deletions tests/Doctrine/Tests/DBAL/Query/QueryBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,17 @@ public function testSelectWithSimpleWhere() : void
self::assertEquals('SELECT u.id FROM users u WHERE u.nickname = ?', (string) $qb);
}

public function testSimpleSelectWithDistinct() : void
{
$qb = new QueryBuilder($this->conn);

$qb->select('u.id')
->from('users', 'u')
->distinct();

self::assertEquals('SELECT DISTINCT u.id FROM users u', (string) $qb);
}

public function testSelectWithLeftJoin() : void
{
$qb = new QueryBuilder($this->conn);
Expand Down