[10.x] After commit callback throwing an exception causes transaction level to be incorrect

[oprypkhantc]

Hey.

Whenever an afterCommit callback throws an exception inside of a DB::transaction() closure, it's caught by the ManagesTransactions trait and internal ->transactions transaction level is decremented. In tests that use DatabaseTransactions trait this makes DB::transactionLevel() return 0, although the real transaction level (on the database side) is still 1.

class SomeTest extends TestCase 
{
    use DatabaseTransactions;

    public function testSomething()
    {
        $this->assertSame(1, DB::transactionLevel());
    
        try {
            DB::transaction(function () {
                $this->assertSame(2, DB::transactionLevel());
            
                DB::afterCommit(fn () => throw new \RuntimeException());
            });
        } catch (\RuntimeException) {}
        
        // Currently fails
        $this->assertSame(1, DB::transactionLevel());
    }
}

This PR fixes this by running after commit callbacks outside of the try-catch, next to the "committing" event.

[taylorotwell]

I dunno - is this causing many real world problems? If so, please describe them from your own application and re-submit the PR. This code seems to be pretty fragile and if a lot of community members aren't raising serious bugs with it I would rather not touch it.

Can you share more practical problem other than asserting against the internal transaction level and re-submit if necessary?

[oprypkhantc]

@taylorotwell It is causing problems, otherwise I wouldn't be submitting this PR 🤷🏻‍♂️ .

Some context: we're a large project that has 20k tests, nearly all of which use DatabaseTransactions trait. We've had our own implementation of "after commit" jobs, notifications and events/listeners long before Laravel first introduced it and have tackled a lot of the nuances that come with it. We're now trying to move away from our custom implementation in favour of Laravel built-in methods. We did not encounter any issues with this in our own implementation because we relied on committed event which is outside of the try-catch block, so it works as expected even when something throws. In our case, this bug causes a lot of "negative" tests to fail that call two or more endpoints.

The reason the community doesn't seem to be aware of this is because it's a niche bug in a niche feature: it's only reproducible in tests with DatabaseTransactions trait, when using afterCommit that throws, and two or more transactions. I don't think too many projects even use afterCommit, let alone have 20k tests for the code using it. This makes it very unlikely to stumble upon for a regular Laravel user.

The internal transactions level is very important to be kept in full sync with the actual connection's level. Not doing so causes things not being committed, or being committed at a wrong time, or rolling back the transaction when not expected. For example:

class SomeTest extends TestCase 
{
    use DatabaseTransactions;

    public function testSomething()
    {
        $this->assertSame(1, DB::transactionLevel());
    
        try {
            DB::transaction(function () {
                $this->assertSame(2, DB::transactionLevel());
            
                DB::afterCommit(fn () => throw new \RuntimeException());
            });
        } catch (\RuntimeException) {}
        
        // Incorrect transaction level here
        // That's also the reason why `DatabaseTransactions` trait is important: without it the transaction level would
        // actually be 0 by the time `afterCommit` callbacks even execute
        $this->assertSame(0, DB::transactionLevel());
        
        // Case 1:
        // Instead of creating a savepoint, this attempts to create a transaction, which fails with a PDO exception:
        //   PDOException: There is already an active transaction
        DB::beginTransaction();
        
        // Case 2:
        // Instead of making an actual "COMMIT;" query, this will silently not do anything, thinking it's outside of a
        // transaction. Nothing will be committed, as the connection believes the transaction level to be 0.
        DB::commit();
        
        // Case 3:
        // Similar to the commit case, this won't actually do anything as it believes the connection is at transaction level 0.
        DB::rollBack();
    }
}

Obviously we generally don't use transactions directly in tests, but integration tests do call real endpoints/commands which do use transactions, which causes tests to fail. The real use case looks more like this:

class SomeTest extends TestCase 
{
    use DatabaseTransactions;

    public function testSomething()
    {
        $this->getJson('some_endpoint_with_after_commit_error')
            ->assertUnprocessable()
            ->assertJson(['code' => 'something']);
            
        // Fails immediately due to `PDOException: There is already an active transaction`
        $this->getJson('some_endpoint_with_any_transaction')
            ->assertOk();
    }
}