Replace \Drupal\Core\Database\Connection::destroy() with a proper destructor

Added something extra to the test to see if it works.

@alexpott In the patch you are adding a call to gc_collect_cycles(). Should we not test that we have done so. When you remove the call to gc_collect_cycles() in Drupal\Core\Database\Database::closeConnection() the test will fail.

1. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -405,26 +405,15 @@ public static function closeConnection($target = NULL, $key = NULL) {
   +    gc_collect_cycles();
   

   Is there any possibility that calling the function gc_collect_cycles() has a negative side effect for something else in Drupal? Personally I do not see any, but I think it is a question that should be asked.

2. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -405,26 +405,15 @@ public static function closeConnection($target = NULL, $key = NULL) {
   +    // Force garbage collection of PDO connection objects so the connections
   +    // are really closed.
   

   Can we change this to: "Force the garbage collection to run. In that way PDO connection objects are really closed"

3. In Drupal\Core\Database\Connection we also have the class variable $logger with a link to Drupal\Core\Database\Log. Should that also not be set to NULL in __destruct()?

4. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -262,6 +265,24 @@ public function destroy() {
   +      $this->connection = NULL;
   

   Should $this->connection not always be set to NULL, instead of now only when the if-statement is true?

5. Should we not add the code parent::__destruct() to \Drupal\Core\Database\Connection::__destruct(). According to the documentation on php.net: "Like constructors, parent destructors will not be called implicitly by the engine. In order to run a parent destructor, one would have to explicitly call parent::__destruct() in the destructor body." What does the default implementation of stdClass::__destruct() do?

6. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -251,8 +251,11 @@ public static function open(array &$connection_options = []) {}
   +   * @deprecated @todo
   

   Fix the @todo.

Will do a full review after the testbot returns green.

+++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
@@ -319,6 +319,8 @@ public function testSchema($expected, $driver, $namespace) {
+   * @expectedDeprecation @todo

Nitpick: Fix the @todo.

#25 a PDOStatement object left open after a PDO::prepare, somewhere, maybe?

Lovely! Let's make objects do OOP work! LGTM

Nit:

+++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
@@ -336,6 +339,29 @@ public function testDestroy() {
+    $mock_pdo = $this->createMock('Drupal\Tests\Core\Database\Stub\StubPDO');
+    $fake_connection = new class($mock_pdo, ['namespace' => 'Drupal\\Tests\\Core\\Database\\Stub\\Driver']) extends StubConnection {

maybe we can use StubPDO::class instead of FQCN here?

I half-remember from very early D8 development that we had a reason for avoiding using __destruct(), but I don't remember what it was. The only clue I found was in core/lib/Drupal/Core/Lock/DatabaseLockBackend.php which alludes to a problem with garbage collection, so maybe it is not relevant anymore on more modern PHP versions? We have very few destructors in core, but more than 0.

Also dredged up #1891980: Add Service termination API to reliable terminate/shut down services in the container from less early D8 development.

From #1811730-14: Refactor Database to rely on the DIC solely:

Note that adding a __destruct() to Connection can not replace destroy(), the purpose of destroy() is to remove circular references so that the connection actually can be destructed, it needs to be called manually.

I also want a "Needs Sascha review" tag.

> #1811730: Refactor Database to rely on the DIC solely

Oh, the dark and sad memories.

Has been a very long time since I worked on that. I doubt that newer PHP versions changed what I said back then, but the problems there might have been specific to when the connection is in the container And I can see that there are some workarounds to solve circular references in the patch now, so maybe @alexpott just figured out what I couldn't back then.

+++ b/core/tests/Drupal/KernelTests/Core/Database/NextIdTest.php
@@ -38,4 +40,37 @@ public function testDbNextId() {
+    // Test that \Drupal\Core\Database\Driver\mysql\Connection::__destruct()
+    // successfully trims the sequences table if the connection is closed.
+    $count = $this->connection->select('sequences')->countQuery()->execute()->fetchField();
+    $this->assertEquals(1, $count);
+  }

countQuery() is quite inefficient as it does a SELECT COUNT(*) from (inner query), directly doing a "$query->addExpression('COUNT(*)')" is more efficient, but doesn't really matter here and for this query. (countQuery() is built to be reliable with complex queries, not fast)

Don't we have a soft requirement to put more than 2 chained calls on separate lines though?

The change record should mention custom database drivers need to call parent::__destruct now (? or is that only for D10 and not doing so is merely deprecated for now? needs to clarify this).

Better status.

I cannot understand what is still open here, sorry. This patch improves DX to orderly shutdown db connections, which otherwise now requires balancing what needs to be done in destroy() and what in __destruct(), and that feels fragile and obscure. Let’s kill destroy that smells like procedural legacy.

Edit: see also the problems that this generated in #3126563: Replace usage of assertAttributeEquals() that is deprecated

I drafted the CR, please review.

Also, tested this patch with the DruDbal experimental driver. PR #179 is passing - that applies this patch and a cleanup of the driver to fix HEAD there that is failing since #3128880: Make ConnectionUnitTest also run for PostgreSQL.

Thanks, it is very clear to me. Perhaps change the title "\Drupal\Core\Database\Connection::destroy has been replaced by __destruct()". Most change record titles I can remember describe what happened. Also note even if we don't change to that, destruct has a typo in the title.

1. +++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
   @@ -336,6 +339,29 @@ public function testDestroy() {
   +    $fake_connection = NULL;
   

   The test fails when I change the line to: $fake_connection->__destruct();

   The error that I get is:

   Drupal\Tests\Core\Database\ConnectionTest::testDestructBcLayer
   Error: Call to a member function setAttribute() on null
   
   /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Connection.php:275
   /var/www/drupalvm/drupal/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php:353
   /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Connection.php:286
   /var/www/drupalvm/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:691
   /var/www/drupalvm/drupal/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
   /var/www/drupalvm/drupal/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:621
   /var/www/drupalvm/drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:204
   /var/www/drupalvm/drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:163
   

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -258,15 +258,36 @@ public static function open(array &$connection_options = []) {}
   +    if ($backtrace[1]['class'] != self::class && $backtrace[1]['function'] !== '__destruct') {
   

   We have != and !== on the same line. Should we not use !== for both?

The nitpick from comment #28 is still unaddressed.

The BC layer looks good to me, only I am not a 100% convinced that it will work in all circumstances. I am very happy with the tag "Needs framework manager review".

@alexpott: Thank you for explanation for #41.1.

All remarks are addressed.
All code changes look good.
Changing the status to RTBC for the framework managers review.

Actually #42 doesn’t address #28, since another prexisting method is changed, not the one introduced by the patch itself. Superminor though...

Fixed the nitpick from comment #44.
Leaving the status to RTBC, because the change is very small. Feel free to change the status when you do not agree with me.

Straight reroll.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -258,15 +258,36 @@ public static function open(array &$connection_options = []) {}
+
+  /**
+   * Ensures that the PDO connection can be garbage collected.
+   */
+  public function __destruct() {
+    // Call the ::destroy method to provide a BC layer.
+    $this->destroy();
+    // Ensure that the circular reference caused by Connection::__construct()
+    // using $this in the call to set the statement class can be garbage
+    // collected.
+    $this->connection = NULL;
   }
 

I think we need a @todo on ::__destruct() to say exactly what needs to happen to it in 10.0.x. It's not clear to me reading the patch.

OK the reason I asked is because the logic in $this->destroy() is different to the logic in __destruct(), so it wasn't clear whether it needed to be copied over prior to removal or not.

This is the kind of explanation I was hoping we could add to the comment.

+++ b/core/lib/Drupal/Core/Update/UpdateCompilerPass.php
@@ -27,28 +27,16 @@ public function process(ContainerBuilder $container) {
-
-        // Ensure all the method call arguments are valid.
-        foreach ($definition->getMethodCalls() as $call) {
-          foreach ($call[1] as $argument) {
-            if ($this->isArgumentMissingService($argument, $container)) {
+          if ($argument instanceof Reference) {
+            $argument_id = (string) $argument;
+            if (!$container->has($argument_id) && $argument->getInvalidBehavior() === ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE) {
+              // If the container does not have the argument and would throw an
+              // exception, remove the service.
               $container->removeDefinition($key);
-              $container->log($this, sprintf('Removed service "%s"; reason: method call "%s" depends on non-existent service "%s".', $key, $call[0], (string) $argument));
+              $container->log($this, sprintf('Removed service "%s"; reason: depends on non-existent service "%s".', $key, $argument_id));
               $has_changed = TRUE;
               $process_aliases = TRUE;
-              // Process the next definition.
-              continue 3;
             }
           }
         }

This is from a different patch.

Committed 2ee59be and pushed to 9.1.x. Thanks!

Publish CR