Merge pull request #3417 from morozov/issues/3358

Statement::fetchColumn() will throw an exception in the case of invalid index
doctrine · Mar 18, 2019 · cd7df71 · cd7df71
2 parents 7041a7c + f5cc335
commit cd7df71
Show file tree

Hide file tree

Showing 12 changed files with 115 additions and 10 deletions.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -1,5 +1,9 @@
 # Upgrade to 3.0
 
+## MINOR BC BREAK `Statement::fetchColumn()` with an invalid index.
+
+Similarly to `PDOStatement::fetchColumn()`, DBAL statements throw an exception in case of an invalid column index.
+
 ## BC BREAK `Statement::execute()` with redundant parameters.
 
 Similarly to the drivers based on `pdo_pgsql` and `pdo_sqlsrv`, `OCI8Statement::execute()` and `MySQLiStatement::execute()` do not longer ignore redundant parameters.

diff --git a/lib/Doctrine/DBAL/Cache/ArrayStatement.php b/lib/Doctrine/DBAL/Cache/ArrayStatement.php
@@ -3,10 +3,12 @@
 namespace Doctrine\DBAL\Cache;
 
 use ArrayIterator;
+use Doctrine\DBAL\DBALException;
 use Doctrine\DBAL\Driver\ResultStatement;
 use Doctrine\DBAL\FetchMode;
 use InvalidArgumentException;
 use IteratorAggregate;
+use function array_key_exists;
 use function array_merge;
 use function array_values;
 use function count;
@@ -130,7 +132,14 @@ public function fetchColumn($columnIndex = 0)
     {
         $row = $this->fetch(FetchMode::NUMERIC);
 
-        // TODO: verify that return false is the correct behavior
-        return $row[$columnIndex] ?? false;
+        if ($row === false) {
+            return false;
+        }
+
+        if (! array_key_exists($columnIndex, $row)) {
+            throw DBALException::invalidColumnIndex($columnIndex, count($row));
+        }
+
+        return $row[$columnIndex];
     }
 }
diff --git a/lib/Doctrine/DBAL/Cache/ResultCacheStatement.php b/lib/Doctrine/DBAL/Cache/ResultCacheStatement.php
@@ -4,14 +4,17 @@
 
 use ArrayIterator;
 use Doctrine\Common\Cache\Cache;
+use Doctrine\DBAL\DBALException;
 use Doctrine\DBAL\Driver\ResultStatement;
 use Doctrine\DBAL\Driver\Statement;
 use Doctrine\DBAL\FetchMode;
 use InvalidArgumentException;
 use IteratorAggregate;
+use function array_key_exists;
 use function array_merge;
 use function array_values;
 use function assert;
+use function count;
 use function reset;
 
 /**
@@ -179,8 +182,15 @@ public function fetchColumn($columnIndex = 0)
     {
         $row = $this->fetch(FetchMode::NUMERIC);
 
-        // TODO: verify that return false is the correct behavior
-        return $row[$columnIndex] ?? false;
+        if ($row === false) {
+            return false;
+        }
+
+        if (! array_key_exists($columnIndex, $row)) {
+            throw DBALException::invalidColumnIndex($columnIndex, count($row));
+        }
+
+        return $row[$columnIndex];
     }
 
     /**

diff --git a/lib/Doctrine/DBAL/DBALException.php b/lib/Doctrine/DBAL/DBALException.php
@@ -277,4 +277,14 @@ public static function typeNotFound($name)
     {
         return new self('Type to be overwritten ' . $name . ' does not exist.');
     }
+
+    public static function invalidColumnIndex(int $index, int $count) : self
+    {
+        return new self(sprintf(
+            'Invalid column index %d. The statement result contains %d column%s.',
+            $index,
+            $count,
+            $count === 1 ? '' : 's'
+        ));
+    }
 }
diff --git a/lib/Doctrine/DBAL/Driver/IBMDB2/DB2Statement.php b/lib/Doctrine/DBAL/Driver/IBMDB2/DB2Statement.php
@@ -2,6 +2,7 @@
 
 namespace Doctrine\DBAL\Driver\IBMDB2;
 
+use Doctrine\DBAL\DBALException;
 use Doctrine\DBAL\Driver\Statement;
 use Doctrine\DBAL\Driver\StatementIterator;
 use Doctrine\DBAL\FetchMode;
@@ -18,6 +19,7 @@
 use const DB2_PARAM_FILE;
 use const DB2_PARAM_IN;
 use function array_change_key_case;
+use function array_key_exists;
 use function count;
 use function db2_bind_param;
 use function db2_execute;
@@ -338,7 +340,11 @@ public function fetchColumn($columnIndex = 0)
             return false;
         }
 
-        return $row[$columnIndex] ?? null;
+        if (! array_key_exists($columnIndex, $row)) {
+            throw DBALException::invalidColumnIndex($columnIndex, count($row));
+        }
+
+        return $row[$columnIndex];
     }
 
     /**

diff --git a/lib/Doctrine/DBAL/Driver/Mysqli/MysqliStatement.php b/lib/Doctrine/DBAL/Driver/Mysqli/MysqliStatement.php
@@ -2,6 +2,7 @@
 
 namespace Doctrine\DBAL\Driver\Mysqli;
 
+use Doctrine\DBAL\DBALException;
 use Doctrine\DBAL\Driver\Statement;
 use Doctrine\DBAL\Driver\StatementIterator;
 use Doctrine\DBAL\Exception\InvalidArgumentException;
@@ -12,6 +13,7 @@
 use mysqli_stmt;
 use function array_combine;
 use function array_fill;
+use function array_key_exists;
 use function assert;
 use function count;
 use function feof;
@@ -380,7 +382,11 @@ public function fetchColumn($columnIndex = 0)
             return false;
         }
 
-        return $row[$columnIndex] ?? null;
+        if (! array_key_exists($columnIndex, $row)) {
+            throw DBALException::invalidColumnIndex($columnIndex, count($row));
+        }
+
+        return $row[$columnIndex];
     }
 
     /**

diff --git a/lib/Doctrine/DBAL/Driver/OCI8/OCI8Statement.php b/lib/Doctrine/DBAL/Driver/OCI8/OCI8Statement.php
@@ -2,6 +2,7 @@
 
 namespace Doctrine\DBAL\Driver\OCI8;
 
+use Doctrine\DBAL\DBALException;
 use Doctrine\DBAL\Driver\Statement;
 use Doctrine\DBAL\Driver\StatementIterator;
 use Doctrine\DBAL\FetchMode;
@@ -517,7 +518,11 @@ public function fetchColumn($columnIndex = 0)
             return false;
         }
 
-        return $row[$columnIndex] ?? null;
+        if (! array_key_exists($columnIndex, $row)) {
+            throw DBALException::invalidColumnIndex($columnIndex, count($row));
+        }
+
+        return $row[$columnIndex];
     }
 
     /**

diff --git a/lib/Doctrine/DBAL/Driver/PDOStatement.php b/lib/Doctrine/DBAL/Driver/PDOStatement.php
@@ -2,6 +2,7 @@
 
 namespace Doctrine\DBAL\Driver;
 
+use Doctrine\DBAL\DBALException;
 use Doctrine\DBAL\FetchMode;
 use Doctrine\DBAL\ParameterType;
 use IteratorAggregate;
@@ -193,7 +194,17 @@ public function fetchAll($fetchMode = null, ...$args)
     public function fetchColumn($columnIndex = 0)
     {
         try {
-            return $this->stmt->fetchColumn($columnIndex);
+            $value = $this->stmt->fetchColumn($columnIndex);
+
+            if ($value === null) {
+                $columnCount = $this->columnCount();
+
+                if ($columnIndex < 0 || $columnIndex >= $columnCount) {
+                    throw DBALException::invalidColumnIndex($columnIndex, $columnCount);
+                }
+            }
+
+            return $value;
         } catch (\PDOException $exception) {
             throw new PDOException($exception);
         }

diff --git a/lib/Doctrine/DBAL/Driver/SQLAnywhere/SQLAnywhereStatement.php b/lib/Doctrine/DBAL/Driver/SQLAnywhere/SQLAnywhereStatement.php
@@ -2,6 +2,7 @@
 
 namespace Doctrine\DBAL\Driver\SQLAnywhere;
 
+use Doctrine\DBAL\DBALException;
 use Doctrine\DBAL\Driver\Statement;
 use Doctrine\DBAL\Driver\StatementIterator;
 use Doctrine\DBAL\FetchMode;
@@ -284,7 +285,9 @@ public function fetchColumn($columnIndex = 0)
             return false;
         }
 
-        return $row[$columnIndex] ?? null;
+        if (! array_key_exists($columnIndex, $row)) {
+            throw DBALException::invalidColumnIndex($columnIndex, count($row));
+        }
     }
 
     /**

diff --git a/lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvStatement.php b/lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvStatement.php
@@ -2,6 +2,7 @@
 
 namespace Doctrine\DBAL\Driver\SQLSrv;
 
+use Doctrine\DBAL\DBALException;
 use Doctrine\DBAL\Driver\Statement;
 use Doctrine\DBAL\Driver\StatementIterator;
 use Doctrine\DBAL\FetchMode;
@@ -410,7 +411,11 @@ public function fetchColumn($columnIndex = 0)
             return false;
         }
 
-        return $row[$columnIndex] ?? null;
+        if (! array_key_exists($columnIndex, $row)) {
+            throw DBALException::invalidColumnIndex($columnIndex, count($row));
+        }
+
+        return $row[$columnIndex];
     }
 
     /**

diff --git a/phpstan.neon.dist b/phpstan.neon.dist
@@ -65,3 +65,8 @@ parameters:
 
         # https://github.com/doctrine/dbal/issues/3237
         - '~^Call to an undefined method Doctrine\\DBAL\\Driver\\PDOStatement::nextRowset\(\)~'
+
+        # https://github.com/phpstan/phpstan/pull/1886
+        -
+            message: '~^Strict comparison using === between string|false and null will always evaluate to false\.~'
+            path: %currentWorkingDirectory%/lib/Doctrine/DBAL/Driver/PDOStatement.php
diff --git a/tests/Doctrine/Tests/DBAL/Functional/StatementTest.php b/tests/Doctrine/Tests/DBAL/Functional/StatementTest.php
@@ -3,6 +3,7 @@
 namespace Doctrine\Tests\DBAL\Functional;
 
 use Doctrine\DBAL\DBALException;
+use Doctrine\DBAL\Driver\PDOConnection;
 use Doctrine\DBAL\Driver\PDOOracle\Driver as PDOOracleDriver;
 use Doctrine\DBAL\Driver\Statement;
 use Doctrine\DBAL\FetchMode;
@@ -349,4 +350,34 @@ public function testExecWithRedundantParameters() : void
         self::expectException(DBALException::class);
         $stmt->execute([null]);
     }
+
+    /**
+     * @throws DBALException
+     *
+     * @dataProvider nonExistingIndexProvider
+     */
+    public function testFetchColumnNonExistingIndex(int $index) : void
+    {
+        if ($this->connection->getWrappedConnection() instanceof PDOConnection) {
+            $this->markTestSkipped('PDO supports this behavior natively but throws a different exception');
+        }
+
+        $platform = $this->connection->getDatabasePlatform();
+        $query    = $platform->getDummySelectSQL();
+        $stmt     = $this->connection->query($query);
+
+        self::expectException(DBALException::class);
+        $stmt->fetchColumn($index);
+    }
+
+    /**
+     * @return mixed[][]
+     */
+    public static function nonExistingIndexProvider() : iterable
+    {
+        return [
+            [1],
+            [-1],
+        ];
+    }
 }