Allow returning explicitly to the prior nesting level in transactions (aka allow explicit COMMIT in Transaction objects)

OK, let's try this then.

Ran the test-only feature and they all seemed to still pass shouldn't the base test file fail?

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?

Let’s wait for more comments before jumping into CR writing

Need to postpone on #3407080: Leaving the savepoint in the transaction stack upon rollback is incorrect

Postponed on #3384999: Introduce a Schema::executeDdlStatement method. 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 #3407979: Cleanup DriverSpecificTransactionTestBase.

Note to self: when this is unblocked, do #3386263-10: [ignore] testing issue first thing.

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

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.

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

Thanks @daffie!

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.

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 (for example, ::commitOrRelease()), or we could have a simple match construct in calling code like
  

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

EDIT - added proposed rename for ::yield()

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.

On it.

Updated IS with #44

Done

Updated CR with the latest approach.

Please consider crediting contributors to #1025314: Transactions should be allowed to be committed explicitly, that will become outdated when this is committed.

fwiw, the #3406985: Convert all transactions in core to use explicit ::commitOrRelease() follow-up MR is built on top of this MR, and passes all tests.

@alexpott this issue did, but #3406985: Convert all transactions in core to use explicit ::commitOrRelease() is still wip. So the edit of https://www.drupal.org/node/3524461 is incorrect, actually. Not reverting it since it’s still a draft.

Changed https://www.drupal.org/node/3512006 which indeed was published pointing to 11.3.0 whereas it should refer to 11.2.0

#64 you're right. It's not in 11.2.x, it was only committed to 11.x (yes, at the time when 11.2.0 was still in RC, but not to its branch). So it will be GA only when 11.3.0 will be released.

Changed again the CR.

Thanks!