Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
The code managing database transaction is very complex - part of it sits in the Connection class, part of it in the Transaction class, and there's back and forth of calls between the two objects.
This makes hard to implement transaction management in contrib database drivers if they need to deviate from the core implementation. Also when looking at #3348590: Add transaction-related events to the Database API, it's complicated to find the right places to dispatch events.
Proposed resolution
Disentangle the transaction management code, introducing a transaction manager that does all the client operations and deals with the calls stack; reduce code in Connection and Transaction to a minimum i.e. calls to the manager's methods.
In the course of that, fix #3074033: Add a test for Connection::addRootTransactionEndCallback
Deprecate something (???)
Remaining tasks
Review by committers if something we want.
Identify if anything needs to be deprecated.
Change Record
Final Review
User interface changes
NA
API changes
This methods wil be deprecated:
Drupal\Core\Database\Connection::transactionDepth()
Drupal\Core\Database\Connection::rollBack()
Drupal\Core\Database\Connection::pushTransaction()
Drupal\Core\Database\Connection::addPostTransactionCallback()
Drupal\Core\Database\Connection::commit()
New methods:
Drupal\Core\Database\Connection::transactionManager()->stackDepth();
Drupal\Core\Database\Connection::transactionManager()->rollback();
Drupal\Core\Database\Connection::transactionManager()->startTransaction();
Drupal\Core\Database\Connection::transactionManager()->addPostTransactionCallback()
Data model changes
None
Release notes snippet
TBD
Issue fork drupal-3364706
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3364706-refactor-transactions
changes, plain diff MR !4101
Comments
Comment #3
mondrakeThe MR introduces the TransactionManagerInterface and its implementation for SQLite. MySql and PostgreSQL will come later.
Comment #4
mondrakeApart 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?’
Comment #5
andypostQuickly 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
Comment #6
catchComment #7
mondrakeI 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.
Comment #8
mondrakeIn 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.
Comment #9
smustgrave commentedLets get into committer eyes.
Updated IS with some remaining tasks though.
Comment #10
daffie commentedThis 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?
Comment #11
mondrakeThanks for your review @daffie! Comments in the MR.
Comment #12
mondrakeAs part of this issue, I would suggest to deprecate:
Connection::transactionDepth()- it's an internal detail, should not be exposed IMHOConnection::rollback()- likewise::commit()that was removed from the Connection, also::rollback()should be called from the transaction objectConnection::addRootTransactionEndCallback()- it's called rarely, better call the method on the new transacgtion manager insteadConnection::commit()- it's there just to throw an exception, probably cruft from an earlier deprecationConnection::doCommit()- becomes unused when the TransactionManager is in placeConnection::popCommittableTransactions()- becomes unused when the TransactionManager is in placeConnection::popTransaction()- becomes unused when the TransactionManager is in placeConnection::pushTransaction()- becomes unused when the TransactionManager is in placeComment #13
mondrakeRebased.
Comment #14
daffie commentedThe MR looks good to me.
I opened a couple of threads on the MR.
Comment #15
mondrakeThanks @daffie. What would you think of #12?
Comment #16
daffie commented@mondrake: doing comment #12 is fine by me.
Comment #17
mondrakeComment #18
mondrakeFiled #3378528: Allow arbitrary data types for PHPStan compatibility in @return class methods in Coder issue queue for PHPCS failure in last test run.
Comment #19
mondrakeThanks @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 change. 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.
Comment #20
daffie commentedAll 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.
Comment #21
yustintr commentedI'm going to work on the change record.
Comment #22
yustintr commentedI created the change records and updated the issues summary.
Comment #23
mondrakeRebased, adjusted references to CR, added PHPStan ignores for BC deprecated method calls.
Comment #24
daffie commentedAll 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.
Comment #25
catchOK 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.
Comment #26
mondrakeReplied inline to @andypost's comment, to the best of my knowledge. Back to RTBC.
Comment #27
andypostThank you! RTBC++
but method return value should be
?objecthttps://git.drupalcode.org/project/drupal/-/merge_requests/4101#note_204242Comment #28
mondrakeTried 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.
Comment #30
catchI 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!
Comment #32
mondrakeA couple of nasty edge cases requiring follow-up, filed #3384960: Strengthen TransactionManager. Hopefully we still have time for some fixes as this is not released yet.
Comment #33
mondrakeAdded some more follow-ups: