Problem/Motivation
Contrib migration tools would like to discover working migration plugins, something like:
$manager = \Drupal::service('plugin.manager.migration');
$plugin_definitions = $manager->getDefinitions();
foreach ($plugin_definitions as $plugin_id => $definition) {
try {
$migration = $manager->createInstance($plugin_id, $definition);
$migration_list[$plugin_id] = $migration;
}
catch (PluginNotFoundException $e) {
// Ignore migrations that are not fully configured.
}
}
When the migrate_drupal module is enabled and no connection is defined for the drupal migrations, however, we get ConnectionNotDefinedException from getDefinitions(), and thus get no migrations back (even the ones that are fine). Even if we had a connection defined, it would only be applicable to a subset of migrations (those for the Drupal version in the source database).
These are the following broken scenarios when trying to getDefinitions() with migrate_drupal enabled:
- With no 'migrate' connection defined and no migrate.fallback_state_key set, you die with a ConnectionNotDefinedException for the 'migrate' connection, per above.
- If you have a 'migrate' connection (or migrate.fallback_state_key) pointing to a non-Drupal 6/7 database (i.e., you're presumably trying to migrate some other data into D8), you will (I believe) retrieve all core Drupal migrations without error, but they'll be unusable. Note that the current test of the plugin manager does this - it uses the D8 default connection as the 'migrate' connection, so it returns all core migrations (D6 and D7) but they're actually looking at a D8 database. The test only checks that one or more migrations are returned, not whether they make sense.
- If you have a 'migrate' connection (or migrate.fallback_state_key) pointing to a Drupal 6 database to do a custom D6 migration, you will retrieve all core Drupal migrations without error, including irrelevant D6 migrations and all the Drupal 7 migrations. And of course vice versa.
Proposed resolution
The main thing here, I think, will be making the SQL source plugin more robust in the face of a missing or invalid connection. Ideally, I think there should be no global defaulting - no fallback_state_key or magic 'migrate' connection. SqlBase should require explicit database information - a key pointing to a pre-existing connection (presumably from settings.php), a database_state_key pointing to state containing database info, or explicit database info in the source configuration. We'd need a mechanism (perhaps throwing RequirementsException) for the plugin manager to skip any migrations using a SqlBase source without a connection configured. migrate_drupal_ui would, instead of setting the global fallback_state_key, set database_state_key on each migration (#2681869: Provide clean way to merge configuration into migration plugins of course being helpful here).
For backwards compatibility (so anyone depending on the current fallback behavior isn't immediately broken), a hook_migration_plugins_alter() could recognize when fallback_state_key or a 'migrate' connection is set, and inject them into SqlBase source plugins which do not have explicit database configuration.
Remaining tasks
- Remove fallback_state_key and the 'migrate' connection fallback from SqlBase.
- Implement hook_migration_plugins_alter() for BC.
- Have migrate_drupal_ui inject database_state_key to source plugins instead of using the global fallback_state_key.
- Tests: no database connection provided, non-Drupal database provided, D6 database provided (avoid D7 migrations), and vice versa.
User interface changes
N/A
API changes
SqlBase fallbacks to fallback_state_key and 'migrate' connection removed.
Data model changes
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #30 | make-2700693-30.patch | 8.59 KB | mikeryan |
| #20 | make-2700693-20-FAIL.patch | 1.14 KB | mikeryan |
| #15 | make-2700693-15.patch | 4.7 KB | andypost |
| #2 | better_handle-2700693-2-FAIL.patch | 2.45 KB | mikeryan |
Comments
Comment #2
mikeryanRetitling to not assume a solution, but focus on the problem being tested...
Fail tests added - one to test without migrate_drupal enabled, and one to with it enabled.
The first test fails with "The service definition "plugin.manager.migrate.cckfield" does not exist." - a fix for that is in #2560795: Source plugins have a hidden dependency on migrate_drupal.
The second test fails with "The specified database connection is not defined: migrate" - that will be the immediate focus here.
Comment #4
mikeryanOK, catching the connection exceptions in the node derivers certainly seems to help...
Comment #6
mikeryanLooking good - the remaining failure here should be fixed by #2560795: Source plugins have a hidden dependency on migrate_drupal.
Comment #7
mikeryanComment #8
mikeryanComment #10
mikeryanComment #11
Pradnya Pingat commentedThe latest patch applies cleanly, Removing reroll tag
Comment #12
andypostBetter to log this error to easy track down
Comment #13
ilya.no commentedRerolled.
Comment #14
andypost@mikeryan what about #12
Comment #15
andypostproper re-roll
Comment #17
chx commentedIs this not postponed on #2560795: Source plugins have a hidden dependency on migrate_drupal ?
Comment #18
mikeryanYes, we do need to get #2560795: Source plugins have a hidden dependency on migrate_drupal fixed first.
@andypost: It would make sense to log it if it reflected an actual error condition - however, the base problem here is that the exception is thrown in the perfectly normal circumstance of having migrate_drupal enabled without having (yet) configured migration from a specific source Drupal system.
Comment #19
mikeryanUnblocked - first thing, I'll submit a fresh fail test to demonstrate the problem.
Comment #20
mikeryanThis will fail, complaining that there's no connection named 'migrate'.
Comment #22
mikeryanSo, there's a lot more to this than my original approach of just trying to get the Drupal 6/7 node derivers to stop complaining about 'migrate'. When trying to getDefinitions() with migrate_drupal enabled:
Ideally, I think there should be no global defaulting - no fallback_state_key or magic 'migrate' connection. SqlBase should require explicit database information - a key pointing to a pre-existing connection (presumably from settings.php), a database_state_key pointing to state containing database info, or explicit database info in the source configuration. We'd need a mechanism (perhaps throwing RequirementsException) for the plugin manager to skip any migrations using a SqlBase source without a connection configured. migrate_drupal_ui would, instead of setting the global fallback_state_key, set database_state_key on each migration (#2681869: Provide clean way to merge configuration into migration plugins of course being helpful here).
The thing is, this would be a significant BC break - anyone currently depending on that 'migrate' fallback would break if it were taken out. And I bet a lot of people are using that connection, precisely because they ran into the present issue and the message seemed to be strongly hinting they should be... I've been trying very hard to get at least the migrate module (the basic migration API) stable - up to experimental beta level - with 8.2.x, and it seems too late to pursue this approach for that deadline.
Thoughts?
Comment #23
mikeryanComment #24
mikeryanComment #25
mikeryanDiscussed on the weekly call, and making this a Migrate-critical. Another angle - blocker for #2796779: drush migrate-import should make use of config-override.
Comment #26
heddnComment #27
mikeryanThe approach I'm working on should maintain BC - if it turns out we need a break, we can add the tag at that time.
Comment #28
ckaotikI was wondering ... Couldn't we use
Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase::checkRequirementsfor that and directly check them during or after instantiation? The plugin definitions would still be available to any interested party, while only instances that have fulfilled requirements can be used for importing.I understand that this change would lead to a shift in how the definitions and plugins may be used but at least to me it would make more sense ;)
Comment #30
mikeryanI'm working on a fuller work-up on the issues and trade-offs around here, but for the moment here's a modest patch which helps the tools not at least blow up. This patch:
This is not a perfect solution - the tools aren't going to be able to fully protect people from some ugliness if they choose to, say, enable migrate_drupal and define a connection named 'migrate' - but more on that in a bit. Not ready to get this RTBC'd, but feedback on what's been done here welcome.
To try this out with the contrib tools, you'll also need the migrate_plus patch at #2752335: Properly integrate configuration-entity-based migrations with the core plugin manager and the migrate_tools patch at #2795447: Use the core plugin manager.
Comment #32
mikeryanI think it's better to break this down into manageable chunks - turning this into a meta, I'll create child issues.
Comment #33
mikeryanChild issues #2830031: Fix SqlBase fallback priorities and document them and #2830036: MigrationPluginManager::getDefinitions() blows up in node derivers created, which cover the aspects I've addressed in my last patch.
Removing the migrate-critical tag - the critical part of this is now separated out into #2830036: MigrationPluginManager::getDefinitions() blows up in node derivers, which is thusly tagged.
Comment #34
saltednutUsing both patches referenced in the child issues but we are still seeing issues (
"exception 'Drupal\Core\Database\ConnectionNotDefinedException' with message 'The specified database connection is not defined: migrate'") when attempting to load migrations as of 8.2.5https://www.drupal.org/files/issues/2830036-17.patch
#2830036: MigrationPluginManager::getDefinitions() blows up in node derivers
https://www.drupal.org/files/issues/fix_sqlbase_fallback-2830031-5.patch
#2830031: Fix SqlBase fallback priorities and document them
Also patching migrate_tools and migrate_plus
https://www.drupal.org/files/issues/properly_integrate-2752335-33.patch
#2752335: Properly integrate configuration-entity-based migrations with the core plugin manager
https://www.drupal.org/files/issues/use_the_core_plugin-2795447-3.patch
#2795447: Use the core plugin manager
We filed #2841437: Do not scan the migration_templates directory in the MigrationPluginManager but after talking to @Heddn we were thinking the issue is duplicate perhaps?
That said, the patch from this issue does solve our problems. Though it was noted that the BC layer should not be removed, perhaps there is some other check that is missing?
The specific error dump is not particularly helpful for us, but this is it:
The failed test run is publicly available here: https://travis-ci.org/acquia/df/jobs/189320740#L1437
Comment #35
mikeryan@brantwynn: Looks like #2746671: CCK field data not available for D7 taxonomy term migrations (committed since the last patch at #2830036: MigrationPluginManager::getDefinitions() blows up in node derivers) breaks things, the deriver introduced in that patch needs to do the same as the node deriver does. Following up at https://www.drupal.org/node/2830036#comment-11855267.
Comment #37
mikeryanRevisiting this world as I approach finally getting the contrib modules (#2752335: Properly integrate configuration-entity-based migrations with the core plugin manager and #2795447: Use the core plugin manager taking advantage of the work here, I will point out one problem not yet solved, and I'm not sure how to solve it. Now that we're using the core plugin manager to pull in both straight plugin configs and configuration entities, with migrate_drupal installed but no Drupal migrations configured, some Drupal migrations are available:
The problem here is that these migrations have embedded_data sources rather than SQL sources. We're able to skip the other unconfigured migrations because they throw RequirementsException (they have SQL-based sources but no database configured), but these are complete in themselves. Indeed, they'll run fine - but you don't really want to run them unless you're running them in the context of a Drupal migration. I don't really see how a general-purpose tool can look at one of these migrations and know whether or not it's relevant - if it should be exposed.
*This* is why I always liked the idea of explicitly registering the migrations you want to use...
Comment #38
andypostComment #40
heddnRe #37: per #2225587-123: Migrate D6 i18n menu links, we should switch all those md_empty and embedded_data source to instead use DrupalSqlBase so we can check if the source module(s) are enabled. This is only for discovery. And all d6/d7 source plugins should be doing this type of requirements check, no? If the source module isn't there, then fail the source and fail the discovery.
Each of the source plugins for the migration should check requirements and throw an exception if the source module it needs to be discovered is not enabled in the source db.
Comment #46
quietone commentedTime to see if there is still work to do here.
I skimmed the issue and on reading it seemed like everything is already done here. I applied the latest patch and discovered that all the changes have already been made. Then I re-read the comments since the last patch and didn't find anything that still needs to be done.
I used git to find where some, not all, the changes were made. The issues are:
Therefor, closing as outdated.