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

Jump to bottom

Fix wrong type assertion and annotation #3543

Closed
wants to merge 2 commits into from
Closed

Fix wrong type assertion and annotation #3543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc63986
Fix wrong type assertion and annotation
nicolas-grekas May 9, 2019
89f6987
Update typing information to allow for PDO classes
alcaeus May 10, 2019
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
18 changes: 12 additions & 6 deletions lib/Doctrine/DBAL/Cache/ResultCacheStatement.php
View file
Open in desktop
Expand Up @@ -10,9 +10,11 @@
use InvalidArgumentException;
use IteratorAggregate;
use PDO;
use PDOStatement;
use function array_merge;
use function array_values;
use function assert;
use function is_array;
use function reset;

/**
Expand Down Expand Up @@ -42,7 +44,7 @@ class ResultCacheStatement implements IteratorAggregate, ResultStatement
/** @var int */
private $lifetime;

/** @var ResultStatement */
/** @var ResultStatement|PDOStatement */
private $statement;

/**
Expand All @@ -59,11 +61,12 @@ class ResultCacheStatement implements IteratorAggregate, ResultStatement
private $defaultFetchMode = FetchMode::MIXED;

/**
* @param string $cacheKey
* @param string $realKey
* @param int $lifetime
* @param ResultStatement|PDOStatement $stmt
Copy link
Member

Choose a reason for hiding this comment

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

I wanted to add an assert statement to the constructor to check for this, but apparently PHPStan thinks it's redundant. The code in question would be

assert($stmt instanceof ResultStatement || $stmt instanceof PDOStatement);

Do we want to add it and silence the error or leave it as is?

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be removed and the type hint added back, isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

I agree here, it looks like we're missing an assert on the caller and not here.

* @param string $cacheKey
* @param string $realKey
* @param int $lifetime
*/
public function __construct(ResultStatement $stmt, Cache $resultCache, $cacheKey, $realKey, $lifetime)
public function __construct($stmt, Cache $resultCache, $cacheKey, $realKey, $lifetime)
Copy link
Member

Choose a reason for hiding this comment

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

Even when passing the PDO instance we modify the statement class, so I'd say we shouldn't remove this type hint here.

Copy link
Member

Choose a reason for hiding this comment

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

PHPStan was complaining about it, that's why I had to modify it.

{
$this->statement = $stmt;
$this->resultCache = $resultCache;
Expand Down Expand Up @@ -167,7 +170,10 @@ public function fetch($fetchMode = null, $cursorOrientation = PDO::FETCH_ORI_NEX
*/
public function fetchAll($fetchMode = null, $fetchArgument = null, $ctorArgs = null)
{
$this->data = $this->statement->fetchAll($fetchMode, $fetchArgument, $ctorArgs);
$data = $this->statement->fetchAll($fetchMode, $fetchArgument, $ctorArgs);
assert(is_array($data));

$this->data = $data;
$this->emptied = true;

return $this->data;
Expand Down
27 changes: 18 additions & 9 deletions lib/Doctrine/DBAL/Connection.php
View file
Open in desktop
Expand Up @@ -20,11 +20,14 @@
use Doctrine\DBAL\Schema\AbstractSchemaManager;
use Doctrine\DBAL\Types\Type;
use Exception;
use PDO;
use PDOStatement;
use Throwable;
use function array_key_exists;
use function assert;
use function func_get_args;
use function implode;
use function is_array;
use function is_int;
use function is_string;
use function key;
Expand Down Expand Up @@ -82,7 +85,7 @@ class Connection implements DriverConnection
/**
* The wrapped driver connection.
*
* @var \Doctrine\DBAL\Driver\Connection|null
* @var \Doctrine\DBAL\Driver\Connection|PDO|null
*/
protected $_conn;

Expand Down Expand Up @@ -831,7 +834,10 @@ public function quote($input, $type = null)
*/
public function fetchAll($sql, array $params = [], $types = [])
{
return $this->executeQuery($sql, $params, $types)->fetchAll();
$data = $this->executeQuery($sql, $params, $types)->fetchAll();
assert(is_array($data));

return $data;
}

/**
Expand Down Expand Up @@ -867,7 +873,7 @@ public function prepare($statement)
* @param int[]|string[] $types The types the previous parameters are in.
* @param QueryCacheProfile|null $qcp The query cache profile, optional.
*
* @return ResultStatement The executed statement.
* @return ResultStatement|PDOStatement The executed statement.
*
* @throws DBALException
*/
Expand Down Expand Up @@ -902,6 +908,8 @@ public function executeQuery($query, array $params = [], $types = [], ?QueryCach
throw DBALException::driverExceptionDuringQuery($this->_driver, $ex, $query, $this->resolveParams($params, $types));
}

assert($stmt instanceof ResultStatement || $stmt instanceof PDOStatement);

$stmt->setFetchMode($this->defaultFetchMode);

if ($logger) {
Expand Down Expand Up @@ -986,7 +994,7 @@ public function project($query, array $params, Closure $function)
/**
* Executes an SQL statement, returning a result set as a Statement object.
*
* @return \Doctrine\DBAL\Driver\Statement
* @return DriverStatement|PDOStatement
Copy link
Member

Choose a reason for hiding this comment

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

The Interface containing query() only expects a return type of Doctrine\DBAL\Driver\Statement. Should PDOStatement be added to the interface too?

*
* @throws DBALException
*/
Expand All @@ -1007,6 +1015,7 @@ public function query()
throw DBALException::driverExceptionDuringQuery($this->_driver, $ex, $args[0]);
}

assert($statement instanceof PDOStatement || $statement instanceof DriverStatement);
$statement->setFetchMode($this->defaultFetchMode);

if ($logger) {
Expand Down Expand Up @@ -1424,12 +1433,12 @@ public function rollbackSavepoint($savepoint)
/**
* Gets the wrapped driver connection.
*
* @return DriverConnection
* @return DriverConnection|PDO
*/
public function getWrappedConnection()
{
$this->connect();
assert($this->_conn instanceof DriverConnection);
assert($this->_conn instanceof DriverConnection || $this->_conn instanceof PDO);

return $this->_conn;
}
Expand Down Expand Up @@ -1516,9 +1525,9 @@ public function convertToPHPValue($value, $type)
* @internal Duck-typing used on the $stmt parameter to support driver statements as well as
* raw PDOStatement instances.
*
* @param \Doctrine\DBAL\Driver\Statement $stmt The statement to bind the values to.
* @param mixed[] $params The map/list of named/positional parameters.
* @param int[]|string[] $types The parameter types (PDO binding types or DBAL mapping types).
* @param DriverStatement|PDOStatement $stmt The statement to bind the values to.
* @param mixed[] $params The map/list of named/positional parameters.
* @param int[]|string[] $types The parameter types (PDO binding types or DBAL mapping types).
*
* @return void
*/
Expand Down
3 changes: 3 additions & 0 deletions lib/Doctrine/DBAL/Portability/Connection.php
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@
use PDO;
use const CASE_LOWER;
use const CASE_UPPER;
use function assert;
use function func_get_args;

/**
Expand Down Expand Up @@ -124,6 +125,8 @@ public function query()
$connection = $this->getWrappedConnection();

$stmt = $connection->query(...func_get_args());
assert($stmt !== false);

$stmt = new Statement($stmt, $this);
$stmt->setFetchMode($this->defaultFetchMode);

Expand Down
8 changes: 6 additions & 2 deletions lib/Doctrine/DBAL/Portability/Statement.php
View file
Open in desktop
Expand Up @@ -9,8 +9,10 @@
use Doctrine\DBAL\ParameterType;
use IteratorAggregate;
use PDO;
use PDOStatement;
use function array_change_key_case;
use function assert;
use function is_array;
use function is_string;
use function rtrim;

Expand All @@ -22,7 +24,7 @@ class Statement implements IteratorAggregate, DriverStatement
/** @var int */
private $portability;

/** @var DriverStatement|ResultStatement */
/** @var DriverStatement|ResultStatement|PDOStatement */
private $stmt;

/** @var int */
Expand All @@ -34,7 +36,7 @@ class Statement implements IteratorAggregate, DriverStatement
/**
* Wraps <tt>Statement</tt> and applies portability measures.
*
* @param DriverStatement|ResultStatement $stmt
* @param DriverStatement|ResultStatement|PDOStatement $stmt
*/
public function __construct($stmt, Connection $conn)
{
Expand Down Expand Up @@ -159,6 +161,8 @@ public function fetchAll($fetchMode = null, $fetchArgument = null, $ctorArgs = n
$rows = $this->stmt->fetchAll($fetchMode);
}

assert(is_array($rows));

$iterateRow = $this->portability & (Connection::PORTABILITY_EMPTY_TO_NULL|Connection::PORTABILITY_RTRIM);
$fixCase = $this->case !== null
&& ($fetchMode === FetchMode::ASSOCIATIVE || $fetchMode === FetchMode::MIXED)
Expand Down
5 changes: 3 additions & 2 deletions lib/Doctrine/DBAL/Query/QueryBuilder.php
View file
Open in desktop
Expand Up @@ -3,10 +3,11 @@
namespace Doctrine\DBAL\Query;

use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Driver\Statement;
use Doctrine\DBAL\Driver\ResultStatement;
use Doctrine\DBAL\ParameterType;
use Doctrine\DBAL\Query\Expression\CompositeExpression;
use Doctrine\DBAL\Query\Expression\ExpressionBuilder;
use PDOStatement;
use function array_key_exists;
use function array_keys;
use function array_unshift;
Expand Down Expand Up @@ -192,7 +193,7 @@ public function getState()
* Uses {@see Connection::executeQuery} for select statements and {@see Connection::executeUpdate}
* for insert, update and delete statements.
*
* @return Statement|int
* @return ResultStatement|PDOStatement|int
*/
public function execute()
{
Expand Down
7 changes: 6 additions & 1 deletion lib/Doctrine/DBAL/Schema/PostgreSqlSchemaManager.php
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@
use function explode;
use function implode;
use function in_array;
use function is_array;
use function preg_match;
use function preg_replace;
use function sprintf;
Expand All @@ -43,7 +44,10 @@ public function getSchemaNames()
{
$statement = $this->_conn->executeQuery("SELECT nspname FROM pg_namespace WHERE nspname !~ '^pg_.*' AND nspname != 'information_schema'");

return $statement->fetchAll(FetchMode::COLUMN);
$schemaNames = $statement->fetchAll(FetchMode::COLUMN);
assert(is_array($schemaNames));

return $schemaNames;
}

/**
Expand Down Expand Up @@ -222,6 +226,7 @@ protected function _getPortableTableIndexesList($tableIndexes, $tableName = null

$stmt = $this->_conn->executeQuery($columnNameSql);
$indexColumns = $stmt->fetchAll();
assert(is_array($indexColumns));

// required for getting the order of the columns right.
foreach ($colNumbers as $colNum) {
Expand Down
14 changes: 9 additions & 5 deletions lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php
View file
Open in desktop
Expand Up @@ -13,8 +13,10 @@
use function array_map;
use function array_reverse;
use function array_values;
use function assert;
use function explode;
use function file_exists;
use function is_array;
use function preg_match;
use function preg_match_all;
use function preg_quote;
Expand Down Expand Up @@ -173,6 +175,7 @@ protected function _getPortableTableIndexesList($tableIndexes, $tableName = null
$this->_conn->quote($tableName)
));
$indexArray = $stmt->fetchAll(FetchMode::ASSOCIATIVE);
assert(is_array($indexArray));

usort($indexArray, static function ($a, $b) {
if ($a['pk'] === $b['pk']) {
Expand Down Expand Up @@ -207,11 +210,12 @@ protected function _getPortableTableIndexesList($tableIndexes, $tableName = null
$idx['primary'] = false;
$idx['non_unique'] = ! $tableIndex['unique'];

$stmt = $this->_conn->executeQuery(sprintf(
'PRAGMA INDEX_INFO (%s)',
$this->_conn->quote($keyName)
));
$indexArray = $stmt->fetchAll(FetchMode::ASSOCIATIVE);
$stmt = $this->_conn->executeQuery(sprintf(
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
$stmt = $this->_conn->executeQuery(sprintf(
$stmt = $this->_conn->executeQuery(sprintf(

Because of the newline above this, I assume that the spaces aren't needed here.

'PRAGMA INDEX_INFO (%s)',
$this->_conn->quote($keyName)
));
$indexArray = $stmt->fetchAll(FetchMode::ASSOCIATIVE);
assert(is_array($indexArray));

foreach ($indexArray as $indexColumnRow) {
$idx['column_name'] = $indexColumnRow['name'];
Expand Down
13 changes: 7 additions & 6 deletions lib/Doctrine/DBAL/Statement.php
View file
Open in desktop
Expand Up @@ -7,7 +7,9 @@
use Doctrine\DBAL\Types\Type;
use IteratorAggregate;
use PDO;
use PDOStatement;
use Throwable;
use function assert;
use function is_array;
use function is_string;

Expand Down Expand Up @@ -41,7 +43,7 @@ class Statement implements IteratorAggregate, DriverStatement
/**
* The underlying driver statement.
*
* @var \Doctrine\DBAL\Driver\Statement
* @var \Doctrine\DBAL\Driver\Statement|PDOStatement
*/
protected $stmt;

Expand Down Expand Up @@ -249,11 +251,10 @@ public function fetch($fetchMode = null, $cursorOrientation = PDO::FETCH_ORI_NEX
*/
public function fetchAll($fetchMode = null, $fetchArgument = null, $ctorArgs = null)
{
if ($fetchArgument) {
return $this->stmt->fetchAll($fetchMode, $fetchArgument);
}
$data = $fetchArgument ? $this->stmt->fetchAll($fetchMode, $fetchArgument) : $this->stmt->fetchAll($fetchMode);
assert(is_array($data));

return $this->stmt->fetchAll($fetchMode);
return $data;
}

/**
Expand All @@ -277,7 +278,7 @@ public function rowCount()
/**
* Gets the wrapped driver statement.
*
* @return \Doctrine\DBAL\Driver\Statement
* @return \Doctrine\DBAL\Driver\Statement|PDOStatement
*/
public function getWrappedStatement()
{
Expand Down
6 changes: 3 additions & 3 deletions tests/Doctrine/Tests/DBAL/DriverManagerTest.php
View file
Open in desktop
Expand Up @@ -38,11 +38,11 @@ public function testInvalidPdoInstance()
*/
public function testValidPdoInstance()
{
$conn = DriverManager::getConnection([
'pdo' => new PDO('sqlite::memory:'),
]);
$pdo = new PDO('sqlite::memory:');
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we need more tests covering the basic DBAL features for when an instance is used?

$conn = DriverManager::getConnection(['pdo' => $pdo]);

self::assertEquals('sqlite', $conn->getDatabasePlatform()->getName());
self::assertSame($pdo, $conn->getWrappedConnection());
}

/**
Expand Down