MySQL on PHP 8 now errors when committing or rolling back when there is no active transaction

This is turns out to be fixed in PHP 8.0 and results in problems for Drupal 8 on PHP 8 with MySQL. There are code paths that only work when committing when a transaction does not exist because MySQL does not report an error. For example: \Drupal\Core\Routing\MatcherDumper::dump() - if the router table does not exist we'll create the table but on MySQL DDL statements will cause the transactions to be committed.

Here's the patch from #3156595: Make Drupal 9 installable on PHP8

Targeting 9.1.x as that is the release we want to be PHP 8 compatible.

Trying to update the issue summary - in doing that I have a concern with solution. Given that when rolling back a transaction in core we're doing something like

    catch (\Exception $e) {
      $transaction->rollBack();
      watchdog_exception('workspaces', $e);
      throw $e;
    }

the current solution of actually throwing the exception on PHP 8 when there is no active transaction means that we'll never hit the lines:

      watchdog_exception('workspaces', $e);
      throw $e;

Therefore on PHP 8 rather than being more accurate in the exception we report we are going to lose information. I think the correct thing here is to make PHP 7 and 8 behave the same and eat the exception in PHP 8 so we can report the real reason for the rollback which is the earlier exception. I think we need to issue a warning telling the user that due to MySQL not having transactional DDL the database might be in an inconsistent state because we are unable to rollback the transaction.

Updating patch to work in way that is consistent for PHP 7 and PHP 8 but has the added benefit on PHP 8 that you get a warning that something v bad has happened.

Is the message $e->getMessage() === 'There is no active transaction' reliable? It's coming from https://github.com/php/php-src/blob/bd555b6ccaa9f5e24e2b071067ed1ed51fe6... but I do wonder what's the policy on the stability of these messages. Perhaps in light of the recent commit our issue attacks we could ask PHP internals to please change the code from a fixed zero to something and add a PDO constant for it so it is reliable? Until then perhaps move this check into a method because it occurs twice.

Wouldn't it be better to rely on \PDO::inTransaction instead of catching an exception to determine if we are in a transaction currently, and throw DatabaseExceptionWrapper if not (or, TransactionNoActiveException)? Then yes, we can still catch the failure and rethrow, as last resort.

@mondrake looking at php-src it sounds much cleaner - cost of one function call but pdo already doing it later https://github.com/php/php-src/blob/bd555b6ccaa9f5e24e2b071067ed1ed51fe6...

We cannot re-throw the exception. That will break Drupal. We rely on the PHP 7 of not failing when calling commit. And if we want decent errors when rolling back we need to ignore it too.

If we call Pdo::inTransaction() then we'll take an extra trip to the server - see https://github.com/php/php-src/blob/master/ext/pdo_mysql/mysql_driver.c#... - I guess this is not so bad given this will not be the normal code path. It only happens when there are DDL statements during a transaction which only occurs when creating things like cache tables or the router.

I just tried to implement this. It's not so pretty because we need to copy code from \Drupal\Core\Database\Connection::rollBack to \Drupal\Core\Database\Driver\mysql\Connection::rollBack() but I think it's okay.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -417,6 +418,56 @@ protected function popCommittableTransactions() {
+    if (!$this->connection->inTransaction()) {
+      // On PHP 7 $this->connection->rollback() does not throw an exception if
+      // there is no active transaction. In order to maintain consistent
+      // behavior on PHP 8 we have to ignore this error. This situation occurs
+      // when tables are altered or created (DDL transactions are not
+      // supported).
+      trigger_error('Rollback attempted when there is no active transaction. This can cause data integrity issues.', E_USER_WARNING);
+      return;
+    }

Sadly PHP 7 actually returns TRUE for $this->connection->inTransaction() after a DDL statement has caused an automatic commit. :( I was hoping that we'd get the warning on PHP 7 too...

If I understand correctly, the above won't work? Then another thought: the DDL statements are ours, the transaction code is ours. What if we explicitly set a flag "DDL was ran in this transaction"? We could add a query, a queryRange and an update method to Drupal\Core\Database\Driver\mysql\Schema and these would flag the transaction before calling the relevant method on the connection.

@Charlie ChX Negyesi what do you mean by won't work precisely? #10 works fine on PHP 7 and PHP 8 - see #3156595-128: Make Drupal 9 installable on PHP8. What #11 is pointing out is that the new code unfortunately does not improve the error reporting in PHP 7. Yes we could add more state to the MySQL driver but I think the solution in #10 is okay and probably preferable. If you call rollback on a transaction in MySQL where a DDL statement has occurred now on PHP 8 you get a warning that things are properly broken - which is an improvement from HEAD/PHP 7. And there's no way we can do the rollback (because MySQL just does not support it).

I misunderstood #11 then, I thought you are saying the patch in #10 didn't work out and I was trying to come up with something bettter but if it works, that's just great.

1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -417,6 +418,56 @@ protected function popCommittableTransactions() {
   +
   +    if (!$this->connection->inTransaction()) {
   +      // On PHP 7 $this->connection->rollback() does not throw an exception if
   +      // there is no active transaction. In order to maintain consistent
   +      // behavior on PHP 8 we have to ignore this error. This situation occurs
   +      // when tables are altered or created (DDL transactions are not
   +      // supported).
   +      trigger_error('Rollback attempted when there is no active transaction. This can cause data integrity issues.', E_USER_WARNING);
   +      return;
   

   We should add a comment here that inTransaction() will return TRUE on PHP 7 and we'll never reach the trigger_error() there. I think it's fine that it doesn't - it's just the status quo which we won't support in Drupal 10 anyway.

2. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -417,6 +418,56 @@ protected function popCommittableTransactions() {
   +      // committed. This situation occurs when tables are altered or created
   +      // (DDL transactions are not supported).
   +      $success = TRUE;
   +      if (!empty($this->rootTransactionEndCallbacks)) {
   +        $callbacks = $this->rootTransactionEndCallbacks;
   +        $this->rootTransactionEndCallbacks = [];
   +        foreach ($callbacks as $callback) {
   +          call_user_func($callback, $success);
   +        }
   +      }
   

   If this is copied, can we point to the method where it's copied from explicitly?

Addressing #15. Note we will never be able to throw the exception from \Drupal\Core\Database\Driver\mysql\Connection::rollBack() because we want the real reason to be reported.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -431,12 +431,16 @@ public function rollBack($savepoint_name = 'drupal_transaction') {
 
+    // MySQL will automatically commit transactions when tables are altered or
+    // created (DDL transactions are not supported). On PHP 7
+    // $this->connection->rollback() does not throw an exception if there is no
+    // active transaction. This behavior is fixed in PHP 8. In order to maintain
+    // consistent behavior regardless of PHP version, prevent triggering an
+    // exception on PHP 8. This ensures that the error that has caused the
+    // rollback is properly reported.

Only thing I'm wondering here is if we could structure the comment as if we already require PHP 8 (i.e. the future reality), then in a separate paragraph discuss the PHP 7 situation. This way when we do drop support for PHP 7 we should be able to just delete lines rather than restructure. It might also make it easier to follow.

Good point. I think @catch is correct that this does make it easier to follow.

That's much easier to read, which I didn't really expect, but nice side effect. I think this is ready to go.

+++ b/core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php
@@ -258,18 +258,17 @@ public function testTransactionWithDdlStatement() {
+      // The PHP 7 MySQL driver does not trigger an exception when calling
+      // rollback and there is no active transaction.
+      if (PHP_VERSION_ID >= 80000) {
+        $this->expectWarning();
+        $this->expectWarningMessage('Rollback attempted when there is no active transaction. This can cause data integrity issues.');
       }
+
+      // Rollback the outer transaction.
+      $transaction->rollBack();
+      unset($transaction);
       $this->assertRowPresent('row');

Doh I've fallen into the expect exception trap... on PHP 8 the $this->assertRowPresent('row'); is never called because we bail out of the test after the warning...

The changes attached ensure that the test fails on PHP 7 and PHP 8 when you change $this->assertRowPresent('row'); to $this->assertRowPresent('BLAH');

The fail message is incorrect. It should produce an exception - if it does then everything is broken for us. But on PHP 8 it should now produce a warning.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -417,6 +418,61 @@ protected function popCommittableTransactions() {
+    if (!$this->inTransaction()) {
+      throw new TransactionNoActiveException();
+    }
+    // A previous rollback to an earlier savepoint may mean that the savepoint
+    // in question has already been accidentally committed.
+    if (!isset($this->transactionLayers[$savepoint_name])) {
+      throw new TransactionNoActiveException();
+    }
+

Pity that this will be executed twice at every rollback on MySql, here and again once calling parent::rollBack. Is it absolutely necessary to have it here?

@mondrake yes it is. We need to throw those exceptions on PHP 8... but we can do a bit better because we only need to do this if we're not calling the parent.

Assuming it turns out green, this looks neat and ready to me.

#3156595-130: Make Drupal 9 installable on PHP8 is testing this patch against PHP 8.0 and it is green too.

Asked @alexpott about a test-only patch, if we remove the fix here, Drupal isn't installable at all, so the test run blows up - looks like this: https://dispatcher.drupalci.org/job/drupal_patches/65266/

So the test can pass, but it's hard to demonstrate that it will fail in isolation. However thousands of fatal errors is a good clue that something's wrong so that's fine.

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!