Fix SqlBase::initializeIterator()'s cross-database joining when using particular DB/table names

Does this have any overlap with #3171091: \MigrateSQLMap::getQualifiedMapTable may break with MySQL 8 support in D7 core?

+++ b/core/lib/Drupal/Core/Database/Query/Select.php
@@ -846,6 +846,11 @@ public function __toString() {
+          [$db_name, $table_name] = explode('.', $table_string, 2);
+          $table_string = "[$db_name].[$table_name]";

With this construction the table name will no longer be prefixed. As is done in the if part with $table_string = '{' . $table_string . '}';.

1. +++ b/core/lib/Drupal/Core/Database/Query/Select.php
   @@ -846,6 +846,11 @@ public function __toString() {
   +          $table_string = "[$db_name].[$table_name]";
   

   Why do we not do:

   $table_string = $this->connection->escapeDatabase($db_name) . '.{' . $table_name . '}';
   

2. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -288,9 +288,12 @@ protected function initializeIterator() {
   +            $field_name = '[' . $field_schema['alias'] . '].' . $this->query->escapeField($field_name);
   

   Why do we not do:

   $field_name = $this->query->escapeAlias($field_schema['alias']) . '.' . $this->query->escapeField($field_name);
   

3. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -288,9 +288,12 @@ protected function initializeIterator() {
   +            $field_name = "[$field_name]";
   

   Why do we not do:

   $field_name = $this->query->escapeField($field_name);
   

4. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -288,9 +288,12 @@ protected function initializeIterator() {
   +          $map_join .= "$delimiter$field_name = [map].[sourceid" . $count++ . ']';
   

   Why do we not do:

   $map_join .= "$delimiter$field_name = " . $this->query->escapeAlias(map) . '.' . $this->connection->escapeField(sourceid" . $count++);
   

5. +++ b/core/lib/Drupal/Core/Database/Query/Select.php
   @@ -846,6 +846,11 @@ public function __toString() {
   +          [$db_name, $table_name] = explode('.', $table_string, 2);
   

   This can for PostgreSQL databases be 3 parts: database name, schema name and table name.

The testbot is failing for PostgreSQL, because of #19.5. Maybe we could do something like:

  // But do ensure identifiers (databases and table names) are escaped.
  else {
    $table_string_parts = explode('.', $table_string);
    $table_name = array_pop($table_string_parts);
    $table_string = '';
    foreach($table_string_parts as $table_string_part) {
      $table_string .= $this->connection->escapeDatabase($table_string_part) . '.';
    }
    $table_string .= "[$table_name]";
  }

Not sure about the part: $table_string .= "[$table_name]";
This way it works for all relational databases, including PostgreSQL.

#18 + #19.1: {…} applies the prefix for the current DB connection, not the prefix in use on the other database you're joining against.

I would very much to like have testing for that.

#19.2-4: ok, lets wait with that for the moment. Lets fix the important parts first.

It is becoming crystal clear that the slapdash join we have in dbtng is just not enough.

I have been thinking on that for a long, long time. And Views has a join builder, for a very different reason, of course.

Rule of thumb when writing DBTNG was: never parse a query. It will always fail in unexpected ways and if I gather correctly, that's what happens here.

We need an expression builder for DBTNG which we partially have but not fully. like ->condition('foo', 1) translates to foo = 1 but if you wanted foo.bar = 1 properly escaped? sucks to be you. And for any complex expression we have ->expression() and ->where().

That's the full solution to this. And then you don't escape anything because the builder does.

It's quite a bit of work -- but perhaps ... we just need to build a tree. The leaves can have value objects based on escaping strategy necessary (variable/fieldname/tablename/dbname) and then one level up, what do we have? we need an infix operator class and a function call.

$e = \Drupal::database()->getConnection()->expression();
$left = $e->infix('.', $e->table($field_schema['alias']), $e->field($field_name));
$right = $e->infix('.', $e->table('map'), $e->field('sourceId'  . $ocunt++);
$piece = $e->Infix('=', $left, $right);
if (!isset($return)) {
  $return = $piece;
}
else {
  $return = $e->infix(' AND ', $return, $piece);
}

That was just an example. Make the join method accept n expression besides a string and then the objects can deal with printing themselves via __toString(). Everywhere where migrate uses the database name, use an expression. Do not attempt to solve this. Make it smart and make it solve itself.

Bonus: the builder is instantly usable for where and expression.

Here's a start: https://gist.github.com/chx/e74ac22799ba6cd4a7beebad92d1301a

The expression builder I suggested would be very limited -- just enough for writing the join conditions and such. I will write it over the weekend.

Edit: however, while writing it, I found a bug, I think: Connection::getFullQualifiedTableName does not escape $options['database']. I think we should and that would fix this?

This seems a simple but better route. If the problem is funny database names, just .... escape those? Especially since we already have the escape method for those?

I have changed the [] in the plugin base to explicit escapes. We already have an escapeField there and half explicit half magic beans is not a tasty soup.

It had a JavaScript fail, that was unrelated.

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -531,7 +531,7 @@ public function getUnprefixedTablesMap() {
   -    return $options['database'] . '.' . $prefix . $table;
   +    return $this->escapeDatabase($options['database']) . '.' . $prefix . $table;
   
   +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -319,7 +319,7 @@ public function getFullQualifiedTableName($table) {
   -    return $options['database'] . '.public.' . $prefix . $table;
   +    return $this->escapeDatabase($options['database']) . '.public.' . $prefix . $table;
   

   I think these changes are wrong. The method getFullQualifiedTableName() returns it unquoted. With this change, only the database name gets quoted.

2. I think we should add a new method to the class Drupal\Core\Database\Select. In this method we should be able to add all parameters that belong to the join. It is then up to the query builder to add the quotes as it sees fit.
   The code comes from Drupal\Core\Database\Query\SelectInterface and modules/views/src/Plugin/views/join/JoinPluginBase.php

     /**
      * Join against another table in the database with an inner join.
      *
      * This method does the "hard" work of queuing up a table to be joined against.
      * In some cases, that may include dipping into the Schema API to find the necessary
      * fields on which to join.
      *
      * @param string $table
      *   The table against which to join. May be a string or another SelectQuery
      *   object. If a query object is passed, it will be used as a subselect.
      *   Unless the table name starts with the database / schema name and a dot
      *   it will be prefixed.
      * @param string $field
      *   The field of the table $table to use in the join.
      * @param string $left_table
      *   The table to which the table $table joins to.
      * @param string $left_field
      *   The field of the table $left_table to use in the join.
      * @param string $alias
      *   The alias for the table. In most cases this should be the first letter
      *   of the table, or the first letter of each "word" in the table.
      * @param string $operator
      *   (optional) The operator to use in the join. Defaults to '='.
      * @param array $extra
      *   An array of extra conditions on the join.
      *
      *   Each condition is either a string that's directly added, or an array of
      *   items:
      *     - table(optional): If not set, current table; if NULL, no table.
      *     - field(optional): Field or expression. In expressions we can reference the
      *       right table by using %alias.
      *     - left_field(optional): Field or expression. In expression we can reference
      *       the left table by using %alias.
      *     - operator(optional): The operator used, Defaults to "=".
      *     - value: Must be set. If an array, operator will be defaulted to IN.
      *     - numeric: If true, the value will not be surrounded in quotes.
      *
      * @return $this
      *   The called object.
      */
     public function addInnerJoin($table, $field, $left_table, $left_field, $alias, $operator = '=', $extra = NULL) {
   

   We should also add addLeftJoin() for a left join.

Long issue titles hurt my brain. Since this in migration system I don't think it will be confused with \Drupal\views\Plugin\views\pager\SqlBase.

Updated IS to start with the database name formats that cause the error.

Changing status to NW for the tests and the comments in #28.

Patch #25 no longer applies, I rerolled patch #25 however then I read the comments from @daffie , @daffie says this is wrong. I'd like this issue to be fixed because it's not only affecting Symfony Drupal it's also affecting Drupal 7.94 which has modeled it's db abstraction layer changes from changes made to symfony Drupals abstraction layer.

For now, handing this off to someone else, good luck!

I am pushing the reroll of patch 25 to see if it still passes testing just for curiosity sake.

Apparently this patch needs changes as per #28 @daffie comment and #31 @quietone

hmm, checking between branches, 9.5.x, 10.0.x, 10.1.x, the patch #34 conflicts with 10.1.x core/lib/Drupal/Core/Database/Connection.php

Perhaps part/or/all of this has been fixed already in 10.1.x ?

Probably not, but ya, hopefully someone with more experience working on the db abstraction layer can make the expected changes.

I tried applying the latest patch to 10.0 and it doesn't fix the problem -- running a migration from D7 in the UI shows lots of:

> Migration failed with source plugin exception: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupalfirecore10.migrate_map_d7_action' doesn't exist: SELECT "a".*, "map"."sourceid1" AS "migrate_map_sourceid1", "map"."source_row_status" AS "migrate_map_source_row_status" FROM "actions" "a" LEFT OUTER JOIN drupalfirecore10.migrate_map_d7_action "map" ON aid = map.sourceid1 WHERE ("map"."sourceid1" IS NULL) OR ("map"."source_row_status" = :db_condition_placeholder_0); Array ( [:db_condition_placeholder_0] => 1 ) in /Users/joachim/Sites/firecore/web/core/modules/mysql/src/Driver/Database/mysql/ExceptionHandler.php line 46

My DB is called drupal-firecore-10, not drupalfirecore10!

I've received this suggestion as a fix:

  public function escapeTable($table) {
    if (!isset($this->escapedTables[$table])) {
      $database = $this->getConnectionOptions()['database'];
      if (str_starts_with($table, "$database.")) {
        $to_escape = substr($table, strlen($database) + 1);
        $prefix = $this->quoteIdentifiers("[$database].");
      }
      else {
        $to_escape = $table;
        $prefix = '';
      }
      $this->escapedTables[$table] = $prefix . preg_replace('/[^A-Za-z0-9_.]+/', '', $to_escape);
    }
    return $this->escapedTables[$table];
  }

#38 worked for me. thank you @joachim

Patch for 11.2.5 attached