Deprecate Connection::supportsTransactions()

I am +1 here. Let's simplify the code and eliminate rarely if ever used code paths.

Almost 5 years after, we have an orderly manner to proceed here, i.e. deprecate for removal.

Let's see what a first deprecation patch yields to.

With removals of calls to ::supportsTransactions(). Mainly leftovers in tests.

Oops

Updated IS with what current patch proposes.

1. +++ b/core/tests/Drupal/KernelTests/Core/Database/InvalidDataTest.php
   @@ -43,16 +42,7 @@ public function testInsertDuplicateData() {
          if ($name == 'Elvis') {
   ...
   +        $this->fail('The whole transaction is rolled back when a duplicate key insert occurs.');
          }
          else {
            $this->pass('The whole transaction is rolled back when a duplicate key insert occurs.');
   

   This looks like it can just become an assertNotEquals()?

2. +++ b/core/tests/Drupal/KernelTests/Core/Database/InvalidDataTest.php
   @@ -120,16 +110,7 @@ public function testInsertDuplicateDataFromSelect() {
          if ($name == 'Frank') {
   ...
   +        $this->fail('The whole transaction is rolled back when a duplicate key insert occurs.');
          }
          else {
            $this->pass('The whole transaction is rolled back when a duplicate key insert occurs.');
   

   Same here

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1381,9 +1371,15 @@ public function clientVersion() {
   +   * @see xxx
   

   Needs link to change record.

2. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -334,21 +334,6 @@ class Connection extends DatabaseConnection {
   -    // MySQL never supports transactional DDL.
   -    $this->transactionalDDLSupport = FALSE;
   
   +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -71,21 +71,17 @@ class Connection extends DatabaseConnection {
   -    // Transactional DDL is always available in PostgreSQL,
   -    // but we'll only enable it if standard transactions are.
   -    $this->transactionalDDLSupport = $this->transactionSupport;
   
   +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -65,11 +70,6 @@ public function __construct(\PDO $connection, array $connection_options) {
   -    // This driver defaults to transaction support, except if explicitly passed FALSE.
   -    $this->transactionSupport = $this->transactionalDDLSupport = !isset($connection_options['transactions']) || $connection_options['transactions'] !== FALSE;
   

   Are we also deprecation this?

3. +++ b/core/modules/node/tests/src/Functional/NodeCreationTest.php
   @@ -289,15 +278,4 @@ protected static function getWatchdogIdsForTestExceptionRollback() {
   -    return Database::getConnection()->query("SELECT wid FROM {watchdog} WHERE message LIKE 'Explicit rollback failed%'")->fetchAll();
   

   Somewhere in core is a record added to the watchdog table if transactions are not supported. Should that code also be removed?

Thanks for reviews!

#20 actually that entire test class deserves some phpunit love. Done here.

#21:

1. Sure, but 9.1.0 is a bit away, so we can come back on that when closer.
2. No, but that is no longer dependent on the 'transactions' setting of the connection options, rather inherent to the db, so just moved that to an override of the variable. The abstract implementation is set to FALSE, so no purpose to override in MySQL, wheras for PgSQL and SQLite we set straight to TRUE.
3. I cannot find where that code is.

Adding a related issue which is kinda orthogonal to this - I believe that one should be won't fix'ed but ATM still open for any comment.

Re: #21.3 it looks like this was removed from core in 2010! Maybe @Berdir remembers deleting this code almost 10 years ago in #711108-42: Richer exception reporting for watchdog()? ;)

https://git.drupalcode.org/project/drupal/commit/72db95c988c57d9ac1b073e...

Rerolled. Next, adding reference to a CR in the deprecation errors. There's not a clear CR for dropping support of non-transactional DBs, but there is a clear issue: #2275535: [policy, no patch] Drop support for non-transactional database engines (including MyISAM), that has https://www.drupal.org/node/2278745 as a CR, so will reference the latter.

Added link to CR, both in the issue and the patch.

Fixes to deprecation message and CS.

All the calls to the method supportsTransactions() have been removed.
Deprecation triggers have been added.
There is testing for he deprecation triggers.
All my reviewing points have been addressed.
The testbot is happy.
All code changes look good.
The change record is in order.
For me it is RTBC.

I have got the ok from @alexpott to deprecate hook_query_alter, not hook_query_TAG_alter. I have not made an issue for it. If somebody is interested, then go for it. Add an link and I will review your patch.

Thanks @daffie

+++ b/core/tests/Drupal/KernelTests/Core/Database/InvalidDataTest.php
@@ -17,55 +16,36 @@ class InvalidDataTest extends DatabaseTestBase {
-      else {
-        $this->pass('The whole transaction is rolled back when a duplicate key insert occurs.');
-      }
+    $this->expectException(IntegrityConstraintViolationException::class);
+    $this->connection->insert('test')
+      ->fields(['name', 'age', 'job'])
+      ->values([
+        'name' => 'Elvis',
+        'age' => 63,
+        'job' => 'Singer',
+      ])
+      ->values([
+        // Duplicate value 'John' on unique field 'name'.
+        'name' => 'John',
+        'age' => 17,
+        'job' => 'Consultant',
+      ])
+      ->values([
+        'name' => 'Frank',
+        'age' => 75,
+        'job' => 'Singer',
+      ])
+      ->execute();
 
-      // Ensure the other values were not inserted.
-      $record = $this->connection->select('test')
-        ->fields('test', ['name', 'age'])
-        ->condition('age', [17, 75], 'IN')
-        ->execute()->fetchObject();
+    // Check if the first record was inserted.
+    $this->assertSame('Elvis', $this->connection->query('SELECT name FROM {test} WHERE age = :age', [':age' => 63])->fetchField());
 

I learned https://stackoverflow.com/questions/14561908/phpunit-doesnt-continue-tes... that the test code after the exception will not be executed, so this needs to be reworked.

Good, the test refactoring was wrong. This should be better now.

+++ b/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php
@@ -117,6 +118,22 @@ public function testConnectionOptions() {
+  /**
+   * Tests the deprecation of the 'transactions' connection option.
+   *
+   * @group legacy
+   * @expectedDeprecation Passing a 'transactions' connection option to Drupal\Core\Database\Connection::__construct is deprecated in drupal:9.1.0 and is removed in drupal:10.0.0. All database drivers must support transactions. See https://www.drupal.org/node/2278745
+   * @expectedDeprecation Drupal\Core\Database\Connection::supportsTransactions is deprecated in drupal:9.1.0 and is removed in drupal:10.0.0. All database drivers must support transactions. See https://www.drupal.org/node/2278745
+   */
+  public function testTransactionsOptionDeprecation() {
+    $connection_info = Database::getConnectionInfo('default');
+    $connection_info['default']['transactions'] = FALSE;
+    Database::addConnectionInfo('default', 'foo', $connection_info['default']);
+    $foo_connection = Database::getConnection('foo', 'default');
+    $this->assertInstanceOf(Connection::class, $foo_connection);
+    $this->assertTrue($foo_connection->supportsTransactions());
+  }

Do you have the same problem here? That the test code stops after the exception will not be run?

For @expectedDeprecation? No, that can be accumulated.

AFAICU in PHPUnit, when you call $this->expectException, then the runner will catch the thrown exception, assert it is thrown, then re-throw it into the test. If the test does not catch the re-throw, it just stops. But deprecation assertions work with listeners instead, it's a different pattern.

So, TIL it's the other way round, apparently - you need to catch the exception in the test, assert what you need in the catch block, then re-throw the exception for $this->expectException to deal with it.

The changes to the test Drupal\KernelTests\Core\Database\InvalidDataTest look good.
My question on the testing of multiple deprecation message in a single test are answered.
The patch was for me already RTBC.
Back to RTBC.

Committed a1d4b16 and pushed to 9.1.x. Thanks!

#2347867: Race conditions with lock/cache using non-database storage - add a non-transactional database connection was discussion a non-transactional database connection, but it mostly just needs a separate one from the main one, so I think this is good.