Framework: Throw exception when committing and no transaction is started

[digilist]

• Laravel Version: master (is a "feature" since #14085 / 5.2)
• PHP Version: irrelevant
• Database Driver & Version: irrelevant

Description:

https://github.com/laravel/framework/blob/e778e3296dce73fddf772844158a8c87f1857bb5/src/Illuminate/Database/Concerns/ManagesTransactions.php#L154-L163

In cases where the user calls commit() more often than beginTransaction() (e.g. because beginTransaction() is removed by accident) the user will never notice.

This method should throw an exception instead, because otherwise the user will never notice (that the transaction has been committed to early or that no transaction is active at all).

@GrahamCampbell already pointed to that issue in #12382 and #14085 but was ignored...

[driesvints]

Heya, thanks for submitting this.

This seems like a feature request or an improvement. It's best to post these at the laravel/ideas repository to get support for your idea. After that you may send a PR to the framework. Please only use this issue tracker to report bugs and issues with the framework.

Thanks!

[digilist]

This is not a feature, it's a bug and can lead to serious data inconsistencies without anybody noticing it! (actually happend in the project I am currently working on)

[driesvints]

@digilist then why did you mark this as "feature" in your issue template? Please try to properly fill out issue templates when posting them.

I think maybe an exception could indeed work here. Can you maybe send in a PR with a test?

[aman00323]

@driesvints I have created PR, it is first time i am contributing, please drive me if i have done something wrong thanks. :)

[driesvints]

A pr for this fixed this for 6.0: https://github.com/laravel/framework/pull/29067

[digilist]

This PR does not fix the handling of commit(), does it? It's only fixing another bug in the transaction method, but you can use commit() without using transaction().

[driesvints]

@digilist right! My bad, sorry.

[BobMali]

@driesvints I have opened a PR on this issue. First time contributor here, please be gentle. 😄

[driesvints]

Unfortunately we've decided that we won't be pursuing a fix for this, sorry. The risk that it'd break apps is too great.

[digilist]

@driesvints So having a bug that might silently lead to inconsistent databases is better than forcing some users to fix this? Why not start at least with logging on critical level and later convert it to an exception?

[driesvints]

@digilist I'm sorry but we had to weigh against the downside of this breaking apps. You're free to open an issue on the ideas repo to think about alternative solutions or what's your proposing in the above comment.