Merge pull request #3369 from morozov/issues/3366

Handle binding errors in OCI8Statement::execute() and MySQLiStatement::execute()
doctrine · Dec 6, 2018 · 7c38e80 · 7c38e80
2 parents d986ab4 + 57fe3fb
commit 7c38e80
Show file tree

Hide file tree

Showing 5 changed files with 69 additions and 32 deletions.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -1,5 +1,9 @@
 # Upgrade to 3.0
 
+## 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.
+
 ## BC BREAK: `Doctrine\DBAL\Types\Type::getDefaultLength()` removed
 
 The `Doctrine\DBAL\Types\Type::getDefaultLength()` method has been removed as it served no purpose.

diff --git a/lib/Doctrine/DBAL/Driver/Mysqli/MysqliStatement.php b/lib/Doctrine/DBAL/Driver/Mysqli/MysqliStatement.php
@@ -46,7 +46,7 @@ class MysqliStatement implements IteratorAggregate, Statement
     protected $_rowBindedValues;
 
     /** @var mixed[] */
-    protected $_bindedValues;
+    protected $_bindedValues = [];
 
     /** @var string */
     protected $types;
@@ -138,18 +138,18 @@ public function bindValue($param, $value, $type = ParameterType::STRING)
      */
     public function execute($params = null)
     {
-        if ($this->_bindedValues !== null) {
-            if ($params !== null) {
-                if (! $this->_bindValues($params)) {
-                    throw new MysqliException($this->_stmt->error, $this->_stmt->errno);
-                }
-            } else {
-                [$types, $values, $streams] = $this->separateBoundValues();
-                if (! $this->_stmt->bind_param($types, ...$values)) {
-                    throw new MysqliException($this->_stmt->error, $this->_stmt->sqlstate, $this->_stmt->errno);
-                }
-                $this->sendLongData($streams);
+        if ($params !== null && count($params) > 0) {
+            if (! $this->_bindValues($params)) {
+                throw new MysqliException($this->_stmt->error, $this->_stmt->errno);
             }
+        } else {
+            [$types, $values, $streams] = $this->separateBoundValues();
+
+            if (count($values) > 0 && ! $this->_stmt->bind_param($types, ...$values)) {
+                throw new MysqliException($this->_stmt->error, $this->_stmt->sqlstate, $this->_stmt->errno);
+            }
+
+            $this->sendLongData($streams);
         }
 
         if (! $this->_stmt->execute()) {

diff --git a/lib/Doctrine/DBAL/Driver/OCI8/OCI8Statement.php b/lib/Doctrine/DBAL/Driver/OCI8/OCI8Statement.php
@@ -359,9 +359,13 @@ public function execute($params = null)
             $hasZeroIndex = array_key_exists(0, $params);
             foreach ($params as $key => $val) {
                 if ($hasZeroIndex && is_numeric($key)) {
-                    $this->bindValue($key + 1, $val);
+                    $param = $key + 1;
                 } else {
-                    $this->bindValue($key, $val);
+                    $param = $key;
+                }
+
+                if (! $this->bindValue($param, $val)) {
+                    throw OCI8Exception::fromErrorInfo($this->errorInfo());
                 }
             }
         }

diff --git a/tests/Doctrine/Tests/DBAL/Driver/OCI8/OCI8StatementTest.php b/tests/Doctrine/Tests/DBAL/Driver/OCI8/OCI8StatementTest.php
@@ -41,24 +41,15 @@ public function testExecute(array $params)
             ->disableOriginalConstructor()
             ->getMock();
 
-        $statement->expects($this->at(0))
-            ->method('bindValue')
-            ->with(
-                $this->equalTo(1),
-                $this->equalTo($params[0])
-            );
-        $statement->expects($this->at(1))
-            ->method('bindValue')
-            ->with(
-                $this->equalTo(2),
-                $this->equalTo($params[1])
-            );
-        $statement->expects($this->at(2))
-            ->method('bindValue')
-            ->with(
-                $this->equalTo(3),
-                $this->equalTo($params[2])
-            );
+        foreach ($params as $index => $value) {
+            $statement->expects($this->at($index))
+                ->method('bindValue')
+                ->with(
+                    $this->equalTo($index + 1),
+                    $this->equalTo($value)
+                )
+                ->willReturn(true);
+        }
 
         // can't pass to constructor since we don't have a real database handle,
         // but execute must check the connection for the executeMode

diff --git a/tests/Doctrine/Tests/DBAL/Functional/StatementTest.php b/tests/Doctrine/Tests/DBAL/Functional/StatementTest.php
@@ -2,6 +2,7 @@
 
 namespace Doctrine\Tests\DBAL\Functional;
 
+use Doctrine\DBAL\DBALException;
 use Doctrine\DBAL\Driver\PDOOracle\Driver as PDOOracleDriver;
 use Doctrine\DBAL\Driver\Statement;
 use Doctrine\DBAL\FetchMode;
@@ -10,6 +11,7 @@
 use Doctrine\DBAL\Types\Type;
 use Doctrine\Tests\DbalFunctionalTestCase;
 use function base64_decode;
+use function sprintf;
 use function stream_get_contents;
 
 class StatementTest extends DbalFunctionalTestCase
@@ -311,4 +313,40 @@ public function testFetchInColumnMode() : void
 
         self::assertEquals(1, $result);
     }
+
+    public function testExecWithRedundantParameters() : void
+    {
+        $driver = $this->connection->getDriver()->getName();
+
+        switch ($driver) {
+            case 'pdo_mysql':
+            case 'pdo_oracle':
+            case 'pdo_sqlsrv':
+                self::markTestSkipped(sprintf(
+                    'PDOStatement::execute() implemented in the "%s" driver does not report redundant parameters',
+                    $driver
+                ));
+
+                return;
+            case 'ibm_db2':
+                self::markTestSkipped('db2_execute() does not report redundant parameters');
+
+                return;
+            case 'sqlsrv':
+                self::markTestSkipped('sqlsrv_prepare() does not report redundant parameters');
+
+                return;
+        }
+
+        $platform = $this->connection->getDatabasePlatform();
+        $query    = $platform->getDummySelectSQL();
+        $stmt     = $this->connection->prepare($query);
+
+        // we want to make sure the exception is thrown by the DBAL code, not by PHPUnit due to a PHP-level error,
+        // but the wrapper connection wraps everything in a DBAL exception
+        $this->iniSet('error_reporting', 0);
+
+        self::expectException(DBALException::class);
+        $stmt->execute([null]);
+    }
 }