Issue #3405976 by alexpott, jrglasgow, fago, catch, mondrake, solideogloria,...

Issue #3405976 by alexpott, jrglasgow, fago, catch, mondrake, solideogloria, mglaman, Driskell, derickr, longwave, Mile23, YesCT, daffie: Transaction autocommit during shutdown relies on unreliable object destruction order (xdebug 3.3+ enabled)

(cherry picked from commit 26308296)

core/lib/Drupal/Core/Database/Connection.php

@@ -292,6 +292,31 @@ public function __destruct() {
     $this->connection = NULL;
   }
 
+  /**
+   * Commits all the open transactions.
+   *
+   * @internal
+   *   This method exists only to work around a bug caused by Drupal incorrectly
+   *   relying on object destruction order to commit transactions. Xdebug 3.3.0
+   *   changes the order of object destruction when the develop mode is enabled.
+   */
+  public function commitAll() {
+    $manager = $this->transactionManager();
+    if ($manager && $manager->inTransaction() && method_exists($manager, 'commitAll')) {
+      $this->transactionManager()->commitAll();
+    }
+
+    // BC layer.
+    // @phpstan-ignore-next-line
+    if (!empty($this->transactionLayers)) {
+      // Make all transactions committable.
+      // @phpstan-ignore-next-line
+      $this->transactionLayers = array_fill_keys(array_keys($this->transactionLayers), FALSE);
+      // @phpstan-ignore-next-line
+      $this->popCommittableTransactions();
+    }
+  }
+
   /**
    * Returns the client-level database connection object.
    *

core/lib/Drupal/Core/Database/Database.php

@@ -486,10 +486,18 @@ public static function closeConnection($target = NULL, $key = NULL) {
     if (!isset($key)) {
       $key = self::$activeKey;
     }
-    if (isset($target)) {
+    if (isset($target) && isset(self::$connections[$key][$target])) {
+      if (self::$connections[$key][$target] instanceof Connection) {
+        self::$connections[$key][$target]->commitAll();
+      }
       unset(self::$connections[$key][$target]);
     }
-    else {
+    elseif (isset(self::$connections[$key])) {
+      foreach (self::$connections[$key] as $connection) {
+        if ($connection instanceof Connection) {
+          $connection->commitAll();
+        }
+      }
       unset(self::$connections[$key]);
     }
     // Force garbage collection to run. This ensures that client connection
@@ -740,4 +748,45 @@ private static function isWithinModuleNamespace(string $namespace) {
     return ($first === 'Drupal' && strtolower($second) === $second);
   }
 
+  /**
+   * Calls commitAll() on all the open connections.
+   *
+   * If drupal_register_shutdown_function() exists the commit will occur during
+   * shutdown so that it occurs at the latest possible moment.
+   *
+   * @param bool $shutdown
+   *   Internal param to denote that the method is being called by
+   *   _drupal_shutdown_function().
+   *
+   * @return void
+   *
+   * @internal
+   *   This method exists only to work around a bug caused by Drupal incorrectly
+   *   relying on object destruction order to commit transactions. Xdebug 3.3.0
+   *   changes the order of object destruction when the develop mode is enabled.
+   */
+  public static function commitAllOnShutdown(bool $shutdown = FALSE): void {
+    static $registered = FALSE;
+
+    if ($shutdown) {
+      foreach (self::$connections as $targets) {
+        foreach ($targets as $connection) {
+          if ($connection instanceof Connection) {
+            $connection->commitAll();
+          }
+        }
+      }
+      return;
+    }
+
+    if (!function_exists('drupal_register_shutdown_function')) {
+      return;
+    }
+
+    if (!$registered) {
+      $registered = TRUE;
+      drupal_register_shutdown_function('\Drupal\Core\Database\Database::commitAllOnShutdown', TRUE);
+    }
+  }
+
 }

core/lib/Drupal/Core/Database/Transaction.php

@@ -57,6 +57,11 @@ public function __construct(
     $name = NULL,
     protected readonly string $id = '',
   ) {
+    // Transactions rely on objects being destroyed in order to be committed.
+    // PHP makes no guarantee about the order in which objects are destroyed so
+    // ensure all transactions are committed on shutdown.
+    Database::commitAllOnShutdown();
+
     if ($connection->transactionManager()) {
       $this->connection = $connection;
       $this->name = $name;

core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php

@@ -127,6 +127,20 @@ protected function stack(): array {
     return $this->stack;
   }
 
+  /**
+   * Commits the entire transaction stack.
+   *
+   * @internal
+   *   This method exists only to work around a bug caused by Drupal incorrectly
+   *   relying on object destruction order to commit transactions. Xdebug 3.3.0
+   *   changes the order of object destruction when the develop mode is enabled.
+   */
+  public function commitAll(): void {
+    foreach (array_reverse($this->stack()) as $id => $item) {
+      $this->unpile($item->name, $id);
+    }
+  }
+
   /**
    * Adds an item to the transaction stack.
    *
@@ -253,7 +267,6 @@ public function unpile(string $name, string $id): void {
     // DDL statement breaking an active transaction). That should be listed in
     // $voidedItems, so we can remove it from there.
     if (!isset($this->stack()[$id]) || $this->stack()[$id]->name !== $name) {
-      assert(isset($this->voidedItems[$id]), "Transaction {$id}/{$name} is out of sequence. Active stack: " . $this->dumpStackItemsAsString());
       unset($this->voidedItems[$id]);
       return;
     }

core/tests/Drupal/KernelTests/Core/Database/DriverSpecificTransactionTestBase.php

@@ -4,8 +4,10 @@
 
 use Drupal\Core\Database\Database;
 use Drupal\Core\Database\Transaction;
+use Drupal\Core\Database\Transaction\ClientConnectionTransactionState;
 use Drupal\Core\Database\Transaction\StackItem;
 use Drupal\Core\Database\Transaction\StackItemType;
+use Drupal\Core\Database\Transaction\TransactionManagerBase;
 use Drupal\Core\Database\TransactionExplicitCommitNotAllowedException;
 use Drupal\Core\Database\TransactionNameNonUniqueException;
 use Drupal\Core\Database\TransactionOutOfOrderException;
@@ -807,11 +809,24 @@ public function testTransactionManagerFailureOnPendingStackItems(): void {
     $testConnection = Database::getConnection('test_fail');
 
     // Add a fake item to the stack.
-    $reflectionMethod = new \ReflectionMethod(get_class($testConnection->transactionManager()), 'addStackItem');
-    $reflectionMethod->invoke($testConnection->transactionManager(), 'bar', new StackItem('qux', StackItemType::Savepoint));
+    $manager = $testConnection->transactionManager();
+    $reflectionMethod = new \ReflectionMethod($manager, 'addStackItem');
+    $reflectionMethod->invoke($manager, 'bar', new StackItem('qux', StackItemType::Root));
+    // Ensure transaction state can be determined during object destruction.
+    // This is necessary for the test to pass when xdebug.mode has the 'develop'
+    // option enabled.
+    $reflectionProperty = new \ReflectionProperty(TransactionManagerBase::class, 'connectionTransactionState');
+    $reflectionProperty->setValue($manager, ClientConnectionTransactionState::Active);
 
     $this->expectException(\AssertionError::class);
     $this->expectExceptionMessageMatches("/^Transaction .stack was not empty\\. Active stack: bar\\\\qux/");
+    // Ensure that __destruct() results in an assertion error. Note that this
+    // will normally be called by PHP during the object's destruction but Drupal
+    // will commit all transactions when a database is closed thereby making
+    // this impossible to test unless it is called directly.
+    $manager->__destruct();
+
+    // Clean up.
     unset($testConnection);
     Database::closeConnection('test_fail');
   }
@@ -854,6 +869,10 @@ public function testConnectionDeprecations(): void {
     $this->expectDeprecation('Drupal\\Core\\Database\\Connection::popCommittableTransactions() is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Use TransactionManagerInterface methods instead. See https://www.drupal.org/node/3381002');
     $this->expectDeprecation('Drupal\\Core\\Database\\Connection::doCommit() is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Use TransactionManagerInterface methods instead. See https://www.drupal.org/node/3381002');
     $this->connection->popTransaction('foo');
+
+    // Ensure there are no outstanding transactions left. This is necessary for
+    // the test to pass when xdebug.mode has the 'develop' option enabled.
+    $this->connection->commitAll();
   }
 
 }