Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
getQualifiedMapTableName in
\core\modules\migrate\src\Plugin\migrate\id_map\Node.php
is generating full qualified table names that are missing the SCHEMA part.
It will return {database}.{prefix}{tablename}
when it should return {database}.{schema}.{prefix}{tablename}
Looks like in mySQL you can cross database without using a schema, but this fails to work on other databases.
Beta phase evaluation
Beta phase evaluation
Issue category | Bug: database specific logic should only be present in database drivers. |
---|---|
Issue priority | Major because: this breaks automated testing on SQL Server and possibly other database engines. |
Unfrozen changes | Unfrozen because it just moves a piece of logic into the database driver, so specific driver can override it. |
Disruption | Non disruptive: it just moves a piece of logic into the database drivers, where it belongs. Current implementation of that logic is kept exactly the same. |
Proposed resolution
Remaining tasks
User interface changes
API changes
Comments
Comment #1
david_garcia CreditAttribution: david_garcia commentedAdded schema information to the fully qualified name + moved the logic into the Schema class.
If it does not pass tests in mySQL, at least we should move the logic to the Schema class so that database drivers can have their own implementation.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedDavid, this actually looks correct. What Drupal calls a
database
is actually a schema for PostgreSQL, i.e. a namespace that allows cross-namespace querying. Drupal doesn't have any knowledge of the PostgreSQL concept of database, and it doesn't have to. Multiple PostgreSQL databases map to multiple connections.Comment #4
david_garcia CreditAttribution: david_garcia commentedI don't want to be rude reopening this, but at least wan't to have it clear before doing so.
This issue is arising on SQL Server, not Postgre, during D6 migration tests execution.
Currently the database driver (drupal, not specific for any driver) manages 3 concepts:
['database']: this is supposed to be the "name" of the database.
['schema']: a database can have multiple schemas, defaults to dbo.
['prefix']: a prefix to append to tables to void collisions when running multiple things inside the same database.
In SQL Server, the correct way of globaly targeting a specific table in a database would be: database.schema.table (where table has the embedded prefix from drupal).
In SQL Server:
The issue here isn't wether or not the FullyQualifiedName is properly generated (it does for mySQL), what I am trying to say is that the FullQualifiedName generated inside getQualifiedMapTableName() is not working on SQL Server because it needs the SCHEMA between database and table name, and that Drupal should delegate any database specific handling (such as generating a full qualified name) to the specific database drivers.
So I am proposing to move that logic into the Schema class, so that specific database drivers can override it if necessary.
I confess to not have properly honored the original implementation, in an attempt to generate something that was mySQL and SQL Server friendly. But tests clearly show I failed at that.
So, at least that logic can be moved into the Schema class to allow specific driver overriding.
Comment #5
david_garcia CreditAttribution: david_garcia commentedComment #6
Damien Tournoud CreditAttribution: Damien Tournoud commented@david_garcia that actually makes sense.
^ Please document the parameters and return value, and add a @see to
getPrefixInfo
.^ This should be
getFullyQualifiedTableName
^ The default implementation should be simply
return $table_info['schema'] . '.' . $table_info['table']
. You can override it in the SQL Server driver (but you probably actually have to, except if you want to support cross-database distributed queries).Comment #8
david_garcia CreditAttribution: david_garcia commented@Damien Tournoud
I know that the number of sites that will ever directly benefit from this change is exactly 0 (unless D7 to D8 migration will use cross database queries), but this change is needed in order too pass D6 migration tests in core with SQL Server.
We need to move any database specific logic into the drivers to be able to say that Drupal is truly database agnostic.
I cannot believe the patch failed because the stub database driver is not supporting getPrefixInfo!!
Rewrote the patch to not depend on that method.
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commented@david_garcia: please just add getPrefixInfo to the stub driver. Your current code is just incorrect.
The default implementation needs to be exactly this::
Comment #11
david_garcia CreditAttribution: david_garcia commented@Damien Tournoud: I think your proposed code is also broken, the original implementation is using 'database' (from connection options) instead of 'schema'.
I also agree that the stub schema should have the getPrefiInfo implemented, but I don't feel comfortable adding it because whoever wrote the stub schema class decided to leave nearly 95% of it's methods throwing a Not Supported exception, and I must confess as to not having a clue of what the stub schema is being used for.
Looks like last patch failed because at some point in the tests there is a Schema instance that does not have it's internal $connection property initialized, and the getQualifiedMapTableName depends on information provided by the connection itself (getConnectionOptions).
I'll wait to see if I can get my local testing to work, it's been broken for a while now and I am working blindfolded, combine that with a 5 minute long bursts dedication to Drupal and that translates in publishing broken patches over and over again :(
Comment #14
david_garcia CreditAttribution: david_garcia commentedLooks like the stub Schema is constructed without a reference to a Connection object, it's built with the database contents themselves:
Meaning that, on the stub schema, getQualifiedMapTableName() should not be supported or be implementad on such a way that it will not depend on the existence of a Connection object, as it depends on connection specific settings such as 'prefix', 'schema' and/or 'database'.
Comment #15
david_garcia CreditAttribution: david_garcia commentedLet's try luck, still cannot test locally.
Moved getFullQualifiedTableName to Connection, where it makes more sense along with tablePrefix, and connectionOptions.
Comment #16
david_garcia CreditAttribution: david_garcia commentedComment #17
david_garcia CreditAttribution: david_garcia commentedComment #18
benjy CreditAttribution: benjy commentedWould be good if Damien could review this again.
Comment #19
david_garcia CreditAttribution: david_garcia commentedBoosted to critical, I'm not sure it qualifies but the truth is that, without this, a D8 release will not be compatible (at least) with MS SQL because there's database specific logic out of the database driver. This bug is breaking (at least) continuous QA because it prevents core tests to pass on MSSQL.
Comment #20
dawehnerJust ensured, we don't block a release because of MS SQL support, and so its not critical ... afaik you can even override the schema class anyway for it anyway.
Comment #21
david_garcia CreditAttribution: david_garcia commentedAcknowledged. But I think the issue cannot be solved by overriding the schema class. That's the core of the problem, the "failing" logic is outside of the database abstraction layer, inside:
/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
Comment #22
benjy CreditAttribution: benjy commentedYou can replace the id_map plugin. This issue seems like something we could fix, but there could be further changes required in the future for MS SQL specific support.
See this in the Migration entity. You can set $this->idMap['plugin'] on the migration and that will change the id map plugin to be created.
Comment #23
david_garcia CreditAttribution: david_garcia commentedMakes no sense to support D6-D7 migrations in MSSQL because D6 did not support MSSQL. But sooner or later D7-D8 migrations will be there, and the sooner we start fixing this small issues, less problems there will be latter for the D7-D8 upgrade path.
At this moment the issue is just a matter of QA where tests are not passing (yes MSSQL users can disable the broken tests) but no matter how "mucho" simple the justification for disabling them is, the QA Audit guys don't like having tests disabled, specially if they are from core.
And overally, the solution to this issue is just shifting three lines of code to the place they belong: the database abstraction layer! Then MSSQL driver can override it to it's needs.
The chance of breaking anything with the patch in #15 is 0% because the code just just been moved from one place to another, the logic has not changed.
Comment #24
Crell CreditAttribution: Crell commentedSomeone in IRC pinged me about this issue. The general changes here seem reasonable at first glance. Database-specific logic belongs in database-specific classes. Is there any related cleanup that can/should be done at the same time? Do we need an overridden version of the new method for any of the core-supported databases?
Comment #25
david_garcia CreditAttribution: david_garcia commented1. I spent some time a few months ago testing (and implementing) the MSSQL driver for D8, I found a few things that needed fixes in core (some are already commited by now), but this was the only "database logic out of database layer" issue.
2. The issue shows up on automated core migration tests. I believe these tests must have been run on core supported databases, and if no one has complained then they are probably passing. But because the logic has not changed, if it didn't work before it still will not work, and if it did, it will continue working.
Thanks for bringing attention into this, fighting the MSSQL front on core feels like swimming upstream.
Comment #26
benjy CreditAttribution: benjy commentedI think we could just remove this, then RTBC for me.
Would be good to checkout the Postgres test results after this goes in. #2160433: [policy, no patch] Move PostgreSQL driver support into contrib
Comment #27
david_garcia CreditAttribution: david_garcia commentedComment #28
benjy CreditAttribution: benjy commentedOK Great, lets do it.
Comment #29
alexpottThis issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed b79c650 and pushed to 8.0.x. Thanks!