[10.x] Fix "after commit" callbacks not running on nested transactions

composer.json

@@ -104,7 +104,7 @@
         "league/flysystem-read-only": "^3.3",
         "league/flysystem-sftp-v3": "^3.0",
         "mockery/mockery": "^1.5.1",
-        "orchestra/testbench-core": "^8.10",
+        "orchestra/testbench-core": "^8.11.3",
         "pda/pheanstalk": "^4.0",
         "phpstan/phpstan": "^1.4.7",
         "phpunit/phpunit": "^10.0.7",

src/Illuminate/Database/Concerns/ManagesTransactions.php

@@ -47,11 +47,9 @@ public function transaction(Closure $callback, $attempts = 1)
                     $this->getPdo()->commit();
                 }
 
-                $this->transactions = max(0, $this->transactions - 1);
+                $this->transactionsManager?->commit($this->getName(), $this->transactions);
 
-                if ($this->afterCommitCallbacksShouldBeExecuted()) {
-                    $this->transactionsManager?->commit($this->getName());
-                }
+                $this->transactions = max(0, $this->transactions - 1);
             } catch (Throwable $e) {
                 $this->handleCommitTransactionException(
                     $e, $currentAttempt, $attempts
@@ -194,27 +192,13 @@ public function commit()
             $this->getPdo()->commit();
         }
 
-        $this->transactions = max(0, $this->transactions - 1);
+        $this->transactionsManager?->commit($this->getName(), $this->transactions);
 
-        if ($this->afterCommitCallbacksShouldBeExecuted()) {
-            $this->transactionsManager?->commit($this->getName());
-        }
+        $this->transactions = max(0, $this->transactions - 1);
 
         $this->fireConnectionEvent('committed');
     }
 
-    /**
-     * Determine if after commit callbacks should be executed.
-     *
-     * @return bool
-     */
-    protected function afterCommitCallbacksShouldBeExecuted()
-    {
-        return $this->transactions == 0 ||
-            ($this->transactionsManager &&
-             $this->transactionsManager->callbackApplicableTransactions()->count() === 1);
-    }
-
     /**
      * Handle an exception encountered when committing a transaction.
      *

src/Illuminate/Database/DatabaseTransactionsManager.php

@@ -12,11 +12,11 @@ class DatabaseTransactionsManager
     protected $transactions;
 
     /**
-     * The database transaction that should be ignored by callbacks.
+     * When in test mode, we'll run the after commit callbacks on the top-level transaction.
      *
-     * @var \Illuminate\Database\DatabaseTransactionRecord
+     * @var bool
      */
-    protected $callbacksShouldIgnore;
+    protected $afterCommitCallbacksRunningInTestTransaction = false;
 
     /**
      * Create a new database transactions manager instance.
@@ -26,6 +26,19 @@ class DatabaseTransactionsManager
     public function __construct()
     {
         $this->transactions = collect();
+        $this->afterCommitCallbacksRunningInTestTransaction = false;
+    }
+
+    /**
+     * Sets the transaction manager to test mode.
+     *
+     * @return self
+     */
+    public function withAfterCommitCallbacksInTestTransactionAwareMode()
+    {
+        $this->afterCommitCallbacksRunningInTestTransaction = true;
+
+        return $this;
     }
 
     /**
@@ -54,30 +67,28 @@ public function rollback($connection, $level)
         $this->transactions = $this->transactions->reject(
             fn ($transaction) => $transaction->connection == $connection && $transaction->level > $level
         )->values();
-
-        if ($this->transactions->isEmpty()) {
-            $this->callbacksShouldIgnore = null;
-        }
     }
 
     /**
      * Commit the active database transaction.
      *
      * @param  string  $connection
+     * @param  int  $level
      * @return void
      */
-    public function commit($connection)
+    public function commit($connection, $level = 1)
     {
-        [$forThisConnection, $forOtherConnections] = $this->transactions->partition(
-            fn ($transaction) => $transaction->connection == $connection
-        );
-
-        $this->transactions = $forOtherConnections->values();
+        // If the transaction level being commited reaches 1 (meaning it was the root
+        // transaction), we'll run the callbacks. In test mode, since we wrap each
+        // test in a transaction, we'll run the callbacks when reaching level 2.
+        if ($level == 1 || ($this->afterCommitCallbacksRunningInTestTransaction && $level == 2)) {
+            [$forThisConnection, $forOtherConnections] = $this->transactions->partition(
+                fn ($transaction) => $transaction->connection == $connection
+            );
 
-        $forThisConnection->map->executeCallbacks();
+            $this->transactions = $forOtherConnections->values();
 
-        if ($this->transactions->isEmpty()) {
-            $this->callbacksShouldIgnore = null;
+            $forThisConnection->map->executeCallbacks();
         }
     }
 
@@ -89,36 +100,42 @@ public function commit($connection)
      */
     public function addCallback($callback)
     {
-        if ($current = $this->callbackApplicableTransactions()->last()) {
-            return $current->addCallback($callback);
+        // If there are no transactions, we'll run the callbacks right away. Also, we'll run it
+        // right away when we're in test mode and we only have the wrapping transaction. For
+        // every other case, we'll queue up the callback to run after the commit happens.
+        if ($this->transactions->isEmpty() || ($this->afterCommitCallbacksRunningInTestTransaction && $this->transactions->count() == 1)) {
+            return $callback();
         }
 
-        $callback();
+        return $this->transactions->last()->addCallback($callback);
     }
 
     /**
      * Specify that callbacks should ignore the given transaction when determining if they should be executed.
      *
      * @param  \Illuminate\Database\DatabaseTransactionRecord  $transaction
      * @return $this
+     *
+     * @deprecated Will be removed in a future Laravel version. Use withAfterCommitCallbacksInTestTransactionAwareMode() instead.
      */
     public function callbacksShouldIgnore(DatabaseTransactionRecord $transaction)
     {
-        $this->callbacksShouldIgnore = $transaction;
-
-        return $this;
+        // This method was meant for testing only, so we're forwarding the call to the new method...
+        return $this->withAfterCommitCallbacksInTestTransactionAwareMode();
     }
 
     /**
      * Get the transactions that are applicable to callbacks.
      *
      * @return \Illuminate\Support\Collection
+     *
+     * @deprecated Will be removed in a future Laravel version.
      */
     public function callbackApplicableTransactions()
     {
-        return $this->transactions->reject(function ($transaction) {
-            return $transaction === $this->callbacksShouldIgnore;
-        })->values();
+        return $this->transactions
+            ->when($this->afterCommitCallbacksRunningInTestTransaction, fn ($transactions) => $transactions->skip(1))
+            ->values();
     }
 
     /**

src/Illuminate/Foundation/Testing/DatabaseTransactions.php

@@ -22,9 +22,7 @@ public function beginDatabaseTransaction()
             $connection->setEventDispatcher($dispatcher);
 
             if ($this->app->resolved('db.transactions')) {
-                $this->app->make('db.transactions')->callbacksShouldIgnore(
-                    $this->app->make('db.transactions')->getTransactions()->first()
-                );
+                $this->app->make('db.transactions')->withAfterCommitCallbacksInTestTransactionAwareMode();
             }
         }
 

src/Illuminate/Foundation/Testing/RefreshDatabase.php

@@ -98,9 +98,7 @@ public function beginDatabaseTransaction()
             $connection->setEventDispatcher($dispatcher);
 
             if ($this->app->resolved('db.transactions')) {
-                $this->app->make('db.transactions')->callbacksShouldIgnore(
-                    $this->app->make('db.transactions')->getTransactions()->first()
-                );
+                $this->app->make('db.transactions')->withAfterCommitCallbacksInTestTransactionAwareMode();
             }
         }
 

tests/Database/DatabaseTransactionsManagerTest.php

@@ -67,7 +67,7 @@ public function testCommittingTransactions()
         $manager->begin('default', 2);
         $manager->begin('admin', 1);
 
-        $manager->commit('default');
+        $manager->commit('default', 1);
 
         $this->assertCount(1, $manager->getTransactions());
 
@@ -118,7 +118,11 @@ public function testCommittingTransactionsExecutesCallbacks()
 
         $manager->begin('admin', 1);
 
-        $manager->commit('default');
+        $manager->commit('default', 2);
+
+        $this->assertEmpty($callbacks, 'Should not have ran the callbacks.');
+
+        $manager->commit('default', 1);
 
         $this->assertCount(2, $callbacks);
         $this->assertEquals(['default', 1], $callbacks[0]);
@@ -144,7 +148,11 @@ public function testCommittingExecutesOnlyCallbacksOfTheConnection()
             $callbacks[] = ['admin', 1];
         });
 
-        $manager->commit('default');
+        $manager->commit('default', 2);
+
+        $this->assertEmpty($callbacks, 'Should not have run the callbacks.');
+
+        $manager->commit('default', 1);
 
         $this->assertCount(1, $callbacks);
         $this->assertEquals(['default', 1], $callbacks[0]);

tests/Database/DatabaseTransactionsTest.php

@@ -64,7 +64,7 @@ public function testTransactionIsRecordedAndCommitted()
     {
         $transactionManager = m::mock(new DatabaseTransactionsManager);
         $transactionManager->shouldReceive('begin')->once()->with('default', 1);
-        $transactionManager->shouldReceive('commit')->once()->with('default');
+        $transactionManager->shouldReceive('commit')->once()->with('default', 1);
 
         $this->connection()->setTransactionManager($transactionManager);
 
@@ -83,7 +83,7 @@ public function testTransactionIsRecordedAndCommittedUsingTheSeparateMethods()
     {
         $transactionManager = m::mock(new DatabaseTransactionsManager);
         $transactionManager->shouldReceive('begin')->once()->with('default', 1);
-        $transactionManager->shouldReceive('commit')->once()->with('default');
+        $transactionManager->shouldReceive('commit')->once()->with('default', 1);
 
         $this->connection()->setTransactionManager($transactionManager);
 
@@ -103,7 +103,8 @@ public function testNestedTransactionIsRecordedAndCommitted()
         $transactionManager = m::mock(new DatabaseTransactionsManager);
         $transactionManager->shouldReceive('begin')->once()->with('default', 1);
         $transactionManager->shouldReceive('begin')->once()->with('default', 2);
-        $transactionManager->shouldReceive('commit')->once()->with('default');
+        $transactionManager->shouldReceive('commit')->once()->with('default', 2);
+        $transactionManager->shouldReceive('commit')->once()->with('default', 1);
 
         $this->connection()->setTransactionManager($transactionManager);
 
@@ -130,8 +131,9 @@ public function testNestedTransactionIsRecordeForDifferentConnectionsdAndCommitt
         $transactionManager->shouldReceive('begin')->once()->with('default', 1);
         $transactionManager->shouldReceive('begin')->once()->with('second_connection', 1);
         $transactionManager->shouldReceive('begin')->once()->with('second_connection', 2);
-        $transactionManager->shouldReceive('commit')->once()->with('default');
-        $transactionManager->shouldReceive('commit')->once()->with('second_connection');
+        $transactionManager->shouldReceive('commit')->once()->with('second_connection', 2);
+        $transactionManager->shouldReceive('commit')->once()->with('second_connection', 1);
+        $transactionManager->shouldReceive('commit')->once()->with('default', 1);
 
         $this->connection()->setTransactionManager($transactionManager);
         $this->connection('second_connection')->setTransactionManager($transactionManager);

tests/Integration/Database/EloquentTransactionUsingRefreshDatabaseTest.php

@@ -0,0 +1,101 @@
+<?php
+
+namespace Illuminate\Tests\Integration\Database;
+
+use Illuminate\Foundation\Auth\User;
+use Illuminate\Foundation\Testing\RefreshDatabase;
+use Illuminate\Support\Facades\DB;
+use Orchestra\Testbench\Concerns\WithLaravelMigrations;
+use Orchestra\Testbench\Factories\UserFactory;
+
+class EloquentTransactionUsingRefreshDatabaseTest extends DatabaseTestCase
+{
+    use RefreshDatabase, WithLaravelMigrations;
+
+    protected function setUp(): void
+    {
+        $this->afterApplicationCreated(fn () => User::unguard());
+        $this->beforeApplicationDestroyed(fn () => User::reguard());
+
+        parent::setUp();
+    }
+
+    public function testObserverIsCalledOnTestsWithAfterCommit()
+    {
+        User::observe($observer = EloquentTransactionUsingRefreshDatabaseUserObserver::resetting());
+
+        $user1 = User::create(UserFactory::new()->raw());
+
+        $this->assertTrue($user1->exists);
+        $this->assertEquals(1, $observer::$calledTimes, 'Failed to assert the observer was called once.');
+    }
+
+    public function testObserverCalledWithAfterCommitWhenInsideTransaction()
+    {
+        User::observe($observer = EloquentTransactionUsingRefreshDatabaseUserObserver::resetting());
+
+        $user1 = DB::transaction(fn () => User::create(UserFactory::new()->raw()));
+
+        $this->assertTrue($user1->exists);
+        $this->assertEquals(1, $observer::$calledTimes, 'Failed to assert the observer was called once.');
+    }
+
+    public function testObserverIsCalledOnTestsWithAfterCommitWhenUsingSavepoint()
+    {
+        User::observe($observer = EloquentTransactionUsingRefreshDatabaseUserObserver::resetting());
+
+        $user1 = User::createOrFirst(UserFactory::new()->raw());
+
+        $this->assertTrue($user1->exists);
+        $this->assertEquals(1, $observer::$calledTimes, 'Failed to assert the observer was called once.');
+    }
+
+    public function testObserverIsCalledOnTestsWithAfterCommitWhenUsingSavepointAndInsideTransaction()
+    {
+        User::observe($observer = EloquentTransactionUsingRefreshDatabaseUserObserver::resetting());
+
+        $user1 = DB::transaction(fn () => User::createOrFirst(UserFactory::new()->raw()));
+
+        $this->assertTrue($user1->exists);
+        $this->assertEquals(1, $observer::$calledTimes, 'Failed to assert the observer was called once.');
+    }
+
+    public function testObserverIsCalledEvenWhenDeeplyNestingTransactions()
+    {
+        User::observe($observer = EloquentTransactionUsingRefreshDatabaseUserObserver::resetting());
+
+        $user1 = DB::transaction(function () use ($observer) {
+            return tap(DB::transaction(function () use ($observer) {
+                return tap(DB::transaction(function () {
+                    return User::createOrFirst(UserFactory::new()->raw());
+                }), function () use ($observer) {
+                    $this->assertEquals(0, $observer::$calledTimes, 'Should not have been called');
+                });
+            }), function () use ($observer) {
+                $this->assertEquals(0, $observer::$calledTimes, 'Should not have been called');
+            });
+        });
+
+        $this->assertTrue($user1->exists);
+        $this->assertEquals(1, $observer::$calledTimes, 'Failed to assert the observer was called once.');
+    }
+}
+
+class EloquentTransactionUsingRefreshDatabaseUserObserver
+{
+    public static $calledTimes = 0;
+
+    public $afterCommit = true;
+
+    public static function resetting()
+    {
+        static::$calledTimes = 0;
+
+        return new static();
+    }
+
+    public function created($user)
+    {
+        static::$calledTimes++;
+    }
+}