DB transaction level never hits 0

[mvanduijker]

• Laravel Version: 6.9.0
• PHP Version: any
• Database Driver & Version: any

Description:

PR #30883 has a change that current transaction level is in the committed event. I think this is a pretty hefty change which wasn't noted in the release notes. It is fairly common to check for the root transaction with the transaction level (which is now 1 instead of 0). Packages like https://github.com/therezor/laravel-transactional-jobs, https://github.com/mvanduijker/laravel-transactional-model-events, https://github.com/mvanduijker/laravel-transactional-mails and probably many applications are (silently) breaking. I do understand the change, but can this be reverted and re-introduced in Laravel 7 with a mention in the upgrade guide?

Steps To Reproduce:

The opposite of #30756

[driesvints]

Aren't those packages depending on broken behavior? The PR that changed this actually uses the correct behavior now.

[mvanduijker]

It's depending on behaviour how it has always been. It's questionable if behaviour was broken. You could say that db transaction levels are indexed by 0. (which might be the original thought behind this) Then the first transaction you are in is level 0. (which makes writing the check on root transaction a bit shorter as well)

My biggest problem is not perse what is the right way, but it is unexpected and undocumented inconvenience for a minor release.

[mvanduijker]

To make things a bit more clear. This is only troublesome if you listen on the TransactionCommitted event. So it does reach 0. The inline documentation for the transactionLevel method says: Get the number of active transactions As a programmer I expect this to be always true. I expect the TransactionCommitted event to be fired after the transaction is completed. Now when you are listening on TransactionCommitted event you get the transaction level when it was committed not what it currently is. In my opinion it would be more correct to add the transaction level, when it was committed, as a property on the event.

Anyways it is a matter of opinion. If it is gonna be left like it is now, I will fix my packages. Just wanting to be sure it is gonna stay like this.

[driesvints]

@martinbean can you maybe weigh in here as well?

[martinbean]

@driesvints Sure.

@mvanduijker I instigated this change, but didn’t have time to code the PR before the holidays. I raised the issue as there were events raised around transactions: beginning, committed, and rolled back. I noticed that if I started a transaction, a TransactionBeginning event was dispatched with a transaction level value of 1. When that transaction was committed, a TransactionCommitted event was dispatched, but with its transaction level value of 0. The same if the transaction was rolled back.

To me, if there are events for when a transaction is starting and ending, then I’d expect those events to contain the same transaction level, i.e. transaction level 1 started, transaction level 1 ended. I wasn’t able to listen for a transaction being committed/rolled back without having to do a -1 operation to get the same transaction level as when that transaction started.

[Miguel-Serejo]

Gotta say I disagree with the change here.

If I'm listening for the event TransactionCommitted, I expect it to happen after the commit, so I would also expect the current transaction level to be returned. I would always expect the transaction level on the connection object to be the current transaction level.

This was also a fairly clear breaking change. Even if this is to be changed, it should be reverted until 7.0.

[martinbean]

@36864 OK. So I want something to happen on the start and end of a particular transaction level.

I do something. A TransactionBeginning event is dispatched. I look at the transaction level: 1. The transaction concludes. A TransactionCommitted event is dispatched. Cool. Let’s look at the transaction level: what? 0?

So how am I supposed to know when transaction level 1 was committed/rolled back? Just do $transactionLevel - 1 in all of my listeners on the committed/rolled back events…?

[Miguel-Serejo]

That is what I would prefer to do, yes. Because as it is now, there is no way to know if the transaction level ever reaches 0. So if I want to do something when there are no more pending transactions, I need to listen for the transaction level to equal 1, which isn't intuitive from my point of view.

Right now, a transaction level of 0 means "No pending transactions", and a transaction level of n > 0 means "n pending transaction, unless called in an event listener for these specific events, in which case there are n-1 pending transactions".

Could your use case be solved by adding an extra parameter to the events to indicate the "old" transaction level?

Either way, I think this should only be changed in a major version to avoid breaking existing code.

EDIT:
I'm not too familiar with this part of the framework, but after looking at the relevant PR, it looks to me like an exception happening in the event listener could now prevent the transaction level from being decremented. Is this an issue or is this handled elsewhere?

[driesvints]

@martinbean do you think it's best that we revert for now and try to tackle this in a next major version? At least to give the discussion a bit more time to play out.

[newerton]

The change in DatabaseManager has stopped my tests.

I have an \Event::listen with TransactionCommitted that terminates the connection, and the error occurs in \DB:rollback()

[driesvints]

We've reverted this for now. @martinbean we could reconsider this for either 7.0 or 8.0 but let's first open up a discussion in the ideas repo. Probably mention some of the people in this thread and link to the relevant issues/prs. Thanks