Issue #3440848 by mondrake, quietone, daffie: Ensure post transaction callbacks are only at the end of the root Drupal transaction

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

@@ -23,6 +23,26 @@ use Drupal\Core\Database\TransactionOutOfOrderException;
  */
 abstract class TransactionManagerBase implements TransactionManagerInterface {
 
+  /**
+   * The ID of the root Transaction object.
+   *
+   * The unique identifier of the first 'root' transaction object created, when
+   * the stack is empty.
+   *
+   * Normally, during the transaction stack lifecycle only one 'root'
+   * Transaction object is processed. Any post transaction callbacks are only
+   * processed during its destruction. However, there are cases when there
+   * could be multiple 'root' transaction objects in the stack. For example: a
+   * 'root' transaction object is opened, then a DDL statement is executed in a
+   * database that does not support transactional DDL, and because of that,
+   * another 'root' is opened before the original one is closed.
+   *
+   * Keeping track of the first 'root' created allows us to process the post
+   * transaction callbacks only during its destruction and not during
+   * destruction of another one.
+   */
+  private ?string $rootId = NULL;
+
   /**
    * The stack of Drupal transactions currently active.
    *
@@ -234,11 +254,19 @@ abstract class TransactionManagerBase implements TransactionManagerInterface {
       throw new TransactionNameNonUniqueException("A transaction named {$name} is already in use. Active stack: " . $this->dumpStackItemsAsString());
     }
 
+    // Define a unique ID for the transaction.
+    $id = uniqid('', TRUE);
+
     // Do the client-level processing.
     if ($this->stackDepth() === 0) {
       $this->beginClientTransaction();
       $type = StackItemType::Root;
       $this->setConnectionTransactionState(ClientConnectionTransactionState::Active);
+      // Only set ::rootId if there's not one set already, which may happen in
+      // case of broken transactions.
+      if ($this->rootId === NULL) {
+        $this->rootId = $id;
+      }
     }
     else {
       // If we're already in a Drupal transaction then we want to create a
@@ -248,9 +276,6 @@ abstract class TransactionManagerBase implements TransactionManagerInterface {
       $type = StackItemType::Savepoint;
     }
 
-    // Define an unique id for the transaction.
-    $id = uniqid('', TRUE);
-
     // Add an item on the stack, increasing its depth.
     $this->addStackItem($id, new StackItem($name, $type));
 
@@ -262,6 +287,18 @@ abstract class TransactionManagerBase implements TransactionManagerInterface {
    * {@inheritdoc}
    */
   public function unpile(string $name, string $id): void {
+    // If this is a 'root' transaction, and it is voided (that is, no longer in
+    // the stack), then the transaction on the database is no longer active. An
+    // action such as a rollback, or a DDL statement, was executed that
+    // terminated the database transaction. So, we can process the post
+    // transaction callbacks.
+    if (!isset($this->stack()[$id]) && isset($this->voidedItems[$id]) && $this->rootId === $id) {
+      $this->processPostTransactionCallbacks();
+      $this->rootId = NULL;
+      unset($this->voidedItems[$id]);
+      return;
+    }
+
     // If the $id does not correspond to the one in the stack for that $name,
     // we are facing an orphaned Transaction object (for example in case of a
     // DDL statement breaking an active transaction). That should be listed in
@@ -289,6 +326,10 @@ abstract class TransactionManagerBase implements TransactionManagerInterface {
         // If this was the root Drupal transaction, we can commit the client
         // transaction.
         $this->processRootCommit();
+        if ($this->rootId === $id) {
+          $this->processPostTransactionCallbacks();
+          $this->rootId = NULL;
+        }
       }
       else {
         // The stack got corrupted.
@@ -405,7 +446,6 @@ abstract class TransactionManagerBase implements TransactionManagerInterface {
    * Processes the root transaction rollback.
    */
   protected function processRootRollback(): void {
-    $this->processPostTransactionCallbacks();
     $this->rollbackClientTransaction();
   }
 
@@ -417,7 +457,6 @@ abstract class TransactionManagerBase implements TransactionManagerInterface {
    */
   protected function processRootCommit(): void {
     $clientCommit = $this->commitClientTransaction();
-    $this->processPostTransactionCallbacks();
     if (!$clientCommit) {
       throw new TransactionCommitFailedException();
     }
@@ -519,7 +558,6 @@ abstract class TransactionManagerBase implements TransactionManagerInterface {
       $this->voidStackItem((string) $i);
     }
     $this->setConnectionTransactionState(ClientConnectionTransactionState::Voided);
-    $this->processPostTransactionCallbacks();
   }
 
 }

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

@@ -776,18 +776,60 @@ class DriverSpecificTransactionTestBase extends DriverSpecificDatabaseTestBase {
   /**
    * Tests post-transaction callback executes after transaction rollback.
    */
-  public function testRootTransactionEndCallbackCalledOnRollback(): void {
+  public function testRootTransactionEndCallbackCalledAfterRollbackAndDestruction(): void {
     $transaction = $this->createRootTransaction('', FALSE);
     $this->connection->transactionManager()->addPostTransactionCallback([$this, 'rootTransactionCallback']);
     $this->insertRow('row');
     $this->assertNull($this->postTransactionCallbackAction);
+
+    // Callbacks are processed only when destructing the transaction.
+    // Executing a rollback is not sufficient by itself.
     $transaction->rollBack();
-    $this->assertSame('rtcRollback', $this->postTransactionCallbackAction);
-    unset($transaction);
-    $this->assertRowAbsent('row');
-    // The row insert should be missing since the client rollback occurs after
-    // the processing of the callbacks.
+    $this->assertNull($this->postTransactionCallbackAction);
+    $this->assertRowAbsent('rtcCommit');
     $this->assertRowAbsent('rtcRollback');
+    $this->assertRowAbsent('row');
+
+    // Destruct the transaction.
+    unset($transaction);
+
+    // The post-transaction callback should now have inserted a 'rtcRollback'
+    // row.
+    $this->assertSame('rtcRollback', $this->postTransactionCallbackAction);
+    $this->assertRowAbsent('rtcCommit');
+    $this->assertRowPresent('rtcRollback');
+    $this->assertRowAbsent('row');
+  }
+
+  /**
+   * Tests post-transaction callback executes after a DDL statement.
+   */
+  public function testRootTransactionEndCallbackCalledAfterDdlAndDestruction(): void {
+    $transaction = $this->createRootTransaction('', FALSE);
+    $this->connection->transactionManager()->addPostTransactionCallback([$this, 'rootTransactionCallback']);
+    $this->insertRow('row');
+    $this->assertNull($this->postTransactionCallbackAction);
+
+    // Callbacks are processed only when destructing the transaction.
+    // Executing a DDL statement is not sufficient itself.
+    // We cannot use truncate here, since it has protective code to fall back
+    // to a transactional delete when in transaction. We drop an unrelated
+    // table instead.
+    $this->connection->schema()->dropTable('test_people');
+    $this->assertNull($this->postTransactionCallbackAction);
+    $this->assertRowAbsent('rtcCommit');
+    $this->assertRowAbsent('rtcRollback');
+    $this->assertRowPresent('row');
+
+    // Destruct the transaction.
+    unset($transaction);
+
+    // The post-transaction callback should now have inserted a 'rtcCommit'
+    // row.
+    $this->assertSame('rtcCommit', $this->postTransactionCallbackAction);
+    $this->assertRowPresent('rtcCommit');
+    $this->assertRowAbsent('rtcRollback');
+    $this->assertRowPresent('row');
   }
 
   /**