Transactions should be allowed to be committed explicitly

Totally agree.

It was a poor decision to not require an explicit commit in the first place.

There's no actual bug report here, so this is something to discuss for D8.

I don't think this is major and there are other issues open for transaction handling that need to be resolved first.

I 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.

Agree 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.

How about this?

It sounds like a very good step toward more robust code. I like it.

Reroll (due to conflict with the fix for #1376778: Consistent 'duplicate key' detection in core).

#11: auto-commit-4.patch queued for re-testing.

#11: auto-commit-4.patch queued for re-testing.

Now the test bot is happy.

Reroll.

Seems the title was wrong.

#22: auto-commit-7.patch queued for re-testing.

We are well into API freeze.

Given 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?

I 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.)

Hm. 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.

Not sure if this can still go in or not, but will need a reroll if so.

drupal8 (reroll) $ git rebase 8.3.x
First, rewinding head to replay your work on top of it...
Applying: Applied patch
Using index info to reconstruct a base tree...
M	core/includes/database.inc
M	core/includes/menu.inc
M	core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
M	core/lib/Drupal/Core/Database/Query/Insert.php
M	core/lib/Drupal/Core/Database/Transaction.php
A	core/lib/Drupal/Core/Entity/DatabaseStorageController.php
A	core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
M	core/lib/Drupal/Core/Routing/MatcherDumper.php
A	core/modules/field_sql_storage/field_sql_storage.module
A	core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
A	core/modules/system/lib/Drupal/system/Tests/Database/TransactionTest.php
Falling back to patching base and 3-way merge...
Auto-merging core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php
CONFLICT (content): Merge conflict in core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php
CONFLICT (modify/delete): core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php deleted in HEAD and modified in Applied patch. Version Applied patch of core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php left in tree.
CONFLICT (modify/delete): core/modules/field_sql_storage/field_sql_storage.module deleted in HEAD and modified in Applied patch. Version Applied patch of core/modules/field_sql_storage/field_sql_storage.module left in tree.
Auto-merging core/lib/Drupal/Core/Routing/MatcherDumper.php
CONFLICT (content): Merge conflict in core/lib/Drupal/Core/Routing/MatcherDumper.php
CONFLICT (modify/delete): core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php deleted in HEAD and modified in Applied patch. Version Applied patch of core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php left in tree.
CONFLICT (modify/delete): core/lib/Drupal/Core/Entity/DatabaseStorageController.php deleted in HEAD and modified in Applied patch. Version Applied patch of core/lib/Drupal/Core/Entity/DatabaseStorageController.php left in tree.
Auto-merging core/lib/Drupal/Core/Database/Transaction.php
CONFLICT (content): Merge conflict in core/lib/Drupal/Core/Database/Transaction.php
Auto-merging core/lib/Drupal/Core/Database/Query/Insert.php
CONFLICT (content): Merge conflict in core/lib/Drupal/Core/Database/Query/Insert.php
Auto-merging core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
Auto-merging core/includes/menu.inc
CONFLICT (content): Merge conflict in core/includes/menu.inc
Auto-merging core/includes/database.inc
CONFLICT (content): Merge conflict in core/includes/database.inc
Failed to merge in the changes.

This looks to me like the patch has to be rethought a bit rather than rerolled...

Was there a decision on this?

When 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)?

Switching 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

// Commit the transaction by unsetting the $transaction variable.
unset($transaction);

is totally unintuitive and against what all other frameworks, especially PDO, Doctrine, etc. do:

• https://www.doctrine-project.org/projects/doctrine-orm/en/2.14/reference...
• https://www.php.net/manual/en/pdo.commit.php
• ...

What's expected here is ->commit(); and not unset(). That should be hidden behind the API or whatever. Current unset() implementation might be technically perfect, but is bad DX.

I'd bet that nobody will intuitively understand

unset($transaction);

without a comment! That may lead to crazy results...

Did not see this before, there’s now a MR for this built on top of the new TransactionManager in D10.2