[PHP8] Error: User-supplied statement does not accept constructor arguments in PDO->prepare()

\ConnectionUnitTest::testOpenClose and others in the same test class fail when they do assertNoConnection($id).

Looks like that's because garbage collection doesn't destroy the connection properly unless we ensure that the connection is null.

D9 does that here: https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/lib/Drupal/C...

...we could copy the same (add it to \DatabaseConnection::__destruct()) or we could add it to \DatabaseConnection::destroy().

The latter would be my preference:

  public function destroy() {
    // Destroy all references to this connection by setting them to NULL.
    // The Statement class attribute only accepts a new value that presents a
    // proper callable, so we reset it to PDOStatement.
    if (!empty($this->statementClass)) {
      $this->connection->setAttribute(PDO::ATTR_STATEMENT_CLASS, array('PDOStatement', array()));
    }
    $this->schema = NULL;
+   $this->connection = NULL;
  }

However, you could argue it'd match D9 more closely to do the former. I don't think there's much difference either way, but the comments etc.. match nicely in destroy() without having to add more explanation of what's going on.

Actually doesn't look like it makes sense to set the connection to NULL there straight after calling setAttribute() on the connection?

In the particular case of the failing unit tests, I don't think it makes much difference but perhaps we should stick to copying D9 and add it to \DatabaseConnection::__destruct().

Looks like gitlab can't rebase the MR itself.

Is this the new equivalent of a "needs reroll" then?

I also don't see the fix outlined in #3/#4 in the MR?

Hmmz.

Just to confirm that I get the same result moving this to __destruct() instead (like D9, per #4):

  /**
   * Ensures that the PDO connection can be garbage collected.
   */
  public function __destruct() {
    // Call the ::destroy method to provide a BC layer.
    $this->destroy();
+    $this->connection = NULL;
  }

...which isn't really surprising.

CommentActionsTestCase seems to be a convenient test to reproduce the issue.

This is looking somewhat tricky to me.

The typical error is:

PHP Fatal error:  Uncaught Error: Call to a member function prepare() on null in /path/to/drupal-7.x/includes/database/database.inc:553
Stack trace:
#0 /path/to/drupal-7.x/includes/database/database.inc(752): DatabaseConnection->prepareQuery()
#1 /path/to/drupal-7.x/includes/database/mysql/query.inc(36): DatabaseConnection->query()
#2 /path/to/drupal-7.x/includes/lock.inc(134): InsertQuery_mysql->execute()
#3 /path/to/drupal-7.x/includes/theme.inc(449): lock_acquire()
#4 /path/to/drupal-7.x/includes/bootstrap.inc(461): ThemeRegistry->set()
#5 [internal function]: DrupalCacheArray->__destruct()
#6 {main}
  thrown in /path/to/drupal-7.x/includes/database/database.inc on line 553

This error goes away if we don't set the PDO connection to null explicitly in either \DatabaseConnection::__destruct() or \DatabaseConnection::destroy().

However, if we don't do $this->connection = NULL; (which D9 does in the connection's __destruct()) the PDO connection will not be garbage collected and tests such as \ConnectionUnitTest::testOpenClose() will fail.

In addition to failing tests, there's presumably some risk of connections piling up in real use - see comments such as:

   * PHP does not destruct an object if it is still referenced in other
   * variables. In case of PDO database connection objects, PHP only closes the
   * connection when the PDO object is destructed, so any references to this
   * object may cause the number of maximum allowed connections to be exceeded.

We can't catch the PHP Fatal error: Uncaught Error: Call to a member function ... on null in older versions of PHP at least so something like this won't work (in \DrupalCacheArray::__destruct()):

    if (!empty($data)) {
      try {
        $this->set($data);
      }
      catch (Exception $e) {
        // The database connection may be gone before this fires.
      }
    }

...there seems to be some precedent for doing something like that e.g. in ViewDataCache's destructor before #1891980: Add Service termination API to reliable terminate/shut down services in the container was committed, but it doesn't look like it'd help here.

We probably don't want to introduce checking for the presence of the database connection in the calling code; the error's actually happening when lock_acquire() tries to access the semaphore table before doing a cache_set() but both the lock and cache systems are "pluggable" and may have nothing to do with the database in many cases.

No solution yet, but wanted to scribble down some notes after looking into this a bit.

Sorry to go back to a patch, but I'm still getting familiar the MR workflow...

I thought about a shutdown function, and that might indeed be useful.

Another way to avoid the error is something like this patch where we tweak things to make the exception catchable.

I'm not certain it's a great idea, but want to test with the different PHP versions.

If we went down this road, we'd probably want to do something like proxy the PDO object so that we can check it's actually still there before calling any of its methods - here we're just doing that specifically for prepare.

I'm not entirely sure what the consequences of the call to set($data) failing in \DrupalCacheArray::__destructmight be. If we're effectively accepting data loss that's obviously not good.

Giving this approach a quick try anyway..

Noting that I think we avoid the original "Error: User-supplied statement does not accept constructor arguments in PDO->prepare()" issue if we just don't set this attribute (in destroy()):

    if (!empty($this->statementClass)) {
      //$this->connection->setAttribute(PDO::ATTR_STATEMENT_CLASS, array('PDOStatement', array()));
    }

With that commented out e.g. CommentActionsTestCase will pass without the problem triggered by the DrupalCacheArray destructor.

However, it looks like other tests will then fail - e.g.:

Connection unit tests 8 passes, 4 fails, and 0 exceptions

...which is probably because the connection's not being garbage collected properly (although I've not dug into the details there).

I am wondering what's the smallest possible change we can make to get PHP 8 passing without breaking PHP 5...?

...on the other hand it seems very broken that the DrupalCacheArray destructor is apparently writing to the db after the connection's been closed / destroyed (or at least trying to).

I'm not sure we can backport the D8/9 improvements for that without some really drastic changes.

I wonder if it would matter if e.g. the ThemeRegistry is not written back to (database) cache if that happens after the db connection has been torn down.

There are other places where we seem to shrug this sort of thing off e.g. in \DrupalDatabaseCache::set there is:

    try {
      db_merge($this->bin)
        ->key(array('cid' => $cid))
        ->fields($fields)
        ->execute();
    }
    catch (Exception $e) {
      // The database may not be available, so we'll ignore cache_set requests.
    }

So perhaps something like #11 might be viable?

FWIW, with the latest patch from #3204161: [D7] MySQL on PHP 8 now errors when committing or rolling back when there is no active transaction all Database tests pass for me apart from these 2:

Tests to be run:
 - Sequences API (DatabaseNextIdCase)
 - Select tests, complex 2 (DatabaseSelectComplexTestCase2)

...which both blow up with messages like this:

<h1>Additional uncaught exception thrown while handling exception.</h1><h2>Original</h2><p>Error: User-supplied statement does not accept constructor arguments in PDO-&gt;prepare() (line 544 of /path/to/drupal-7.x/includes/database/database.inc).</p><h2>Additional</h2><p>PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table &amp;#039;drupal7x.simpletest167736semaphore&amp;#039; doesn&amp;#039;t exist: SELECT expire, value FROM {semaphore} WHERE name = :name; Array
(
    [:name] =&amp;gt; theme_registry:runtime:bartik:cache
)
 in lock_may_be_available() (line 167 of /path/to/drupal-7.x/includes/lock.inc).</p><hr /><h1>Uncaught exception thrown in shutdown function.</h1><p>PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table &amp;#039;drupal7x.simpletest167736semaphore&amp;#039; doesn&amp;#039;t exist: DELETE FROM {semaphore} 
WHERE  (value = :db_condition_placeholder_0) ; Array
(
    [:db_condition_placeholder_0] =&amp;gt; 17417034046060314d505095.15403622
)
 in lock_release_all() (line 269 of /path/to/drupal-7.x/includes/lock.inc).</p><hr />

I suspect those two will pass when we fix this issue.

Cautiously optimistic that avoiding messing with the PDO statement class in \DatabaseConnection::destroy() avoids the problem we're trying to address here, without breaking garbage collection / closing connections for PHP 8.

I don't think the Database tests will pass 100% without #3204161: [D7] MySQL on PHP 8 now errors when committing or rolling back when there is no active transaction but I think it may only be the transaction tests from that issue that are still a problem.

Nope, I still had the try/catch in DrupalCacheArray's destructor from #11 when I ran tests locally.

This is still two steps forwards and one step back unfortunately.

Linking the issue which added the connection unit tests and a lot of the code we're looking at which allows the PDO connections to be closed.

Here is the plan how to fix:

- DrupalDatabaseCache gets the Database::connection() injected into `$this->connection`
- Add setConnetion / getConnection() methods, too
- DrupalCacheArray injects the cache object (so it does not get destroyed while it's still using it)
- We make a cache_get_object() method public that calls _cache_get_object() (private)
- We don't add __destroy as its a breaking change

Discussed this with @Fabianx who suggested a different approach, along the lines of using dependency injection to prevent the database connection object from being destructed too early.

We also agreed to try and keep the changes to a minimum in order e.g. not to break PHP 5 (which D8/9 didn't have to worry about) and also any contrib db drivers (note that earlier backport patches meant the driver classes had to call parent::__destruct();).

Here's a simple patch which I think makes the main changes we discussed. No interdiff as wouldn't be useful.

This seems to address the PHP8 problem with DrupalCacheArray's destructor trying to access the db connection after it's been torn down in the test I've been using to reproduce that.

However, we're now hitting different problems:

Drupal test run
---------------

Tests to be run:
 - Comment actions (CommentActionsTestCase)

Test summary
------------

PHP Fatal error:  Uncaught Error: User-supplied statement does not accept constructor arguments in /path/to/drupal-7.x/includes/database/database.inc:544
Stack trace:
#0 /path/to/drupal-7.x/includes/database/database.inc(544): PDO->prepare()
#1 /path/to/drupal-7.x/includes/database/database.inc(743): DatabaseConnection->prepareQuery()
#2 /path/to/drupal-7.x/includes/database/mysql/database.inc(511): DatabaseConnection->query()
#3 /path/to/drupal-7.x/includes/database/mysql/database.inc(456): DatabaseConnection_mysql->nextIdDelete()
#4 [internal function]: DatabaseConnection_mysql->__destruct()
#5 {main}
  thrown in /path/to/drupal-7.x/includes/database/database.inc on line 544

  ...
  
<h1>Uncaught exception thrown in shutdown function.</h1><p>PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table &amp;#039;drupal7x.simpletest946234cache_block&amp;#039; doesn&amp;#039;t exist: DELETE FROM {cache_block} 
WHERE  (expire &amp;lt;&amp;gt; :db_condition_placeholder_0) AND (expire &amp;lt; :db_condition_placeholder_1) ; Array
(
    [:db_condition_placeholder_0] =&amp;gt; 0
    [:db_condition_placeholder_1] =&amp;gt; 1617023850
)
 in cache_clear_all() (line 186 of /path/to/drupal-7.x/includes/cache.inc).</p><hr /><h1>Uncaught exception thrown in shutdown function.</h1><p>PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table &amp;#039;drupal7x.simpletest946234cache_block&amp;#039; doesn&amp;#039;t exist: DELETE FROM {cache_block} 
WHERE  (expire &amp;lt;&amp;gt; :db_condition_placeholder_0) AND (expire &amp;lt; :db_condition_placeholder_1) ; Array
(
    [:db_condition_placeholder_0] =&amp;gt; 0
    [:db_condition_placeholder_1] =&amp;gt; 1617023850
)
 in cache_clear_all() (line 186 of /path/to/drupal-7.x/includes/cache.inc).</p><hr />FATAL CommentActionsTestCase: test runner returned a non-zero error code (255).

I wonder if we need to implement a destructor instead of calling \DatabaseConnection::destroy() from \Database::closeConnection().

NR for the testbot

@Taran2L no problem committing to that fork - I will try to get used to using that / MRs instead of patches but prefer to concentrate on getting fixes ready for the release next week.. that probably means old-school patches for a little bit longer :)

When we discussed this earlier @Fabianx asked an important question about what's calling the code which destroys/breaks the db connection, and having dug into that a bit more I think we may be facing a problem created by the way our tests run.

https://git.drupalcode.org/project/drupal/-/blob/7.78/modules/simpletest...

\DrupalWebTestCase::tearDown() does:

    // Get back to the original connection.
    Database::removeConnection('default');
    Database::renameConnection('simpletest_original_default', 'default');

...and AFAICS it's that call to removeConnection() which calls self::closeConnection() which calls \DatabaseConnection::destroy() which seems to happening just before things go boom.

As a quick experiment, what happens if we don't make that call to closeConnection()? So far in local testing I'm not seeing any failures with PHP 8 or PHP 7.4, including ConnectionUnitTest which checks that connections are closed properly.

Maybe we'll need to do something a bit more nuanced, but let's try just commenting that one line out and running tests...

I don't know whether to laugh or cry :)

Without that call to closeConnection() PHP 8.0 tests run properly (remaining failures are not related AFAICS), and all the other PHP versions still pass.

Looks like the only other place in core that calls Database::removeConnection() is DrupalUnitTestCase::tearDown().

It's public though.

How about adding an optional param for whether to close the connection, which defaults to TRUE.

We'll leave the other call in DrupalUnitTestCase::tearDown() alone "if it ain't broke.."

If results are the same for this patch as for #26 hopefully this is an unexpectedly simple answer to my earlier question

what's the smallest possible change we can make to get PHP 8 passing without breaking PHP 5...?

Anyone see any problems with this approach?

Hmm thinking about it, maybe this is cheating as ConnectionUnitTest calls closeConnection() directly, and the whole point of #843114: DatabaseConnection::__construct() and DatabaseConnection_mysql::__construct() leaks $this (Too many connections) (which added that unit test) was to avoid e.g. test runs leaving connections open.

We could make deciding whether to call closeConnection() conditional on the PHP version (i.e. keep calling it for everything before PHP 8) to reduce the risks.

Perhaps that's not a proper solution...

I think non-test use cases which involve cycling through lots of connections (examples of which are mentioned in that original issue) should still be able to close connections explicitly.

The problem just seems to be a handful of situations which come about in the test suite where db access is attempted (e.g on destruction of DrupalCacheArray) after the test runner swaps the connections over.

Maybe the reasons those cases haven't caused problems before is because writing to cache / semaphore tables will succeed with or without the correct simpletest prefixes?

I'm inclined not to go too much further down the rabbit hole if we don't need to - so long as the test runners won't hit problems with too many connections building up...

To reduce risk by making the change even more minimal, we can omit the call to close the connection only for PHP 8 (and later).

Got the thumbs up from @Fabianx in chat.

@Taran2L what do you mean by "fix the test runner"? IIUC drupalci just uses the run-tests.sh script to run all tests in D7 core (I'm probably forgetting lots of other things it does too).

With any luck, this fix means that tests now run properly for PHP 8 on drupalci. We have about 13 failures left to fix.