[10.x] Fixed implementation related to afterCommit on Postgres and …

…MSSQL database drivers (laravel#48662) * fix expected level in afterCommitCallbacksShouldBeExecuted * remove callbacksShouldIgnore * Fixed transaction level down timing * Fixed transaction level down timing * Add test - after commit is executed on final commit * style fix style fix * [10.x] Test Improvements Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> * wip Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> --------- Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com> Co-authored-by: 武田憲太郎 <takeda@youmind.jp> Co-authored-by: Mior Muhammad Zaki <crynobone@gmail.com>
timacdonald · Oct 24, 2023 · e3f754a · e3f754a
1 parent 1532c1d
commit e3f754a
Show file tree

Hide file tree

Showing 5 changed files with 70 additions and 36 deletions.
diff --git a/src/Illuminate/Database/Concerns/ManagesTransactions.php b/src/Illuminate/Database/Concerns/ManagesTransactions.php
@@ -47,6 +47,8 @@ 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());
                 }
@@ -56,8 +58,6 @@ 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');
     }
 
@@ -210,8 +210,7 @@ public function commit()
      */
     protected function afterCommitCallbacksShouldBeExecuted()
     {
-        return $this->transactions == 0 ||
-            $this->transactionsManager?->afterCommitCallbacksShouldBeExecuted($this->transactions);
+        return $this->transactionsManager?->afterCommitCallbacksShouldBeExecuted($this->transactions) || $this->transactions == 0;
     }
 
     /**

diff --git a/src/Illuminate/Database/DatabaseTransactionsManager.php b/src/Illuminate/Database/DatabaseTransactionsManager.php
@@ -11,13 +11,6 @@ class DatabaseTransactionsManager
      */
     protected $transactions;
 
-    /**
-     * The database transaction that should be ignored by callbacks.
-     *
-     * @var \Illuminate\Database\DatabaseTransactionRecord|null
-     */
-    protected $callbacksShouldIgnore;
-
     /**
      * Create a new database transactions manager instance.
      *
@@ -54,10 +47,6 @@ 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;
-        }
     }
 
     /**
@@ -75,10 +64,6 @@ public function commit($connection)
         $this->transactions = $forOtherConnections->values();
 
         $forThisConnection->map->executeCallbacks();
-
-        if ($this->transactions->isEmpty()) {
-            $this->callbacksShouldIgnore = null;
-        }
     }
 
     /**
@@ -96,19 +81,6 @@ public function addCallback($callback)
         $callback();
     }
 
-    /**
-     * Specify that callbacks should ignore the given transaction when determining if they should be executed.
-     *
-     * @param  \Illuminate\Database\DatabaseTransactionRecord  $transaction
-     * @return $this
-     */
-    public function callbacksShouldIgnore(DatabaseTransactionRecord $transaction)
-    {
-        $this->callbacksShouldIgnore = $transaction;
-
-        return $this;
-    }
-
     /**
      * Get the transactions that are applicable to callbacks.
      *
@@ -127,7 +99,7 @@ public function callbackApplicableTransactions()
      */
     public function afterCommitCallbacksShouldBeExecuted($level)
     {
-        return $level === 1;
+        return $level === 0;
     }
 
     /**

diff --git a/src/Illuminate/Foundation/Testing/DatabaseTransactionsManager.php b/src/Illuminate/Foundation/Testing/DatabaseTransactionsManager.php
@@ -42,6 +42,6 @@ public function callbackApplicableTransactions()
      */
     public function afterCommitCallbacksShouldBeExecuted($level)
     {
-        return $level === 2;
+        return $level === 1;
     }
 }
diff --git a/tests/Database/DatabaseConnectionTest.php b/tests/Database/DatabaseConnectionTest.php
@@ -7,6 +7,7 @@
 use Exception;
 use Illuminate\Contracts\Events\Dispatcher;
 use Illuminate\Database\Connection;
+use Illuminate\Database\DatabaseTransactionsManager;
 use Illuminate\Database\Events\QueryExecuted;
 use Illuminate\Database\Events\TransactionBeginning;
 use Illuminate\Database\Events\TransactionCommitted;
@@ -289,6 +290,20 @@ public function testCommittingFiresEventsIfSet()
         $connection->commit();
     }
 
+    public function testAfterCommitIsExecutedOnFinalCommit()
+    {
+        $pdo = $this->getMockBuilder(DatabaseConnectionTestMockPDO::class)->onlyMethods(['beginTransaction', 'commit'])->getMock();
+        $transactionsManager = $this->getMockBuilder(DatabaseTransactionsManager::class)->onlyMethods(['afterCommitCallbacksShouldBeExecuted'])->getMock();
+        $transactionsManager->expects($this->once())->method('afterCommitCallbacksShouldBeExecuted')->with(0)->willReturn(true);
+
+        $connection = $this->getMockConnection([], $pdo);
+        $connection->setTransactionManager($transactionsManager);
+
+        $connection->transaction(function () {
+            // do nothing
+        });
+    }
+
     public function testRollBackedFiresEventsIfSet()
     {
         $pdo = $this->createMock(DatabaseConnectionTestMockPDO::class);

diff --git a/tests/Integration/Database/EloquentTransactionWithAfterCommitTests.php b/tests/Integration/Database/EloquentTransactionWithAfterCommitTests.php
@@ -2,7 +2,11 @@
 
 namespace Illuminate\Tests\Integration\Database;
 
+use Illuminate\Bus\Queueable;
+use Illuminate\Contracts\Queue\ShouldQueue;
 use Illuminate\Foundation\Auth\User;
+use Illuminate\Foundation\Bus\Dispatchable;
+use Illuminate\Queue\InteractsWithQueue;
 use Illuminate\Support\Facades\DB;
 use Orchestra\Testbench\Concerns\WithLaravelMigrations;
 use Orchestra\Testbench\Factories\UserFactory;
@@ -41,6 +45,21 @@ public function testObserverCalledWithAfterCommitWhenInsideTransaction()
         $this->assertEquals(1, $observer::$calledTimes, 'Failed to assert the observer was called once.');
     }
 
+    public function testObserverCalledWithAfterCommitWhenInsideTransactionWithDispatchSync()
+    {
+        User::observe($observer = EloquentTransactionWithAfterCommitTestsUserObserverUsingDispatchSync::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.');
+
+        $this->assertDatabaseHas('password_reset_tokens', [
+            'email' => $user1->email,
+            'token' => sha1($user1->email),
+        ]);
+    }
+
     public function testObserverIsCalledOnTestsWithAfterCommitWhenUsingSavepoint()
     {
         User::observe($observer = EloquentTransactionWithAfterCommitTestsUserObserver::resetting());
@@ -102,3 +121,32 @@ public function created($user)
         static::$calledTimes++;
     }
 }
+
+class EloquentTransactionWithAfterCommitTestsUserObserverUsingDispatchSync extends EloquentTransactionWithAfterCommitTestsUserObserver
+{
+    public function created($user)
+    {
+        dispatch_sync(new EloquentTransactionWithAfterCommitTestsJob($user->email));
+
+        parent::created($user);
+    }
+}
+
+class EloquentTransactionWithAfterCommitTestsJob implements ShouldQueue
+{
+    use Dispatchable, InteractsWithQueue, Queueable;
+
+    public function __construct(public string $email)
+    {
+        // ...
+    }
+
+    public function handle(): void
+    {
+        DB::transaction(function () {
+            DB::table('password_reset_tokens')->insert([
+                ['email' => $this->email, 'token' => sha1($this->email), 'created_at' => now()],
+            ]);
+        });
+    }
+}