[10.x] Fixed implementation related to afterCommit on Postgres and MSSQL database drivers

[SakiTakamachi]

Overview

fixes #48658

Hi

I checked the implementation regarding afterCommit, fixed defects, and deleted unused functions.

Cause of the problem

The cause was that by calling afterCommit before lowering the transaction level, the transaction behaved as if it existed, even though it actually did not.

I solved this problem by lowering the transaction level before calling afterCommit, and by lowering the level expected by afterCommitCallbacksShouldBeExecuted() by one step.

I also fixed commit() in the same way.

Other problem

The finally of ManagesTransactions::transaction is unnecessary, so it has been removed. Since the transaction level is lowered with handleCommitTransactionException(), using finally will lower the transaction level by two levels.

Removed unnecessary features

I removed the functionality related to callbacksShouldIgnore as it is no longer used anywhere.

How should TransactionsManager behave?

During normal execution, afterCommit is expected to be executed when the transaction no longer exists. That is, when the transaction level is 0.

When testing using RefreshDatabase, the transaction level is increased by +1 first, as shown in the following link.

framework/src/Illuminate/Foundation/Testing/RefreshDatabase.php

Line 100 in cddb4f3

$connection->beginTransaction();

So in this case, transaction level 1 will be the lowest, and we would expect afterCommit to be called when level drops to 1.

notes

I swapped the order of the OR in afterCommitCallbacksShouldBeExecuted.
This should basically all be handled with $this->transactionsManager?->afterCommitCallbacksShouldBeExecuted($this->transactions), and to make it clear that $this->transactions == 0 is a fallback is.

Best regards.

[SakiTakamachi]

The test has failed, so I'll draft it for now.

[KentarouTakeda]

The failing test doesn't seem to be related to this pull request. It's like a test with a probability of failure.

$ echo PING | redis-cli
PONG

$ git branch
* 10.x
  fix/gh-48658

$ git rev-parse HEAD
cddb4f3bb5231f44f18fce304dbf190ad8348ddc

$ i=0; while true; do ((i++)); phpunit tests/Integration/Queue/ThrottlesExceptionsWithRedisTest.php > /dev/null || { echo "Failed on attempt $i"; break; }; done
Failed on attempt 69

$ i=0; while true; do ((i++)); phpunit tests/Integration/Queue/ThrottlesExceptionsWithRedisTest.php > /dev/null || { echo "Failed on attempt $i"; break; }; done
Failed on attempt 83

When executed in an infinite loop at the beginning of the 10.x branch, there is a very low probability of failure.

[crynobone]

We still need regression tests covering the issue posted to confirm that this fix the issue.

[SakiTakamachi]

@crynobone
@KentarouTakeda added a test.
Thanks!

[crynobone]

@SakiTakamachi Can you merge this PR: SakiTakamachi#2

[SakiTakamachi]

@crynobone
Thank you, I merged it.

[taylorotwell]

Thanks!

[azgooon]

Thank you all!

[tonysm]

Nice. Thanks, y'all! 👍🏼