Refactor transactions

The MR introduces the TransactionManagerInterface and its implementation for SQLite. MySql and PostgreSQL will come later.

Apart from some loony random (?) test failures on pgsql, this is now at least ready for a first ‘architectural’ review.

Please leave CR, typo, deprecation related comments to a later stage, and help answer the question ‘does this make sense doing?’

Quickly skimmed and only question about exposed client connection

Also some naming questions about using "Client" in method names, it probably supposed all to be client related

I reckon there’s an error in the pgsql conversion that could explain (but not justify) the test failures. I will try and fix it. Leaving NR as the architecture review can happen independently from the pgsql implementation.

In 7d0e3196, reverted the changes to pgsql database operation classes that were forcing to open a transaction always and not just savepoints when a transaction is open already. Per se that should theoretically not be a problem, but it may explain the apparently random pgsql test failures.

Lets get into committer eyes.

Updated IS with some remaining tasks though.

This is not ready to be committed. Therefor I am putting it back to NW.

@mondrake: I like the basic idea! Great work. The main question that I have is this a real stack of ordered item or a random list of transactions?

Thanks for your review @daffie! Comments in the MR.

As part of this issue, I would suggest to deprecate:

1. Connection::transactionDepth() - it's an internal detail, should not be exposed IMHO
2. Connection::rollback() - likewise ::commit() that was removed from the Connection, also ::rollback() should be called from the transaction object
3. Connection::addRootTransactionEndCallback() - it's called rarely, better call the method on the new transacgtion manager instead
4. Connection::commit() - it's there just to throw an exception, probably cruft from an earlier deprecation
5. Connection::doCommit() - becomes unused when the TransactionManager is in place
6. Connection::popCommittableTransactions() - becomes unused when the TransactionManager is in place
7. Connection::popTransaction() - becomes unused when the TransactionManager is in place
8. Connection::pushTransaction() - becomes unused when the TransactionManager is in place

Rebased.

The MR looks good to me.
I opened a couple of threads on the MR.

Thanks @daffie. What would you think of #12?

@mondrake: doing comment #12 is fine by me.

Filed ✨ Allow arbitrary data types for PHPStan compatibility in @return class methods Active in Coder issue queue for the last PHPCS failure.

Thanks @daffie. Wrt tests, the refactor is done in a way that the existing tests are untouched, and they should cover already the cases you mention. In fact the API to transaction consumers is unchanged, it's the internals within the Database code that changes. I added a couple of tests that were clearly missing, the others should suffice. Please indicate specific use-cases that need test coverage, in case.

All my points have been addressed.
All code changes look good to me.
The IS needs an update.
We need to add a CR.
Back to needs work for those 2 points.

@mondrake: Great work.

I'm going to work on the change record.

I created the change records and updated the issues summary.

Rebased, adjusted references to CR, added PHPStan ignores for BC deprecated method calls.

All code changes look good to me.
The IS and the CR are in order.
The testbot is green.
The issue is ready for a review by backend framework manager.

OK I hadn't looked at the database transaction code for a long time, and it's hard to follow. This issue makes it a lot better, it'll be even better when we're able to remove the bc layers. I made some UI commits to the MR to fix the deprecation version from 10.0.2 to 10.2.0 for some things.

I wanted to commit this but there is an unresolved question from @andypost on the MR https://git.drupalcode.org/project/drupal/-/merge_requests/4101/diffs#3c...

Happy to commit once that's answered/resolved. There might be more improvements we can make once the bc layers are resolved like making some more methods protected to reduce the API surface, but this is all only called by database drivers anyway.

Replied inline to @andypost's comment, to the best of my knowledge. Back to RTBC.

Thank you! RTBC++

but method return value should be ?object https://git.drupalcode.org/project/drupal/-/merge_requests/4101#note_204242

Tried anticipating the implementation in the mysqli driver repo on GitHub, https://github.com/mondrake/mysqli/commit/fd902b931a1ee9ea4f9a16f34d827a...

Few things to fix, but apart from that the implementation is much more clear than if we were to use current HEAD.

I think if the property is always set in the constructor it's OK to prevent it from being null.

Rest looks good to me, as above we can improve further once it's in and especially once the bc layers are removed.

Committed/pushed to 11.x, thanks!

mondrake → closed merge request !4101

A couple of nasty edge cases requiring follow-up, filed 🐛 Strengthen TransactionManager Needs work . Hopefully we still have time for some fixes as this is not released yet.

Added some more follow-ups:

• 🐛 [PP-1] Commiting a transaction while there are still active savepoints leaves the stack in inconsistent state Postponed
• 🐛 [PP-1] Rolling back to a savepoint should leave the savepoint in the transaction stack Postponed
• ✨ [PP-1] Introduce a Connection::executeDdlStatement method Active

Automatically closed - issue fixed for 2 weeks with no activity.