Convert all transactions in core to use explicit ::commitOrRelease()

It would be helpful to have a small summary on the why in the issue summary

Feedback appears to be addressed but CR is still tbd

Current test failures are deprecations for

1) /builds/issue/drupal-3406985/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:52
Database commit by letting a Transaction object go out of scope is deprecated in drupal:11.3.0 and is removed from drupal:13.0.0. Commit explicitly via Transaction::commitOrRelease() instead. See https://www.drupal.org/node/3524461

1) /builds/issue/drupal-3406985/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php:364
Database commit by letting a Transaction object go out of scope is deprecated in drupal:11.3.0 and is removed from drupal:13.0.0. Commit explicitly via Transaction::commitOrRelease() instead. See https://www.drupal.org/node/3524461

Uploaded as a text file after parsing through Functional and FunctionalJavaScript test results. Summarized as

- locale
- config_translation
- AssetOptimizationUmamiTest
- help
- demo_umami
- core Installer tests

I added some manual testing scenarios for xdebug 3.3+ in develop mode.

With regards to #20 any suggestion on how to test that?

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

1. Maybe consider adding a few words to the CR as to why? "Drupal by default was committing a database transaction when its Transaction object goes out of scope, during the Transaction object destruction." and then add something to the tunes of "This causes problems with unpredictable object destruction order especially under xdebug 3.3+ in develop mode" because at first glance it's not at all understandable why such a boilerplate-y thing needs to be added -- most code doesn't want to return its transaction, destruction seems to be the right time to do it.
2. David nuked direct commit in #301049: Transaction nesting not tracked by connection were those problems considered? (DBTNG is mostly Crell and me which is the only reason I comment here -- but the transaction code is David's. Back then we didn't have the experience necessary for that, he did.)

Are there an easily reproducible steps with XDebug enabled in develop mode to test this? I've read through the examples in the IS, as well as comments from #3405976: Transaction autocommit during shutdown relies on unreliable object destruction order (xdebug 3.3+ enabled) and #3451460: Sessions are invalidated on subsequent page request when written while transaction is open, but reproducing seems to be non-trivial or requires contrib or custom code that's not specified.

Meanwhile, the references to 11.3.0 in the deprecation messages need updating to 11.4.0, and the version set in the CR needs updating as well.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

I've found some test steps with consistently repeatable failures that the changes in the MR here do not solve. It seems related, but not I'm not completely sure. I can file a separate issue if appropriate.

Local env:
DDEV v1.24.10
Using ddev-core-dev add-on: https://github.com/justafish/ddev-drupal-core-dev
DB version SQLite 3.45.1
Xdebug v3.5.0
xdebug.mode = develop
PHP 8.5.2

Steps:
Checkout MR branch
Install Drupal core with standard: ddev drush si standard -y
Uninstall Toolbar ddev drush pmu toolbar
Install Navigation ddev drush en navigation -y
Enable Xdebug ddev xdebug
Rebuild cache ddev drush cr
Observe exception:

Failed to log error: TypeError: Drupal\Core\Database\StatementPrefetchIterator::__construct(): Argument #1 ($clientConnection) must be of type object, null given, called in /var/www/html/core/modules/sqlite/src/Driver/Database/sqlite/Connection.php on line 429 in Drupal\Core\Database\StatementPrefetchIterator->__construct() (line 36 of /var/www/html/core/lib/Drupal/Core/Database/StatementPrefetchIterator.php). #0 /var/www/html/core/modules/sqlite/src/Driver/Database/sqlite/Connection.php(429): Drupal\Core\Database\StatementPrefetchIterator->__construct()
#1 /var/www/html/core/lib/Drupal/Core/Database/Connection.php(667): Drupal\sqlite\Driver\Database\sqlite\Connection->prepareStatement()
#2 /var/www/html/core/lib/Drupal/Core/Database/Query/Select.php(521): Drupal\Core\Database\Connection->query()
#3 /var/www/html/core/lib/Drupal/Core/Database/Query/Merge.php(366): Drupal\Core\Database\Query\Select->execute()
#4 /var/www/html/core/lib/Drupal/Core/Cache/DatabaseCacheTagsChecksum.php(42): Drupal\Core\Database\Query\Merge->execute()
#5 /var/www/html/core/lib/Drupal/Core/Cache/CacheTagsChecksumTrait.php(50): Drupal\Core\Cache\DatabaseCacheTagsChecksum->doInvalidateTags()
#6 [internal function]: Drupal\Core\Cache\DatabaseCacheTagsChecksum->rootTransactionEndCallback()
#7 /var/www/html/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php(580): call_user_func()
#8 /var/www/html/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php(329): Drupal\Core\Database\Transaction\TransactionManagerBase->processPostTransactionCallbacks()
#9 /var/www/html/core/lib/Drupal/Core/Database/Transaction.php(37): Drupal\Core\Database\Transaction\TransactionManagerBase->purge()
#10 [internal function]: Drupal\Core\Database\Transaction->__destruct()
#11 {main}
TypeError: Drupal\Core\Database\StatementPrefetchIterator::__construct(): Argument #1 ($clientConnection) must be of type object, null given, called in /var/www/html/core/modules/sqlite/src/Driver/Database/sqlite/Connection.php on line 429 in /var/www/html/core/lib/Drupal/Core/Database/StatementPrefetchIterator.php on line 36 #0 /var/www/html/core/modules/sqlite/src/Driver/Database/sqlite/Connection.php(429): Drupal\Core\Database\StatementPrefetchIterator->__construct()
#1 /var/www/html/core/lib/Drupal/Core/Database/Connection.php(667): Drupal\sqlite\Driver\Database\sqlite\Connection->prepareStatement()
#2 /var/www/html/core/lib/Drupal/Core/Database/Query/Select.php(521): Drupal\Core\Database\Connection->query()
#3 /var/www/html/core/lib/Drupal/Core/Database/Query/Merge.php(366): Drupal\Core\Database\Query\Select->execute()
#4 /var/www/html/core/lib/Drupal/Core/Cache/DatabaseCacheTagsChecksum.php(42): Drupal\Core\Database\Query\Merge->execute()
#5 /var/www/html/core/lib/Drupal/Core/Cache/CacheTagsChecksumTrait.php(50): Drupal\Core\Cache\DatabaseCacheTagsChecksum->doInvalidateTags()
#6 [internal function]: Drupal\Core\Cache\DatabaseCacheTagsChecksum->rootTransactionEndCallback()
#7 /var/www/html/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php(580): call_user_func()
#8 /var/www/html/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php(329): Drupal\Core\Database\Transaction\TransactionManagerBase->processPostTransactionCallbacks()
#9 /var/www/html/core/lib/Drupal/Core/Database/Transaction.php(37): Drupal\Core\Database\Transaction\TransactionManagerBase->purge()
#10 [internal function]: Drupal\Core\Database\Transaction->__destruct()
#11 {main}
TypeError: Drupal\Core\Database\StatementPrefetchIterator::__construct(): Argument #1 ($clientConnection) must be of type object, null given, called in /var/www/html/core/modules/sqlite/src/Driver/Database/sqlite/Connection.php on line 429 in Drupal\Core\Database\StatementPrefetchIterator->__construct() (line 36 of /var/www/html/core/lib/Drupal/Core/Database/StatementPrefetchIterator.php).

Note that if Xdebug is not enabled, or Xdebug mode is "debug" or if Navigation is uninstalled, the exception is not thrown. I don't know if this is SQLite specific, because I haven't tested on MySQL or Mariadb.

I would like to help get this to RTBC, but I don't think I understand the DB layer well enough to do so without having repeatable test steps that show what this solves.

I set the primary issue for the change record draft to the meta for now. I don’t think we should publish the change record until after the deprecation issue, right?

I don't have a deep understanding of the DB layer, but the MR seems straightforward from the explanation/goals defined in the IS. I do have a couple general questions and one small punctuation nit.

Thanks for the explanation. This makes sense to me.

I see that no one replied to @ghost of drupal past's question from #33, so I've read #301049: Transaction nesting not tracked by connection, and, as far as I can see from the patch that was committed there, the primary goal of that issue was transaction nesting, which is fully preserved by commitOrRelease() (it works through the same stack mechanism). The "prohibition" of explicit commits in that patch was a safeguard against bypassing the nesting layer via raw PDO::commit(), not a design goal in itself.

The new mechanism for explicit commits were discussed and agreed upon in #1025314: Transactions should be allowed to be committed explicitly and especially #3398767: Allow returning explicitly to the prior nesting level in transactions (aka allow explicit COMMIT in Transaction objects), where commitOrRelease() was introduced.

I've read this MR a few times already in the past few weeks, and the smaller scope is appreciated :)

Committed and pushed 8d78571bbef to main and 3c48dc8cccc to 11.x. Thanks!

Note that I haven't published the change record, I think that should be done in #3584238: Deprecate implicit commit-on-destruct.