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.

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

Predictable QueryBuilder::executeQuery() and QueryBuilder::executeStatement() #4578

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 17 additions & 0 deletions UPGRADE.md
Expand Up @@ -36,6 +36,23 @@ Use `Connection::createSchemaManager()` instead.
The usage of `Connection::$_expr` and `Connection::getExpressionBuilder()` is deprecated.
Use `Connection::createExpressionBuilder()` instead.

## Deprecated `QueryBuilder::execute()`

The usage of `QueryBuilder::execute()` is deprecated. Use either `QueryBuilder::executeQuery()` or
`QueryBuilder::executeStatement()`, depending on whether the queryBuilder is a query (SELECT) or a statement (INSERT,
UPDATE, DELETE).

You might also consider the use of the new shortcut methods, such as:

- `fetchAllAssociative()`
- `fetchAllAssociativeIndexed()`
- `fetchAllKeyValue()`
- `fetchAllNumeric()`
- `fetchAssociative()`
- `fetchFirstColumn()`
- `fetchNumeric()`
- `fetchOne()`

# Upgrade to 3.0

## BC BREAK: leading colon in named parameter names not supported
Expand Down
38 changes: 38 additions & 0 deletions src/Query/QueryBuilder.php
Expand Up @@ -300,19 +300,57 @@ public function fetchFirstColumn(): array
return $this->connection->fetchFirstColumn($this->getSQL(), $this->params, $this->paramTypes);
}

/**
* Executes an SQL query (SELECT) and returns a Result.
Copy link
Member

Choose a reason for hiding this comment

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

This may be misleading since it works also for DESCRIBE, SHOW and so on. Let's rephrase to:

Executes an SQL query that returns a Result.

Copy link
Contributor Author

@PowerKiKi PowerKiKi Apr 6, 2021

Choose a reason for hiding this comment

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

I'd agree with that on the Connection class, but AFAIK, there is no way to build anything else than a SELECT query with a QueryBuilder. So I don't think we should document an impossible use-case.

WDYT ?

see:

/*
* The query types.
*/
public const SELECT = 0;
public const DELETE = 1;
public const UPDATE = 2;
public const INSERT = 3;

Copy link
Member

Choose a reason for hiding this comment

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

You're right, I didn't take the scope into account. Sorry for the confusion.

*
* @throws Exception
*/
public function executeQuery(): Result
{
return $this->connection->executeQuery($this->getSQL(), $this->params, $this->paramTypes);
}

/**
* Executes an SQL statement and returns the number of affected rows.
*
* Should be used for INSERT, UPDATE and DELETE
Copy link
Member

Choose a reason for hiding this comment

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

It's not limited to DML (INSERT, etc.), it could be also DDL and other non-DQL statements. See JDBC for reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idem, the QueryBuilder cannot build anything else than DML. Should we really mention that ?

Copy link
Member

Choose a reason for hiding this comment

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

No, you're right.

*
* @return int The number of affected rows.
*
* @throws Exception
*/
public function executeStatement(): int
{
return $this->connection->executeStatement($this->getSQL(), $this->params, $this->paramTypes);
}

/**
* Executes this query using the bound parameters and their types.
*
* @deprecated Use {@link executeQuery()} or {@link executeStatement()} instead.
*
* @return Result|int
*
* @throws Exception
*/
public function execute()
{
if ($this->type === self::SELECT) {
Deprecation::trigger(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/4578',
'QueryBuilder::execute() is deprecated, use QueryBuilder::executeQuery() for SQL queries instead.'
);

return $this->connection->executeQuery($this->getSQL(), $this->params, $this->paramTypes);
}

Deprecation::trigger(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/4578',
'QueryBuilder::execute() is deprecated, use QueryBuilder::executeStatement() for SQL statements instead.'
);

return $this->connection->executeStatement($this->getSQL(), $this->params, $this->paramTypes);
}

Expand Down
69 changes: 69 additions & 0 deletions tests/Query/QueryBuilderTest.php
Expand Up @@ -7,6 +7,7 @@
use Doctrine\DBAL\Query\Expression\ExpressionBuilder;
use Doctrine\DBAL\Query\QueryBuilder;
use Doctrine\DBAL\Query\QueryException;
use Doctrine\DBAL\Result;
use Doctrine\DBAL\Types\Type;
use Doctrine\DBAL\Types\Types;
use PHPUnit\Framework\MockObject\MockObject;
Expand Down Expand Up @@ -1302,4 +1303,72 @@ public static function fetchProvider(): iterable
'SELECT id, username FROM user WHERE password = :password AND username != :username AND id != :id',
];
}

/**
* @param list<mixed>|array<string, mixed> $parameters
* @param array<int, int|string|Type|null>|array<string, int|string|Type|null> $parameterTypes
*
* @dataProvider fetchProvider
*/
public function testExecuteQuery(
string $select,
string $from,
string $where,
array $parameters,
array $parameterTypes,
string $expectedSql
): void {
$qb = new QueryBuilder($this->conn);
$mockedResult = $this->createMock(Result::class);

$this->conn->expects(self::once())
->method('executeQuery')
->with($expectedSql, $parameters, $parameterTypes)
->willReturn($mockedResult);

$results = $qb->select($select)
->from($from)
->where($where)
->setParameters($parameters, $parameterTypes)
->executeQuery();

self::assertSame(
$mockedResult,
$results
);
}

public function testExecuteStatement(): void
{
$qb = new QueryBuilder($this->conn);
$mockedResult = 123;
$expectedSql = 'UPDATE users SET foo = ?, bar = ? WHERE bar = 1';

$parameters = [
'foo' => 'jwage',
'bar' => false,
];

$parameterTypes = [
'foo' => Types::STRING,
'bar' => Types::BOOLEAN,
];

$this->conn->expects(self::once())
->method('executeStatement')
->with($expectedSql, $parameters, $parameterTypes)
->willReturn($mockedResult);

$results = $qb->update('users')
->set('foo', '?')
->set('bar', '?')
->where('bar = 1')
->setParameters($parameters, $parameterTypes)
->executeStatement();

self::assertSame(
$mockedResult,
$results
);
}
}