Migrate Drupal reports wrong version of Drupal if pointed at a Drupal 9 or 10 database

I tested with a Drupal 10 source database and the version displayed was '1'. Attached is a fix for that.

Regarding the situation in the issue summary, as pointed out this is working as designed. The only source of data is the source database and the only way to to figure out the version is the value for the system_scheme. Not sure why the Drupal 10 site shows a Drupal 8 schema.

The last submitted patch, 2: 3323990-2.patch, failed testing. View results →

I guess we could add some tests on the version string. Maybe this will do.

CC failure in #5

That's odd. Anyway updated patch. No interdiff because this is so small.

I pointed my migration setup to a 11.x branch and got this error
Source database is Drupal version 1 but version 7 was selected.
Applying the patch I get
Source database is Drupal version 10 but version 7 was selected.

Which I believe is correct as the 11.x branch guess is D10.

So issue seems to be resolved with the patch in #7

Also ran the test without the fix and got
Failed asserting that two strings are identical.
Expected :'10'
Actual :'1'

Which was the same as my manual test.

The last submitted patch, 7: 3323990-7.patch, failed testing. View results →

Unrelated failure.

The last submitted patch, 7: 3323990-7.patch, failed testing. View results →

Random failure

+++ b/core/modules/migrate_drupal/src/MigrationConfigurationTrait.php
@@ -244,7 +244,14 @@ public static function getLegacyDrupalVersion(Connection $connection) {
+    }
+    $length = 1;
+    if (strlen($version_string) > 4) {
+      $length = -3;
+    }
+    return substr($version_string, 0, $length);
   }

I think this could use a comment.

Patch #7 fixes the error message when connectng to drupal 10 site.
But connecting to drupal 9 site still shows incorrect error message. Error message is "Source database is Drupal version 8 but version 7 was selected." instead of "Source database is Drupal version 8 but version 7 was selected."

I tested it in Drupal 9.5.11. As mentioned in the IS, drupal checks key_value table in source database to determine drupal version of source site.

    // For Drupal 8 (and we're predicting beyond) the schema version is in the
    // key_value store.
    elseif ($connection->schema()->tableExists('key_value')) {
      try {
        $result = $connection
          ->query("SELECT [value] FROM {key_value} WHERE [collection] = :system_schema AND [name] = :module", [
            ':system_schema' => 'system.schema',
            ':module' => 'system',
          ])
          ->fetchField();
        $version_string = unserialize($result);
      }

Output of this query for the D9 site is i:8901; Ideally output should have been in the format "i:9xxxx"(For D10 site, its i:10100; ).

Is there any other table that we can use to get the drupal version?

I have converted to an MR and added a comment per #13. It really does need a comment and I should have done it when I wrote that.

@Akhil Babu, Are you sure that is a Drupal 9 site? Did all the database upgrades run successfully?

Yes. I tried with a fresh d9 site. Also tried installing the site using drush as well as from the UI. In both cases, value getting stored in the table is i:8901;

Would it be easier to just change it to say 'Drupal version 8 or later' if we don't explicitly detect 6 or 7? The migrate UI is only designed for 6 or 7, so it doesn't much matter that we accurately find out the specific version, only that we accurately determine if it's 6 or 7, or something we don't handle.

benjifisher → credited mikelutz → .

@mikelutz and I discussed this issue at 📌 [meeting] Migrate Meeting 2023-12-21 1400Z Active . We agree that getting the Drupal version from the schema version is unreliable. What we should really do is check for the common mistake and give a really specific error message in that case. Otherwise, say something like "it is not a D7 database" instead of trying to say what version it is.

Closely related to how unreliable it is to use the schema version: I confirmed what @Akhil Babu pointed out in Comment #17. With a fresh install of Drupal 9.5.11, the schema version is 8901. I think it would be different if I had started with an earlier version of Drupal 9 and upgraded to 9.5.11 (depending on which earlier version). But for migrations, a clean install is an inmportant use case.

Let's go back and start again. I am setting the issue status back to NW.

I removed everything related to trying to use the schema in key_value to get the Drupal version for D8+. If the source database is a Drupal 8+ database the error message will be, "Source database does not contain a recognizable Drupal version.". That is not the best error message so that needs work.

I also added a check if the source database is the same as the site database. If that is the case then the error message is "Source database is the same as the site database."

@benjifisher, thanks for the review. I have replied to your comments and this is in better shape now.

Think benjifisher should do the re-review here.

I've tested the latest changes.
Using drupal 9/10 sites as the source now throws the new error message "Source database does not contain a recognizable Drupal version."

@Akhil Babu:

Thanks for catching that!

Instead of

      if ($connection->getConnectionOptions() == Database::getConnectionInfo()['default']) {

we should probably use

      if ($connection->getConnectionOptions() === Database::getConnection()->getConnectionOptions()) {

I tested this using both MariaDB and SQLite.

Unfortunately, Strict comparison can't be used here. As mentioned in the MR the array key order is not guaranteed to be that same. And the screenshots from #25 also show that the arrays have different values even for the same db. Therefor, I have reworked the logic to compare a subset of the connection options.

I also looked more closed at the new error message and compared it with the existing language. For example, the Overview page uses old site and new site. Because of that I changed the message. I hope it is an improvement.

Thanks for updating @quietone

I think strict comparison should be removed from the new logic as well. The ‘port’ key in the datatable seems to be causing the condition to fail. But, it works fine with ==.

@Akhil Babu, thanks for the feedback. In the future, when reporting testing results it is good practice to include the steps you took.

I did some manual testing and found that it is rather easy to have the port number a string in one case and a number in the other. I use ddev and ddev sets the port as a number but the example in settings.php is to use a string. Therefor I added an array_map to cast each element to string.

I am not sure why this was causing errors in some Drupal 6 Upgrade tests but the fix is to remove a change I made early when working on this.

1. In the test function:

        $schema->expects($this->any())
          ->method('tableExists')
          ->willReturnMap($table_map);

   And in the data provider:

          'D9' => [
            'expected_version_string' => FALSE,
            'schema_version' => serialize('9270'),
            'exception' => NULL,
            'table_map' => [
              ['system', TRUE, FALSE],
              ['key_value', TRUE, TRUE],
            ],
          ],
          'D10' => [
            'expected_version_string' => FALSE,
            'schema_version' => serialize('10101'),
            'exception' => NULL,
            'table_map' => [
              ['system', FALSE],
              ['key_value', TRUE],
            ],
          ],

   I am not sure whether it is required or just good practice, but when mocking a method with an optional parameter, we explicitly provide the parameter. The existing test case for D9 does this: 'system' and TRUE are the parameters for tableExists(), and FALSE is the result. We should use the same pattern for D10.

2. Since the MR removes the call $connection->schema()->tableExists('key_value') in getLegacyDrupalVersion(), we should remove the lines with 'key_value' from the data provider and add an assertion that tableExists() is not called with 'key_value'. Or there is probably a simpler way to get the same result. (We do not really need a map if we know that the method is always called once.)

3. I think it is a little simpler to use array_intersect_key():

          $options = array_flip(['database', 'host', 'port', 'prefix']);
          $source = array_map('strval', array_intersect_key(
            $connection->getConnectionOptions(),
            $options,
          ));
          $destination = array_map('strval', array_intersect_key(
            Database::getConnection()->getConnectionOptions(),
            $options,
          ));
          ksort($source);
          ksort($destination);

4. More importantly, the list of options has to depend on the driver. If the driver is sqlite, then I think only database (the filename) and prefix matter. If port or host are supplied, then (I think) they are ignored. We should add driver and/or namespace to the list of options. For mysql, port is optional: leaving it unset is the same as setting it to 3306. I am not sure about PostgreSQL.

   It is tempting to use $connection->createUrlFromConnectionOptions(), but that is marked as

       * @internal
       *   This method should only be called from
       *   \Drupal\Core\Database\Database::getConnectionInfoAsUrl().

   and getConnectionInfoAsUrl() only works if we have a key in the $databases array. It is so close to being easy, but it is not!

@benjifisher, thanks for reviewing!

1,2,3 Fixed
4. While it does depend on the driver the keys not used will be filtered out, so we just need the maximum sensible set of keys. So, when I tested with sqlite the arrays being compared only had the 'database' and 'prefix' keys. Adding 'driver' is problematic because you have say 'mysql' and 'Drupal\mysql\Driver\Database\mysql', the later being produced from the CredentialForm. That is why I had a change earlier to strip that to just the driver name. That could be added back. See https://git.drupalcode.org/project/drupal/-/merge_requests/5813#note_246068

Seems to need a rebase.

Small discussion in #d11readiness we are holding the subtree split for the book module because of 📌 Convert MigrateDestination plugin discovery to attributes Postponed which is postponed on this one so hopefully can land soon.

There's also 1 other book ticket unrelated we are also waiting on.

This issue is not a blocker. [#/3421015] is blocked on 📌 Update MigratePluginManager to include both attribute and annotation class Fixed which is waiting for reviews.

I have rebased. The only conflict was removing phpstan-baseline.neon. This is passing commit-code-check locally but fails on gitlabci.

@quietone thanks for clarifying in #36.

Looking at the MR I see the 1 thread open, from what I can tell, has been addressed.

The changes look good, unless there is anything else needed for this?

Sorry, March has been a bit crazy for me.

Thanks for updating MigrationConfigurationTraitTest. I like the new willReturn() bits a lot better.

I have one suggestion for CredentialFormTest. See the MR. Back to NW for that.

As we discussed in today's migration meeting, let's add a follow-up issue to simplify getLegacyDrupalVersion() further. Copying my suggestions from there:

1. Put a lot less in the try block.
2. Return early instead of assigning to $version_string and then returning it at the end

.

Tagging for the followup.

Opened 📌 Simplify getLegacyDrupalVersion() Active

Appears rest of feedback has been addressed

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

Crosspost issues...

quietone → closed merge request !5813

Automatically closed - issue fixed for 2 weeks with no activity.