Problem/Motivation

You have a Drupal 6 database and a Drupal 8 database. Migration puts the id mapping table in the Drupal 8 database because that's the only database it knows it has write permissions to. It tries to join this table against tables in the Drupal 6 database. This breaks if a default prefix is set because Select::__toString unconditionally prefixes every table name and so it'll try do defaultprefix_dbname.tablename. On MySQL this foo.bar notation would mean foo is a database, on PostgreSQL it would it's a schema.

Note that the semi-common practice of using prefixes with dots have absolutely no bearing on this patch. It's the actual table names across databases / schemas table names containing dots are not supported.

Proposed resolution

Do not prefix table names if they contain a dot. Trust the caller to prefix the table name as necessary for that database. Do not trust it to escape however. Never trust the caller with security. And this is fine: escapeTable is already keeping the dots. Note this is reasonably backwards compatible because previously such joins simply didn't work as the prefix would be applied to the database name. It is not impossible that someone actually used default or per table prefixes to make such joins do something tricky but for my life I can't imagine such a scenario. (And I have a very wild imagination in these matters. Also called experience.)

A test would only be possible if there were phpunit tests for the database as without mocking you'd need two databases to test this against. So I don't think a test is feasible to be required at this point.

CommentFileSizeAuthor
#8 2180539_8.patch4.2 KBchx
#8 interdiff.txt3.42 KBchx
#6 interdiff.txt3.17 KBchx
#6 2180539_6.patch3.95 KBchx
cross_db_join.patch798 byteschx

Comments

chx’s picture

Issue summary: View changes
chx’s picture

Issue summary: View changes
chx’s picture

Priority: Normal » Major

This is major as it blocks migrate.

chx’s picture

Issue summary: View changes
Crell’s picture

Discussed this a bit with chx in IRC. We should add a bit of documentation about this loophole in the table prefixing, but otherwise it looks fine to me.

chx’s picture

StatusFileSize
new3.95 KB
new3.17 KB
Crell’s picture

Oh momma! If we're going to go that far, perhaps we should indicate why a period is taken to disable prefixing? I suspect a lot of people don't even realize a period means "database/schema name".

chx’s picture

StatusFileSize
new3.42 KB
new4.2 KB
Crell’s picture

Status: Needs review » Reviewed & tested by the community

My work here is done. Thanks, chx!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Seems straight-forward.

I asked chx about test coverage but he said that this isn't currently possible because the DBTNG tests leverage DrupalUnitTestBase, which means they use the database. In order to test this you'd apparently need two different databases which our set up does not support.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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