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

…s using `RefreshDatabase` or `DatabaseMigrations` (#48523) * Tests observer using afterCommit * Adds failing test for savepoint and observers using afterCommit * Ignore the createOrFirst savepoint to run callbacks * Fix typo * Remove DatabaseEloquentAppTest to see if CI passes * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * Update src/Illuminate/Database/DatabaseTransactionsManager.php Co-authored-by: Mior Muhammad Zaki <crynobone@gmail.com> * Changes the way the after commit callbacks are executed to avoid remembering which transactions to ignore Before, we were remembering the test transaction so we could ignore it when deciding to run the after commit callbacks or not. We're still handling the after commit callbacks like that but now instead of remembering which transactions to ignore, we're always calling the DatabaseTransactionManager::commit method. The difference is that now we're passing the current transaction level to it. The method will decide to call the callbacks or not based on that level and whether or not this is on in test mode. When in tests, instead of setting the current transaction to be remembered so it could be ignored, we're now only setting the DatabaseTransactionManager to test mode. When in test mode, it will execute the callbacks when the transactions count reaches 1 (remember that the test runs in a transaction, so that's the "root" level). Otherwise, it runs the callbacks when the transactions level is on level 0 (like in production). There's also a change in the DatabaseTransactionManager::addCallback method. It now also checks if it's in test mode. When not in test mode, it only adds the callback to the execution queue if there's an open transaction. Otherwise, the callback is executed right away. When in test mode, the number of transactions has to be greater than one for it to be added to the callbacks queue. * Fix DatabaseTransactionsTest * Fix DatabaseTransactionsManagerTest * CSFixer * wip * Rename method and property and inline usage * Adds a depply nested transaction test * Simplify deeply nesting test * Sets default level value to one (since it's a new parameter) * Rename method * Adds back the removed methods from the db.transactions and mark them as deprecated * StyleCI * Inline the if statement using then collection when() method * Tests observer using afterCommit Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * Apply fixes from StyleCI * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * Apply fixes from StyleCI * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * Apply fixes from StyleCI * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * Apply fixes from StyleCI * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * Apply fixes from StyleCI * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * Update src/Illuminate/Foundation/Testing/DatabaseTransactionsManager.php Co-authored-by: Tony Messias <tonysm@hey.com> * Update src/Illuminate/Database/DatabaseTransactionsManager.php * formatting --------- Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> Co-authored-by: Tony Messias <tonysm@hey.com> Co-authored-by: Taylor Otwell <taylor@laravel.com> Co-authored-by: StyleCI Bot <bot@styleci.io>
laravel · Sep 26, 2023 · ebcb151 · ebcb151
1 parent 169c976
commit ebcb151
Show file tree

Hide file tree

Showing 11 changed files with 275 additions and 25 deletions.
diff --git a/composer.json b/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.12",
         "pda/pheanstalk": "^4.0",
         "phpstan/phpstan": "^1.4.7",
         "phpunit/phpunit": "^10.0.7",

diff --git a/src/Illuminate/Database/Concerns/ManagesTransactions.php b/src/Illuminate/Database/Concerns/ManagesTransactions.php
@@ -47,8 +47,6 @@ public function transaction(Closure $callback, $attempts = 1)
                     $this->getPdo()->commit();
                 }
 
-                $this->transactions = max(0, $this->transactions - 1);
-
                 if ($this->afterCommitCallbacksShouldBeExecuted()) {
                     $this->transactionsManager?->commit($this->getName());
                 }
@@ -58,6 +56,8 @@ public function transaction(Closure $callback, $attempts = 1)
                 );
 
                 continue;
+            } finally {
+                $this->transactions = max(0, $this->transactions - 1);
             }
 
             $this->fireConnectionEvent('committed');
@@ -194,12 +194,12 @@ public function commit()
             $this->getPdo()->commit();
         }
 
-        $this->transactions = max(0, $this->transactions - 1);
-
         if ($this->afterCommitCallbacksShouldBeExecuted()) {
             $this->transactionsManager?->commit($this->getName());
         }
 
+        $this->transactions = max(0, $this->transactions - 1);
+
         $this->fireConnectionEvent('committed');
     }
 
@@ -211,8 +211,7 @@ public function commit()
     protected function afterCommitCallbacksShouldBeExecuted()
     {
         return $this->transactions == 0 ||
-            ($this->transactionsManager &&
-             $this->transactionsManager->callbackApplicableTransactions()->count() === 1);
+            $this->transactionsManager?->afterCommitCallbacksShouldBeExecuted($this->transactions);
     }
 
     /**

diff --git a/src/Illuminate/Database/DatabaseTransactionsManager.php b/src/Illuminate/Database/DatabaseTransactionsManager.php
@@ -7,14 +7,14 @@ class DatabaseTransactionsManager
     /**
      * All of the recorded transactions.
      *
-     * @var \Illuminate\Support\Collection
+     * @var \Illuminate\Support\Collection<int, \Illuminate\Database\DatabaseTransactionRecord>
      */
     protected $transactions;
 
     /**
      * The database transaction that should be ignored by callbacks.
      *
-     * @var \Illuminate\Database\DatabaseTransactionRecord
+     * @var \Illuminate\Database\DatabaseTransactionRecord|null
      */
     protected $callbacksShouldIgnore;
 
@@ -112,13 +112,22 @@ public function callbacksShouldIgnore(DatabaseTransactionRecord $transaction)
     /**
      * Get the transactions that are applicable to callbacks.
      *
-     * @return \Illuminate\Support\Collection
+     * @return \Illuminate\Support\Collection<int, \Illuminate\Database\DatabaseTransactionRecord>
      */
     public function callbackApplicableTransactions()
     {
-        return $this->transactions->reject(function ($transaction) {
-            return $transaction === $this->callbacksShouldIgnore;
-        })->values();
+        return $this->transactions;
+    }
+
+    /**
+     * Determine if after commit callbacks should be executed for the given transaction level.
+     *
+     * @param  int  $level
+     * @return bool
+     */
+    public function afterCommitCallbacksShouldBeExecuted($level)
+    {
+        return $level === 1;
     }
 
     /**

diff --git a/src/Illuminate/Foundation/Testing/DatabaseTransactions.php b/src/Illuminate/Foundation/Testing/DatabaseTransactions.php
@@ -13,19 +13,16 @@ public function beginDatabaseTransaction()
     {
         $database = $this->app->make('db');
 
+        $this->app->instance('db.transactions', $transactionsManager = new DatabaseTransactionsManager);
+
         foreach ($this->connectionsToTransact() as $name) {
             $connection = $database->connection($name);
+            $connection->setTransactionManager($transactionsManager);
             $dispatcher = $connection->getEventDispatcher();
 
             $connection->unsetEventDispatcher();
             $connection->beginTransaction();
             $connection->setEventDispatcher($dispatcher);
-
-            if ($this->app->resolved('db.transactions')) {
-                $this->app->make('db.transactions')->callbacksShouldIgnore(
-                    $this->app->make('db.transactions')->getTransactions()->first()
-                );
-            }
         }
 
         $this->beforeApplicationDestroyed(function () use ($database) {

diff --git a/src/Illuminate/Foundation/Testing/DatabaseTransactionsManager.php b/src/Illuminate/Foundation/Testing/DatabaseTransactionsManager.php
@@ -0,0 +1,47 @@
+<?php
+
+namespace Illuminate\Foundation\Testing;
+
+use Illuminate\Database\DatabaseTransactionsManager as BaseManager;
+
+class DatabaseTransactionsManager extends BaseManager
+{
+    /**
+     * Register a transaction callback.
+     *
+     * @param  callable  $callback
+     * @return void
+     */
+    public function 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->callbackApplicableTransactions()->count() === 0) {
+            return $callback();
+        }
+
+        $this->transactions->last()->addCallback($callback);
+    }
+
+    /**
+     * Get the transactions that are applicable to callbacks.
+     *
+     * @return \Illuminate\Support\Collection<int, \Illuminate\Database\DatabaseTransactionRecord>
+     */
+    public function callbackApplicableTransactions()
+    {
+        return $this->transactions->skip(1)->values();
+    }
+
+    /**
+     * Determine if after commit callbacks should be executed for the given transaction level.
+     *
+     * @param  int  $level
+     * @return bool
+     */
+    public function afterCommitCallbacksShouldBeExecuted($level)
+    {
+        return $level === 2;
+    }
+}
diff --git a/src/Illuminate/Foundation/Testing/RefreshDatabase.php b/src/Illuminate/Foundation/Testing/RefreshDatabase.php
@@ -89,19 +89,16 @@ public function beginDatabaseTransaction()
     {
         $database = $this->app->make('db');
 
+        $this->app->instance('db.transactions', $transactionsManager = new DatabaseTransactionsManager);
+
         foreach ($this->connectionsToTransact() as $name) {
             $connection = $database->connection($name);
+            $connection->setTransactionManager($transactionsManager);
             $dispatcher = $connection->getEventDispatcher();
 
             $connection->unsetEventDispatcher();
             $connection->beginTransaction();
             $connection->setEventDispatcher($dispatcher);
-
-            if ($this->app->resolved('db.transactions')) {
-                $this->app->make('db.transactions')->callbacksShouldIgnore(
-                    $this->app->make('db.transactions')->getTransactions()->first()
-                );
-            }
         }
 
         $this->beforeApplicationDestroyed(function () use ($database) {

diff --git a/tests/Integration/Database/EloquentTransactionWithAfterCommitTest.php b/tests/Integration/Database/EloquentTransactionWithAfterCommitTest.php
@@ -0,0 +1,8 @@
+<?php
+
+namespace Illuminate\Tests\Integration\Database;
+
+class EloquentTransactionWithAfterCommitTest extends DatabaseTestCase
+{
+    use EloquentTransactionWithAfterCommitTests;
+}
diff --git a/tests/Integration/Database/EloquentTransactionWithAfterCommitTests.php b/tests/Integration/Database/EloquentTransactionWithAfterCommitTests.php
@@ -0,0 +1,104 @@
+<?php
+
+namespace Illuminate\Tests\Integration\Database;
+
+use Illuminate\Foundation\Auth\User;
+use Illuminate\Support\Facades\DB;
+use Orchestra\Testbench\Concerns\WithLaravelMigrations;
+use Orchestra\Testbench\Factories\UserFactory;
+
+trait EloquentTransactionWithAfterCommitTests
+{
+    use WithLaravelMigrations;
+
+    protected function setUpEloquentTransactionWithAfterCommitTests(): void
+    {
+        User::unguard();
+    }
+
+    protected function tearDownEloquentTransactionWithAfterCommitTests(): void
+    {
+        User::reguard();
+    }
+
+    public function testObserverIsCalledOnTestsWithAfterCommit()
+    {
+        User::observe($observer = EloquentTransactionWithAfterCommitTestsUserObserver::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 = EloquentTransactionWithAfterCommitTestsUserObserver::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 = EloquentTransactionWithAfterCommitTestsUserObserver::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 = EloquentTransactionWithAfterCommitTestsUserObserver::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 = EloquentTransactionWithAfterCommitTestsUserObserver::resetting());
+
+        $user1 = DB::transaction(function () use ($observer) {
+            return tap(DB::transaction(function () use ($observer) {
+                return tap(DB::transaction(function () use ($observer) {
+                    return tap(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');
+                });
+            }), 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 EloquentTransactionWithAfterCommitTestsUserObserver
+{
+    public static $calledTimes = 0;
+
+    public $afterCommit = true;
+
+    public static function resetting()
+    {
+        static::$calledTimes = 0;
+
+        return new static();
+    }
+
+    public function created($user)
+    {
+        static::$calledTimes++;
+    }
+}
diff --git a/tests/Integration/Database/EloquentTransactionWithAfterCommitUsingDatabaseMigrationsTest.php b/tests/Integration/Database/EloquentTransactionWithAfterCommitUsingDatabaseMigrationsTest.php
@@ -0,0 +1,11 @@
+<?php
+
+namespace Illuminate\Tests\Integration\Database;
+
+use Illuminate\Foundation\Testing\DatabaseMigrations;
+
+class EloquentTransactionWithAfterCommitUsingDatabaseMigrationsTest extends DatabaseTestCase
+{
+    use EloquentTransactionWithAfterCommitTests;
+    use DatabaseMigrations;
+}
diff --git a/.../Integration/Database/EloquentTransactionWithAfterCommitUsingDatabaseTransactionsTest.php b/.../Integration/Database/EloquentTransactionWithAfterCommitUsingDatabaseTransactionsTest.php
@@ -0,0 +1,41 @@
+<?php
+
+namespace Illuminate\Tests\Integration\Database;
+
+use Illuminate\Foundation\Testing\DatabaseTransactions;
+use Orchestra\Testbench\TestCase;
+
+class EloquentTransactionWithAfterCommitUsingDatabaseTransactionsTest extends TestCase
+{
+    use EloquentTransactionWithAfterCommitTests;
+    use DatabaseTransactions;
+
+    /**
+     * The current database driver.
+     *
+     * @return string
+     */
+    protected $driver;
+
+    protected function setUp(): void
+    {
+        $this->beforeApplicationDestroyed(function () {
+            foreach (array_keys($this->app['db']->getConnections()) as $name) {
+                $this->app['db']->purge($name);
+            }
+        });
+
+        parent::setUp();
+
+        if ($this->usesSqliteInMemoryDatabaseConnection()) {
+            $this->markTestSkipped('Test cannot be used with in-memory SQLite connection.');
+        }
+    }
+
+    protected function getEnvironmentSetUp($app)
+    {
+        $connection = $app->make('config')->get('database.default');
+
+        $this->driver = $app['config']->get("database.connections.$connection.driver");
+    }
+}
diff --git a/tests/Integration/Database/EloquentTransactionWithAfterCommitUsingRefreshDatabaseTest.php b/tests/Integration/Database/EloquentTransactionWithAfterCommitUsingRefreshDatabaseTest.php
@@ -0,0 +1,37 @@
+<?php
+
+namespace Illuminate\Tests\Integration\Database;
+
+use Illuminate\Foundation\Testing\RefreshDatabase;
+use Orchestra\Testbench\TestCase;
+
+class EloquentTransactionWithAfterCommitUsingRefreshDatabaseTest extends TestCase
+{
+    use EloquentTransactionWithAfterCommitTests;
+    use RefreshDatabase;
+
+    /**
+     * The current database driver.
+     *
+     * @return string
+     */
+    protected $driver;
+
+    protected function setUp(): void
+    {
+        $this->beforeApplicationDestroyed(function () {
+            foreach (array_keys($this->app['db']->getConnections()) as $name) {
+                $this->app['db']->purge($name);
+            }
+        });
+
+        parent::setUp();
+    }
+
+    protected function getEnvironmentSetUp($app)
+    {
+        $connection = $app['config']->get('database.default');
+
+        $this->driver = $app['config']->get("database.connections.$connection.driver");
+    }
+}