Allow explicit COMMIT in Transaction objects

+1 for me.

I really dislike unset($transaction); for the implicit commit.

OK, let's try this then.

Rebased to run the pipeline and it was all green

Ran the test-only feature and they all seemed to still pass shouldn't the base test file fail? https://git.drupalcode.org/issue/drupal-3398767/-/jobs/374406

Seems like something that could use a change record right?

It should in this case, but I don’t think #3395977-4: Test-only changes reverts changes to test modules → was addressed and here the change was to a test base class that got reverted too?

Need to postpone on 🐛 Leaving the savepoint in the transaction stack upon rollback is incorrect Needs review

Postponed on ✨ [PP-1] Introduce a Connection::executeDdlStatement method Active . That will give us the possibility to know about an autocommit due to lack of transactional DDL support having happened BEFORE actually attempting a commit on the DB, and appropriate test coverage, too.

Also we probably need to split the cleanup of DriverSpecificTransactionTestBase - there's a lot of bolierplate that was accumulated by copy/pasting test methods that can be removed.

Filed 📌 Cleanup DriverSpecificTransactionTestBase Needs review .

Note to self: when this is unblocked, do #3386263-10: Ignore, testing issue → first thing.

We now have \Drupal\Core\Database\Database::commitAllOnShutdown. Should we keep this issue open or change it somehow?

#18 this issue is about moving AWAY from ::commitAllOnShutdown() (and in general from committing during object destruction which has drawbacks).

IMHO, ::commitAllOnShutdown() should only be transitional towards such goal.

rebased

Updated title and IS

Green on all three core supported databases! Ready for review.

I am bumping to Major as it's known there are problems with the current handling of autocommit in HEAD.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

rebased

Thanks for review @daffie!

In general, I just c/p DriverSpecificTransactionTestBase into the new TransactionYieldTest, to try and keep the two test sets (one testing the commit-on-destruct behavior, one testing the commit-on-yield one) in sync, doing as few changes as possible. Anyway, I will look into your input and adjust as much as possible.

All the code changes look good to me.
The CI pipeline is green for all 3 databases.
For the new method we shall need a CR.

Added CR https://www.drupal.org/node/3512006 →

Thanks @daffie!

The CR looks good to me.
All code changes look good to me.
The CI pipeline is green for all 3 databases.
For me it is RTBC.

Added some comments to the MR.

I think we should consider having a commit method for situations where you know that you've created the root transaction. I think we should also consider having a startRootTransaction() method to support this. So you can have very explicit code. Yield is a tricky word to use as it already has quite a bit of meaning in PHP and normally means to yield a value as part of an iteration.

Thanks for review @alexpott.

Transaction does not have an interface for now.

We could add an additional Connection::startSavepoint() method, and Transaction::commit() + Transaction::releaseSavepoint() methods. That will somehow force how transactions are managed in code, though, in the sense that devs will have to be aware of whether they are working within the root transaction or within a savepoint. If some code starts a root transaction, you would not be able to encapsulate that code in a higher method if that will try to start a root transaction as well.

The beauty of the MR here is that you do not really care - you always call Connection::startTransaction and then it's the manager deciding what to do depending on the stack depth.

I am not going to make changes until we have agreed on a plan forward.

NR for the proposal in #38.

I am not going to make changes until we have agreed on a plan forward.

Yeah 100%.

One thing I'm thinking about is rollback... Atm we call rollback on a transaction and that will go back to your previous savepoint or the end the root transaction without a commit - right? So given rollback already does 2 different things... why don't we rename yield to commit() and document that it will commit the root transaction or release the savepoint depending on transaction depth as we already have this behaviour with rollback... what do you think. My concern is the yield is an odd word wrt to databases and transactions.

Apologies if this sounds a bit scholarly... not my intention.

One thing I'm thinking about is rollback... Atm we call rollback on a transaction and that will go back to your previous savepoint or the end the root transaction without a commit - right? So given rollback already does 2 different things...

Right. But IMHO we should not assimilate the combination commit/release savepoint to the rollback. Commit permanently saves data to the DBMS, release savepoint doesn't. Rollback, doesn't matter whether to a savepoint or to the begin of the transactions, just returns to a temporary state within the transaction, it does not persist anything. Commit is very a specific, unambiguous action.

why don't we rename yield to commit() and document that it will commit the root transaction or release the savepoint depending on transaction depth as we already have this behaviour with rollback...

In an earlier version of the MR, it was like that. Then, I pondered the above and thought that a 'commit' method that does not really persist anything may be seen as a WTF - anyone not aware of all the details/not reading the docs would call 'commit' on a savepoint and then complain data are not saved.

For me if we add a Transaction::commit() method we should not be allowing ambiguity: it must persist the data as everyone would expect.

So, how about this next proposal:

• once instantiating a new Transaction object, store the transaction type in it
• add a Transaction::commit() and a Transaction::releaseSavepoint() methods, to be used respectively for... what they respectively mean
• in case we need to let the manager decide whether to commit or release a savepoint, we could still have a method like ::yield() with an appropriate rename, or we could have a simple match construct in calling code like
  

  match ($transaction->type) {
      StackItemType::Root => $transaction->commit(),
      StackItemType::Savepoint => $transaction->releaseSavepoint(),
  }

I like the change to only using Transaction::commit() or Transaction::releaseSavepoint() and having no Transaction::yield(). Developer need to learn the difference between to two.
The proposed changes look good to me.

I like the commitOrRelease name way more than yield. I like specific methods for commit and releaseSavepoint being public.My only concern is that commit not being an alias for commitOrRelease() might be problematic - because ATM DB transaction users don't need to be concerned if something wraps their code in another transaction because the abstraction layer hides this complexity away from them.

So we just go ahead renaming yield() to commitOrRelease() without implementing commit() and releaseSavepoint()?

I’d be +1 on that as I really value not having to care about what method to use, and the manager caring about that.

Yeah lets do #44. We can add explicit methods later if needs be.

Let’s do #44

On ir.

Updated IS with #44

fwiw I was behind on this issue and only read the last few comments just now, but #44 sounds good and means we don't need to worry about implications of #43, so extra +1.

Done

Updated CR with the latest approach.

All use of the method yield() has been replaced with commitOrRelease().
The IS and the CR are updated.
Back to RTBC.

Please consider crediting contributors to 📌 Transactions should be allowed to be committed explicitly Needs work , that will become outdated when this is committed.

fwiw, the 📌 [PP-1] Convert all transactions to explicit COMMIT Postponed follow-up MR is built on top of this MR, and passes all tests.