\MigrateSQLMap::getQualifiedMapTable may break with MySQL 8 support in D7 core

There's no patch to review yet, so back to Active (or could be NW).

I've reproduced the problem by setting up a migrate_d2d migration between two vanilla D7 sites with a handful of nodes in the source db created by devel_generate.

With the (current) latest patch from #2978575-218: Mysql 8 Support on Drupal 7 applied to core, migrations fail with an error like this (where d7target is the name of my target db):

Migration for DrupalFile7Migration failed with source plugin exception: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd7target.`d7target`.migrate_map_d7sourcefile' doesn't exist, in /path/to/drupal-7.x/includes/database/database.inc:2227

...which matches the report by @Ronino in the core issue (see #221).

If, however, I short-circuit the db qualifying in getQualifiedMapTable with something as simple as this:

  public function getQualifiedMapTable() {                                         
    $options = $this->connection->getConnectionOptions();                          
    $prefix = $this->connection->tablePrefix($this->mapTable);                     
    if (TRUE || $prefix) {                                                         
      return $this->mapTable;                                                      
    } 

...then the migration works fine.

We know that migrations where the (target?) database uses table prefixes work okay without adding the dbname to qualify the table name (as is the case with the module's simpletests).

So this all suggests to me that migrate could function without doing this table prefixing / qualifying.

However, I don't understand the reason this was added sufficiently well to know exactly what the consequences of removing it would be.

Sorry, didn't mean to revert the Priority.

This is a potential blocker for the current top-priority D7 core issue.

Looking more closely, it's actually the backticks that migrate is placing around the dbname for mysql databases that are causing the problem.

If we remove those lines, it looks like migrate works okay when it adds the dbname to the table name.

I think this is because D7 core (with the latest patches from the MySQL 8 issue) can cope with e.g. {d7target.migrate_map_d7sourcefile} as long as it doesn't contain backticks.

That now gets wrapped in quotes e.g. "d7target.migrate_map_d7sourcefile" so the original reason for adding the backticks no longer applies (once table name quoting is added to core).

So I think we can simply remove the lines that add the backticks - here's a patch which does that.

Oops the patch was a snafu, and I think I may have jumped to an incorrect conclusion anyway.

...looking at this some more.

edit - the patch should have been this:

diff --git a/plugins/sources/sqlmap.inc b/plugins/sources/sqlmap.inc
index 247bf36..c5c11cd 100644
--- a/plugins/sources/sqlmap.inc
+++ b/plugins/sources/sqlmap.inc
@@ -39,13 +39,7 @@ class MigrateSQLMap extends MigrateMap {
       return $this->mapTable;
     }
     else {
-      $dbname = $options['database'];
-
-      if (!empty($options['driver']) && $options['driver'] == 'mysql') {
-        // Backtick the db name on mysql to ensure dbs with hyphens work.
-        $dbname = "`{$dbname}`";
-      }
-      return $dbname . '.' . $this->mapTable;
+      return $options['database'] . '.' . $this->mapTable;
     }
   }

Yes, looks like I got that wrong; in my manual testing the first migration I'm running always succeeds, and that's because getQualifiedMapTable only gets called on subsequent migrations when \MigrateSource::rewind is called.

It's only then that this fails with the MySQL 8 patches when it adds the dbname. qualifier to the table name.

So unfortunately I'm back to thinking this won't work with the latest MySQL 8 patches; and I'm inclined not to add potentially expensive workarounds in core.

getQualifiedMapTable is called from two places, and one of them does this:

https://git.drupalcode.org/project/migrate/-/blob/7.x-2.11/plugins/sourc...

          $count_needs_update = db_query('SELECT COUNT(*) FROM {' .
            $this->activeMap->getQualifiedMapTable() . '} WHERE needs_update = 1')
            ->fetchField();

...which I think is where the problem is happening, because the curly braces are being wrapped around dbname.tablename.

Here's a patch which adds a param to indicate whether the table name (and only that) should be wrapped in braces to avoid this issue.

This works AFAICS, and I think it may actually mean it's possible to remove the logic which skips the qualiification of table names if prefixing is in place. I've not tried to do that in this patch, but it would reduce duplication in the method if we could.

I'm not certain why the tests are hitting composer problems.

However, the patch wasn't working as intended because of a silly PHP issue with double quotes, variable interpolation and the curly braces :face-palm:

With this version, tests are passing for me locally (PHP 7.2 and MySQL 5.6 without MySQL 8 core patches).

I've run the Migrate tests now with the following combinations:

• PHP 7.2 and MySQL 5.6 (with and without the core MySQL 8 patch #218)
• PHP 7.2 and MySQL 5.7 (with and without the core MySQL 8 patch #218)
• PHP 7.2 and MySQL 8 (with the core MySQL 8 patch #218)

All tests pass in each case with #11 applied to migrate.

I've also done a manual test of the 2-stage d2d migration (so that rewind is called) with each of the MySQL versions, and I've not hit any errors.

So looks to me like this patch works.

As mentioned, I think it may actually be possible to remove the "skip if there's a prefix" logic from getQualifiedMapTable with this change, but I've not looked at that in any detail, and it might be one for a follow-up rather than allowing scope-creep here.

I've also been manually testing with a migration between two D7 dbs.

IIUC the difference you're pointing out is:

if ($this->mapJoinable) {

https://git.drupalcode.org/project/migrate/-/blob/7.x-2.11/plugins/sourc...

vs.

if (!$this->mapJoinable) {

https://git.drupalcode.org/project/migrate/-/blob/7.x-2.11/plugins/sourc...

So seems this patch fixes the latter but not the former; I'm guessing then that in my migrate_d2d testing I don't have a mapJoinable mapping.

Noting for context that in #1480762-6: Join of legacy data to default database map table does not work (where the dbname. qualifying was added) @mikeryan said:

... seems to work for normal db-to-db migrations, and in simpletest. It will not work for a migration from an external database into a prefixed Drupal installation (but then it never did, so no regression) - in those cases you'll have to use mapJoinable => FALSE.

I'm not sufficiently familiar with all of this to judge whether that describes an acceptable workaround or whether more work is needed to try and fix:

$this->query->leftJoin($this->activeMap->getQualifiedMapTable(),
          'map', $map_join);

...when mapJoinable is TRUE.

Since the dbname is not added when a table prefix is in use, what would the consequences be of simply not adding it for the leftJoin?

#2978575: Mysql 8 Support on Drupal 7 was just committed to D7 and should be included in the release next week (which will almost certainly be Drupal 7.75).

This issue (or at least a very closely related one) is being looked at in D9 core over in #3227361: Fix SqlBase::initializeIterator()'s cross-database joining when using particular DB/table names.

Perhaps the eventual fix there will be backport-able?