Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This is a fork from this issue. Based on the snippet from Database abstraction layer shown below, a commit will be done automatically when $txn goes out of scope.
function my_transaction_function() {
// The transaction opens here.
$txn = db_transaction();
.
.
.
// $txn goes out of scope here. Unless the transaction was rolled back, it
// gets automatically commited here.
}
It doesn't feel quite right to have commit as the default action because:
- Quote from PHP:
When the script ends or when a connection is about to be closed, if you have an outstanding transaction, PDO will automatically roll it back. This is a safety measure to help avoid inconsistency in the cases where the script terminates unexpectedly--if you didn't explicitly commit the transaction, then it is assumed that something went awry, so the rollback is performed for the safety of your data.
The common practice for a database transaction, whether is MySQL or pgSQL, is to do a default rollback when there is no explicit commit action given. This ensures that for whatever bad happens, inconsistent data are not written. In other words, the coder is sure the data at that point in time is consistent and therefore codes an explicit commit() action there. If something goes wrong, the commit() code will not be executed and rollback should happen. - Quote from PHP:
The destructor will be called even if script execution is stopped using exit().
Therefore, when exit(), drupal_exit() or drupal_goto() is executed, it is as good as doing a commit (tested and true). This might not be the real intention of the coder.
Comment | File | Size | Author |
---|---|---|---|
#22 | auto-commit-7.patch | 20.91 KB | c960657 |
#20 | auto-commit-6.patch | 18.67 KB | c960657 |
#17 | auto-commit-5.patch | 18.41 KB | c960657 |
#11 | auto-commit-4.patch | 22.86 KB | c960657 |
#10 | auto-commit-3.patch | 22.81 KB | c960657 |
Comments
Comment #1
bvanmeurs CreditAttribution: bvanmeurs commentedTotally agree.
It was a poor decision to not require an explicit commit in the first place.
Comment #2
sunThere's no actual bug report here, so this is something to discuss for D8.
Comment #3
catchI don't think this is major and there are other issues open for transaction handling that need to be resolved first.
Comment #4
c960657 CreditAttribution: c960657 commentedI tend to agree that we should require transactions to be committed explicitly. People use transactions because they care about consistency. It seems counter-productive if transactions may be committed “by mistake” e.g. due to an uncaught exception (like in #1022416: Wrong use of db_transaction() in block module), or if you forget to call $transaction->rollback() in a function using the “return early” pattern. When you return early, it is often due to some error detection that may be hard to reproduce during testing.
If commits should be made explicitly, and your forget to call $transaction->commit(), then you will notice immediately and fix the problem.
Also, I think an explicit commit() will make code easier to read. It's somewhat similar to the recommendation of always using curly brackets in if statements, even when they are not required.
Comment #5
pounardAgree and it cause some pain when modules use it in a wrong way, such as I described in the #1608374: Database autocommit disabling at shutdown issue.
Comment #6
c960657 CreditAttribution: c960657 commentedHow about this?
Comment #7
c960657 CreditAttribution: c960657 commentedComment #9
pounardIt sounds like a very good step toward more robust code. I like it.
Comment #10
c960657 CreditAttribution: c960657 commentedComment #11
c960657 CreditAttribution: c960657 commentedReroll (due to conflict with the fix for #1376778: Consistent 'duplicate key' detection in core).
Comment #13
c960657 CreditAttribution: c960657 commented#11: auto-commit-4.patch queued for re-testing.
Comment #15
c960657 CreditAttribution: c960657 commented#11: auto-commit-4.patch queued for re-testing.
Comment #16
c960657 CreditAttribution: c960657 commentedNow the test bot is happy.
Comment #17
c960657 CreditAttribution: c960657 commentedReroll.
Comment #19
jbrown CreditAttribution: jbrown commentedSeems the title was wrong.
Comment #20
c960657 CreditAttribution: c960657 commentedComment #22
c960657 CreditAttribution: c960657 commentedComment #23
jbrown CreditAttribution: jbrown commented#22: auto-commit-7.patch queued for re-testing.
Comment #24
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe are well into API freeze.
Comment #25
Stefan Freudenberg CreditAttribution: Stefan Freudenberg commentedGiven that the current code contravenes a feature of PDO to protect data integrity I would say the current implementation is a logical bug. I also do not see how the API freeze justifies postponing this issue. The only change I see is the change to a protected property of a class that is unlikely to be subclassed. Did I overlook a signature change to a public method?
Comment #26
sunI actually wonder whether explicitly committing transactions wouldn't (1) increase reliability and (2) be more consistent with the rest of D8's architecture?
Perhaps, as a compromise, could this change be implemented in a backwards-compatible way? I.e., allow code to explicitly commit + update transactions throughout core, but keep the auto-commit on destruction behavior (unless committed already)?
I don't want to step on @Damien's toes (as he's one of the dedicated database system maintainers), so only tentatively moving back to D8 for this potential compromise. (@Damien, please move it back to D9, if that's a no-go.)
Comment #27
Stefan Freudenberg CreditAttribution: Stefan Freudenberg commentedHm. As stated in the issue description and comments, an automatic commit in the PHP shutdown sequence is surprising. It can lead to transactions being committed although not all queries and code have been executed that should occur together or not at all. It is bad for developer experience to design a behaviour that contravenes documentation on php.net.
Comment #28
jhedstromNot sure if this can still go in or not, but will need a reroll if so.
Comment #31
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedThis looks to me like the patch has to be rethought a bit rather than rerolled...
Comment #32
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedComment #37
anoopjohn CreditAttribution: anoopjohn at Zyxware Technologies commentedWas there a decision on this?
Comment #40
Inaetaru CreditAttribution: Inaetaru commentedWhen there is a variable typo error in code between DB operations during transaction, catch block with rollback isn't called and transaction is committed, at least in dev environment. Just out of curiosity: is this behaviour considered a bug (transaction is committed although there was an error) or a feature (to make sure developers make 100% coverage tests)?
Comment #44
AnybodySwitching this to Drupal 10 and hooking in :)
Just came across this and was wondering about why explicit commits are forbidden: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%...
Guess I'm missing the details here yet, but I think there are valid examples where you want to decide explicitly at which point to commit or roll back (only the part from starting the transaction to the commit / rollback, not everything).
For example, only a certain part of an execution should be rolled back or committed. The current implementation seems to roll back anything after the transation was started, even behind the try / catch block.
Where can we find further information about these decisions any why they were made? I'd clearly vote to allow explicit commits where it makes sense!
Thanks :)
Reading the docs at https://www.drupal.org/docs/drupal-apis/database-api/database-transactions I'm totally sure that
is totally unintuitive and against what all other frameworks, especially PDO, Doctrine, etc. do:
What's expected here is
->commit();
and notunset()
. That should be hidden behind the API or whatever. Currentunset()
implementation might be technically perfect, but is bad DX.I'd bet that nobody will intuitively understand
without a comment! That may lead to crazy results...
Comment #45
mondrakeDid not see this before, there’s now a MR for this built on top of the new TransactionManager in D10.2
Comment #46
Anybody