Fix inconsistency in PDO transaction state

This addresses an issue introduced by #4996 and reported in
https://bugs.php.net/bug.php?id=80260.

Now that PDO::inTransaction() reports the real transaction state
of the connection, there may be a mismatch with PDOs internal
transaction state (in_tcx). This is compounded by the fact that
MySQL performs implicit commits for DDL queries.

This patch fixes the issue by making beginTransaction/commit/rollBack
work on the real transaction state provided by the driver as well
(or falling back to in_tcx if the driver does not support it).

This does mean that writing something like

    $pdo->beginTransaction();
    $pdo->exec('CREATE DATABASE ...');
    $pdo->rollBack(); // <- illegal

will now result in an error, because the CREATE DATABASE already
committed the transaction. I believe this behavior is both correct
and desired -- otherwise, there is no indication that the code did
not behave correctly and the rollBack() was effectively ignored.
However, this is also a BC break.

Closes GH-6355.

4 files changed, 78 insertions, 21 deletions

diff --git a/ext/pdo/pdo_dbh.c b/ext/pdo/pdo_dbh.c
index a0c2b74c33..eb77693f5b 100644
--- a/ext/pdo/pdo_dbh.c
+++ b/ext/pdo/pdo_dbh.c
@@ -577,6 +577,14 @@ PHP_METHOD(PDO, prepare)
 }
 /* }}} */
 
+
+static zend_bool pdo_is_in_transaction(pdo_dbh_t *dbh) {
+	if (dbh->methods->in_transaction) {
+		return dbh->methods->in_transaction(dbh);
+	}
+	return dbh->in_txn;
+}
+
 /* {{{ Initiates a transaction */
 PHP_METHOD(PDO, beginTransaction)
 {
@@ -586,7 +594,7 @@ PHP_METHOD(PDO, beginTransaction)
 
 	PDO_CONSTRUCT_CHECK;
 
-	if (dbh->in_txn) {
+	if (pdo_is_in_transaction(dbh)) {
 		zend_throw_exception_ex(php_pdo_get_exception(), 0, "There is already an active transaction");
 		RETURN_THROWS();
 	}
@@ -617,7 +625,7 @@ PHP_METHOD(PDO, commit)
 
 	PDO_CONSTRUCT_CHECK;
 
-	if (!dbh->in_txn) {
+	if (!pdo_is_in_transaction(dbh)) {
 		zend_throw_exception_ex(php_pdo_get_exception(), 0, "There is no active transaction");
 		RETURN_THROWS();
 	}
@@ -641,7 +649,7 @@ PHP_METHOD(PDO, rollBack)
 
 	PDO_CONSTRUCT_CHECK;
 
-	if (!dbh->in_txn) {
+	if (!pdo_is_in_transaction(dbh)) {
 		zend_throw_exception_ex(php_pdo_get_exception(), 0, "There is no active transaction");
 		RETURN_THROWS();
 	}
@@ -665,11 +673,7 @@ PHP_METHOD(PDO, inTransaction)
 
 	PDO_CONSTRUCT_CHECK;
 
-	if (!dbh->methods->in_transaction) {
-		RETURN_BOOL(dbh->in_txn);
-	}
-
-	RETURN_BOOL(dbh->methods->in_transaction(dbh));
+	RETURN_BOOL(pdo_is_in_transaction(dbh));
 }
 /* }}} */
 
diff --git a/ext/pdo_mysql/tests/pdo_mysql_commit.phpt b/ext/pdo_mysql/tests/pdo_mysql_commit.phpt
index 506be6475f..f88ae149fa 100644
--- a/ext/pdo_mysql/tests/pdo_mysql_commit.phpt
+++ b/ext/pdo_mysql/tests/pdo_mysql_commit.phpt
@@ -23,9 +23,14 @@ if (false == MySQLPDOTest::detect_transactional_mysql_engine($db))
         // DDL will issue an implicit commit
         $db->exec(sprintf('DROP TABLE IF EXISTS test_commit'));
         $db->exec(sprintf('CREATE TABLE test_commit(id INT) ENGINE=%s', MySQLPDOTest::detect_transactional_mysql_engine($db)));
-        if (true !== ($tmp = $db->commit())) {
-            printf("[002] No commit allowed? [%s] %s\n",
-                $db->errorCode(), implode(' ', $db->errorInfo()));
+        try {
+            $db->commit();
+            $failed = false;
+        } catch (PDOException $e) {
+            $failed = true;
+        }
+        if (!$failed) {
+            printf("[002] Commit should have failed\n");
         }
 
         // pdo_transaction_transitions should check this as well...
diff --git a/ext/pdo_mysql/tests/pdo_mysql_inTransaction.phpt b/ext/pdo_mysql/tests/pdo_mysql_inTransaction.phpt
index 6d82e22b12..7ce93e8409 100644
--- a/ext/pdo_mysql/tests/pdo_mysql_inTransaction.phpt
+++ b/ext/pdo_mysql/tests/pdo_mysql_inTransaction.phpt
@@ -14,11 +14,11 @@ const BEGIN = ['BEGIN', 'START TRANSACTION'];
 const END = ['COMMIT', 'ROLLBACK'];
 
 $db = MySQLPDOTest::factory();
-// $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false); // mysql does not support
-for ($b = 0; $b < count(BEGIN); $b++) {
-    for ($e = 0; $e < count(END); $e++) {
+// $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false); // mysql does not support
+foreach (BEGIN as $begin) {
+    foreach (END as $end) {
         foreach (['exec', 'query', 'execute'] as $w) {
-            foreach ([BEGIN[$b], END[$e]] as $command) {
+            foreach ([$begin, $end] as $command) {
                 switch ($w) {
                     case 'exec':
                         $db->exec($command);
@@ -38,6 +38,37 @@ for ($b = 0; $b < count(BEGIN); $b++) {
         }
     }
 }
+echo "\n";
+
+// Mixing PDO transaction API and explicit queries.
+foreach (END as $end) {
+    $db->beginTransaction();
+    var_dump($db->inTransaction());
+    $db->exec($end);
+    var_dump($db->inTransaction());
+}
+
+$db->exec('START TRANSACTION');
+var_dump($db->inTransaction());
+$db->rollBack();
+var_dump($db->inTransaction());
+$db->exec('START TRANSACTION');
+var_dump($db->inTransaction());
+$db->commit();
+var_dump($db->inTransaction());
+echo "\n";
+
+// DDL query causes an implicit commit.
+$db->beginTransaction();
+var_dump($db->inTransaction());
+$db->exec('DROP TABLE IF EXISTS test');
+var_dump($db->inTransaction());
+
+// We should be able to start a new transaction after the implicit commit.
+$db->beginTransaction();
+var_dump($db->inTransaction());
+$db->commit();
+var_dump($db->inTransaction());
 
 ?>
 --EXPECT--
@@ -65,3 +96,17 @@ bool(true)
 bool(false)
 bool(true)
 bool(false)
+
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+
+bool(true)
+bool(false)
+bool(true)
+bool(false)
diff --git a/ext/pdo_mysql/tests/pdo_mysql_rollback.phpt b/ext/pdo_mysql/tests/pdo_mysql_rollback.phpt
index d27aefc6be..25a467d877 100644
--- a/ext/pdo_mysql/tests/pdo_mysql_rollback.phpt
+++ b/ext/pdo_mysql/tests/pdo_mysql_rollback.phpt
@@ -37,12 +37,15 @@ if (false == MySQLPDOTest::detect_transactional_mysql_engine($db))
     $db->query('DROP TABLE IF EXISTS test2');
     $db->query('CREATE TABLE test2(id INT)');
     $num++;
-    $db->rollBack();
-    $row = $db->query('SELECT COUNT(*) AS _num FROM test')->fetch(PDO::FETCH_ASSOC);
-    if ($row['_num'] != $num)
-        printf("[002] ROLLBACK should have no effect because of the implicit COMMIT
-            triggered by DROP/CREATE TABLE\n");
-
+    try {
+        $db->rollBack();
+        $failed = false;
+    } catch (PDOException $e) {
+        $failed = true;
+    }
+    if (!$failed) {
+        printf("[003] Rollback should have failed\n");
+    }
 
     $db->query('DROP TABLE IF EXISTS test2');
     $db->query('CREATE TABLE test2(id INT) ENGINE=MyISAM');