Minimum MySQL version requirement is not confirmed when upgrading existing sites from Drupal 8 to Drupal 9

Thanks @balsama!

It's probable that the minimum database version is only checked at install time (I'm guessing we'd see the same things for Maria, Postgres, and SQLite versions below the minimum).

The main risk of using an unsupported DB version is that we introduce some syntax that's not supported on the previous versions of the drivers, leading to fatal errors and/or data integrity problems.

I wouldn't want to hardcode any kind of system_requirements() check for MySQL 5.6 specifically, but it seems like a generic check for the DB driver for the status and update reports would be good.

This makes sense. We need to do what we do in the installer - ie. $errors = db_installer_object($driver, $database['namespace'] ?? NULL)->runTasks(); on the driver configured in settings.php during update requirements checking. This is a bug that’s existed for a long time. We avoided in Drupal 7 to Drupal 8 updates.

Is this the fix we want for this issue?

Forgot to add use Drupal\Core\Database\Database;.

I think #6 is good if we can get it committed prior to the release candidate.

The other option is to only do this check in system_requirements() and show a warning (we should add that anyway, for example for situations like reloading a database dump between different environments).

+++ b/core/includes/update.inc
@@ -73,6 +75,16 @@ function update_system_schema_requirements() {
+  // Check before updating that the database requirements are met. Especially
+  // when updating to a new major version.
+  $connection = Database::getConnection();
+  $driver = $connection->driver();
+  $namespace = $connection->getConnectionOptions()['namespace'];
+  $errors = db_installer_object($driver, $namespace)->runTasks();
+  if (!empty($errors)) {
+    throw new InstallerException(implode("\n", $errors));
+  }

I think this should go in system_requirements(). There are decent arguments that these checks should run during the regular runtime check too. But to start with in this issue they should run only during the update phase.

One argument for a runtime check would be that it's possible to change a database to an earlier version of mysql and not run update.php. However this discussion is for a follow-up issue as calling runTasks() does a number of things including checking the engine version.

Thinking a bit more. Maybe we need to add a public function to \Drupal\Core\Database\Install\Tasks that only calls the protected checkEngineVersion function. Since the other tests are a bit unnecessary here. And then we could do the runtime and update time checks in one issue.

Here's an implementation of #9.

No interdiff because it's a different approach.

Also here are screenshots after manually testing by hacking a very high minimum version into the mysql driver.

Update requirements

System status report

+++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
@@ -151,6 +151,19 @@ public function runTasks() {
+  final public function engineVersionRequirementsCheck() {

This is final because we want database drivers to extend checkEngineVersion to do engine version checking and not this method. Like \Drupal\Core\Database\Driver\mysql\Install\Tasks::checkEngineVersion() does already.

Pretty sure that manual testing is the way forward here. We could add a db driver to core that wraps one of the core drivers and fakes the minimum version but that seems quite fraught and fragile.

+++ b/core/modules/system/system.install
@@ -427,18 +427,44 @@ function system_requirements($phase) {
+    $error_count = count($errors);
+    if ($error_count > 0) {
+      if ($error_count > 1) {
+        // If there are multiple errors use an item list.
+        $error_message = [
+          '#type' => 'inline_template',
+          '#template' => '{{ errors }}',
+          '#context' => [
+            'errors' => [
+              '#theme' => 'item_list',
+              '#items' => $errors,
+            ],
+          ],
+        ];
+      }
+      else {
+        $error_message = reset($errors);
+      }

Why not

if ($error_count > 1) {
  // item list....
}
elseif ($error_count > 0) {
  $error_message = reset($errors);
}

to save the nested if?

Re #13 because we only want to add the description & requirement error if there are errors.

Another way to achieve the bullets when multiple but none when only one item would be:

      $error_message = [
        '#theme' => 'item_list',
        '#items' => $errors,
        '#context' => ['list_style' => $error_count == 1 ? 'comma-list' : ''],
      ];

Which is slightly mis-using the comma-list style but has an advantage of always having an unordered list here so there's less markup difference between the two situations. Using the comma list for more that one error doesn't make sense because each error returned from \Drupal\Core\Database\Install\Tasks::checkEngineVersion() is a full sentence.

Here's an implementation of #15 with a comment to explain.

Regarding patch at #10

excerpt from system.install:
+ $errors = $tasks->engineVersionRequirementsCheck();
+ $error_count = count($errors);
+ if ($error_count > 0) {
+ if ($error_count > 1) {
+ // If there are multiple errors use an item list.
+ $error_message = [
+ '#type' => 'inline_template',
+ '#template' => '{{ errors }}',
+ '#context' => [
+ 'errors' => [
+ '#theme' => 'item_list',
+ '#items' => $errors,
+ ],
+ ],
+ ];
+ }
+ else {
+ $error_message = reset($errors);
+ }
+ $requirements['database_system_version']['severity'] = REQUIREMENT_ERROR;
+ $requirements['database_system_version']['description'] = $error_message;
+ }

I think we must initialize the $error_message array right after first if statement

if ($error_count > 0) {
$error_message = reset($errors);
if ($error_count > 1) {
// your piece of code

and do away with the else statement

@atul4drupal I'm confused by #18. Here's what #16 is doing.

+++ b/core/modules/system/system.install
@@ -439,6 +441,19 @@ function system_requirements($phase) {
+    $errors = $tasks->engineVersionRequirementsCheck();
+    $error_count = count($errors);
+    if ($error_count > 0) {
+      $error_message = [
+        '#theme' => 'item_list',
+        '#items' => $errors,
+        // Use the comma-list style to display a single error without bullets.
+        '#context' => ['list_style' => $error_count == 1 ? 'comma-list' : ''],
+      ];
+      $requirements['database_system_version']['severity'] = REQUIREMENT_ERROR;
+      $requirements['database_system_version']['description'] = $error_message;
+    }

Yes with #18 we could avoid an else in the patch attached to #10 but that doesn't really solve #13. And it results in an unnecessary assignment when dealing with multiple errors.

@alexpott apologies for the confusion. actually I mistakenly have #10 and #16 patch mixed up

agreed with #19,patch at #16 does a clean job.

Tiny nit (maybe) that could be fixed on commit:

+++ b/core/modules/system/system.install
@@ -427,9 +427,11 @@ function system_requirements($phase) {
+  if ($phase == 'runtime' || $phase == 'update') {

=== ?

Otherwise, tested this manually on a test server I have access to that's still on mysql 5.5. Everything works great. Adding before/after screenshots to the summary.

Queued tests on other databases/drivers to be safe.

This is disruptive. Mostly in a good way, but we need to mention the change in the release notes for people who had a beta installed, and it needs a CR I think.

+++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
@@ -151,6 +151,19 @@ public function runTasks() {
+  final public function engineVersionRequirementsCheck() {

Could we mock this to provide test coverage? Not sure if the final would work or not. I'd be okay with a followup given how bad this issue could be for the D9 upgrade path and the imminence of RC.

Added a CR and a release note.

CR looks great, thanks. Added a few commas, but otherwise it was ready. Removing tags.

Working on a test borrowing from @daffie's work in another issue.

Adding the commas to the release note, and adding the sentence about the status report.

Here's a test. The test-only patch is the interdiff.

@dww I left the sentence about the status report out of the release note because it didn't really seem so important. It feels like the important change here is that you can no longer update if the database defined in settings.php does not meant the minimum requirements. The status report change is for consistency and especially to help contrib drivers if they change their minimum version because such changes are unlikely to involved db updates. Unlike a core database driver minimum version change, which we try to only do during a major version update.

Note: we need this same test driver for testing #3120731: Incorrect "Drupal already installed" if any database settings are wrong or unsatisfactory so adding & reusing here seems nice. The test driver is all @daffie's work.

That test driver is great!

I did a little work on the release note and CR to make it clear that the warning previously only happened at install time and to give them an actionable step to take if it happens. I agree that we don't need to mention the status report in the release note.

1. +++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
   @@ -151,6 +151,19 @@ public function runTasks() {
   +   * @return array
   

   Nit: What kind of array?

2. +++ b/core/modules/system/tests/src/Functional/Update/DatabaseVersionCheckUpdateTest.php
   @@ -0,0 +1,63 @@
   +    if (Database::getConnection()->driver() !== 'mysql') {
   +      $this->markTestSkipped('This test only works with the mysql driver');
   +    }
   

   Is the idea to eventually provide test versions of each driver? (That's not really in scope here, just wondering.)

3. +++ b/core/modules/system/tests/src/Functional/Update/DatabaseVersionCheckUpdateTest.php
   @@ -0,0 +1,63 @@
   +    // Change the database driver to one that fakes the database version to one
   

   This says "to one" twice.

#22 was also not addressed. No fix-on-commits please.

Drupal\Tests\system\Functional\Update\DatabaseVersionCheckUpdateTest::testUpdate
Exception: Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "access_check.db_update".
Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition()() (Line: 1032)

Fixes #22, #36.1, #36.3, and adds more missing phpdocs for more of the methods in core/lib/Drupal/Core/Database/Install/Tasks.php. I didn't look into fixing the test failure, so uploading this as a do-not-test.

1. +++ b/core/modules/system/tests/src/Functional/Update/DatabaseVersionCheckUpdateTest.php
   @@ -36,8 +36,8 @@
   +    // Change to a database driver that fakes the database version to one that
   +    // does not meet requirements.
   

   I still don't get it. :-) It's still says "to" twice.

2. +++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
   @@ -82,6 +82,8 @@
      /**
       * Ensure the PDO driver is supported by the version of PHP in use.
   +   *
   +   * @return bool
       */
      protected function hasPdoDriver() {
        return in_array($this->pdoDriver, \PDO::getAvailableDrivers());
   @@ -103,6 +105,8 @@ protected function pass($message) {
   
   @@ -103,6 +105,8 @@ protected function pass($message) {
    
      /**
       * Check whether Drupal is installable on the database.
   +   *
   +   * @return bool
       */
      public function installable() {
        return $this->hasPdoDriver() && empty($this->error);
   @@ -110,13 +114,15 @@ public function installable() {
   
   @@ -110,13 +114,15 @@ public function installable() {
    
      /**
       * Return the human-readable name of the driver.
   +   *
   +   * @return string
       */
      abstract public function name();
    
      /**
       * Return the minimum required version of the engine.
       *
   -   * @return
   +   * @return string
       *   A version string. If not NULL, it will be checked against the version
       *   reported by the Database engine using version_compare().
       */
   @@ -127,7 +133,7 @@ public function minimumVersion() {
   
   @@ -127,7 +133,7 @@ public function minimumVersion() {
      /**
       * Run database tasks and tests to see if Drupal can run on the database.
       *
   -   * @return array
   +   * @return string[]
       *   A list of error messages.
   

   None of this is in scope; can you open a separate issue with these changes?

3. +++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
   @@ -211,7 +232,7 @@ protected function checkEngineVersion() {
   -   * @param $database
   +   * @param string[] $database
       *   An array of driver specific configuration options.
       *
       * @return
   @@ -306,10 +327,10 @@ public function getFormOptions(array $database) {
   
   @@ -306,10 +327,10 @@ public function getFormOptions(array $database) {
       * Checks to ensure correct basic database settings and that a proper
       * connection to the database can be established.
       *
   -   * @param $database
   +   * @param string[] $database
       *   An array of driver specific configuration options.
       *
   -   * @return
   +   * @return string[]
   

   Ditto these.

#43.1: Trying again. /shrug

2-3: Crap, I totally misread the git status. I thought this was a new file, and saw a bunch of missing docs. My bad. Reverted all that crap.

Interdiff relative to #29, which is all that matters.

Meanwhile, I couldn't get this test to fail locally, either with phpunit nor run-tests.sh. Not sure WTF is going on with the bot failures. I'll let the bot churn on this one again, in case it was a random fail or something?

Another attempt to be even more clear about #43.1.

Follow-up at #3136388: Fix phpdocs in core/lib/Drupal/Core/Database/Install/Tasks.php

Yeah, more WTF. The test passes locally for me, both phpunit and run-tests.sh. I suspected relative path weirdness in the 'autoload' directive in the test, but I confirmed that isn't the problem in this case. At least not locally.

Here's an initial shot in the dark.

Does this tell us any more?

Okay, so that is interesting. We only died on line 73. Therefore, the first two attempts to fetch update.php worked fine. It's only once we try to swap out the DB driver that the access check is broken. Does rebuilding the container at that point help us?

Okay, clearly the protected static $modules = ['system'] doesn't help, nor rebuildContainer(). However, maybe it's the call to Url::fromRoute() that's killing it? Since we already did that above, maybe we can avoid it?

+  /**
+   * Tests that access_check.db_update service found.
+   */
+  public function testUpdateServiceFound() {
+    $service = \Drupal::service('access_check.db_update');
+    $this->assertInstanceOf(DbUpdateAccessCheck::class, $service);
+  }

Strange. Just added another test to test that the service can be found.

Re: #56: Sadly, that's not going to tell us anything. As I wrote at #52, #50 shows that the service works fine until we swap out the DB driver.

Based on the above, it seems that once we swap the DB connection details to try to use a different driver, all of Drupal breaks since there's no working DB to use for any queries. However, I have no idea why this only happens on the bot, not when testing locally.

I highly doubt this last experiment will work, but here's some defensive programming in case the relative path for the autoload is what's actually breaking things on the bot. Again, this works locally on both phpunit and run-tests.sh. /shrug

I give up for the night. Hopefully when @alexpott wakes up, he can make sense of this mess. 🤞He seems to have a knack for understanding why things are different on the bot vs. local testing.

The variable \PDO::ATTR_SERVER_VERSION returns something like: "5.7.27-0ubuntu0.18.04.1-log" or "10.2.31-MariaDB-1:10.2.31+maria~bionic-log".
The variable \PDO::ATTR_CLIENT_VERSION returns something like: "mysqlnd 5.0.12-dev - 20150407 - $Id: 7cc7cc96e675f6d72e5cf0f267f48e167c2abb23 $".
They are not the same thing. Removed the method override for clientVersion() in the new test driver class. Lets see if that makes the testbot happy.

Let's try this. The test passes and fails locally as expected. But then again so did #30.

Let's try writing the db settings in a different way.

Yay - so this is a run tests thing. If I run #59 using run-tests.sh with no db connection is the test runner database and using the sqlite option for report results I get the same fail as DrupalCI.

So here's a proper patch with a fail test and all changes back to the last proper patch in #45

On my local test cases are getting passed. Testing patch for test case failure.
Apologies I didn't check the updated thread.

All points of @xjm are addressed with the exception of #36.2. For #36.2: We can fix it for all by core supported database drivers. This will result in 2 more test database drivers being added to the code base. For me seeing that it works for MySQL is enough.

The added test work for me locally and the testbot is also happy.
All the code changes look to me as good.
The patch is for me RTBC.

Reuploading the patch from comment #62, because that is the one that needs to be committed.

@alexpott: Great work!

Here's a slightly better way to change the database settings since it relies on the writeSettings test helper. Leaving at RTBC because the changes are test only and very minor.

Committed/pushed to 9.1.x and cherry-picked back to 8.9.x

Had to revert this on 8.9.x because this was causing https://www.drupal.org/pift-ci-job/1686421 - it's because #3124354: Remove unnecessary classes from the database drivers did not make it back to 8.9.x

I think we should backport it without tests then?

I think this is fine to backport without tests, the tests are the complex part of the patch..

Here's a patch for 8.9.x with a test. Considering this is update path stuff it feels worth it.

All code changes look good.
Back to RTBC.

Committed the backport to 8.9.x. Thanks! I didn't commit it to 8.8.x because the new warning is a disruption.

Mrrrr, the test is failing on PHP 7.0:

Drupal\Tests\system\Functional\Update\DatabaseVersionCheckUpdateTest::testUpdate
  TypeError: Return value of
Drupal\Tests\system\Functional\Update\DatabaseVersionCheckUpdateTest::setUp()
must be an instance of Drupal\Tests\system\Functional\Update\void, none
returned


/var/www/html/core/modules/system/tests/src/Functional/Update/DatabaseVersionCheckUpdateTest.php:29

Hotfix comin' right up. Stay tuned.

Please ignore the PHP 7.0.x-dev result, that was confusion on my part. Hotfix is green on all relevant PHP 7 configs. My work here is done. ;)

Looks good to me, the 'void' return type wasn't added until 7.1 so this seems the appropriate fix.

Given it's such a tiny change, going to RTBC it.

Thanks!

Proposed commit message:

git commit -m 'Issue #3135629 by xjm, dww, DamienMcKenna: Follow-up hotfix for PHP 7.0 compatibility.'

Committed the hotfix to 8.9.x. Thanks for the quick fix!