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:

  1. With no 'migrate' connection defined and no migrate.fallback_state_key set, you die with a ConnectionNotDefinedException for the 'migrate' connection, per above.
  2. 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.
  3. 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

  1. Remove fallback_state_key and the 'migrate' connection fallback from SqlBase.
  2. Implement hook_migration_plugins_alter() for BC.
  3. Have migrate_drupal_ui inject database_state_key to source plugins instead of using the global fallback_state_key.
  4. 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

Comments

mikeryan created an issue. See original summary.

mikeryan’s picture

Title: Better handle unconfigured migration source plugins » Make MigratePluginManager::getDefinitions() work cleanly
Status: Active » Needs review
StatusFileSize
new2.45 KB

Retitling 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.

Status: Needs review » Needs work

The last submitted patch, 2: better_handle-2700693-2-FAIL.patch, failed testing.

mikeryan’s picture

Status: Needs work » Needs review
StatusFileSize
new4.7 KB

OK, catching the connection exceptions in the node derivers certainly seems to help...

Status: Needs review » Needs work

The last submitted patch, 4: make-2700693-4.patch, failed testing.

mikeryan’s picture

Looking good - the remaining failure here should be fixed by #2560795: Source plugins have a hidden dependency on migrate_drupal.

mikeryan’s picture

Status: Needs work » Postponed
mikeryan’s picture

Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: make-2700693-4.patch, failed testing.

mikeryan’s picture

Issue tags: +Needs reroll
Pradnya Pingat’s picture

Issue tags: -Needs reroll

The latest patch applies cleanly, Removing reroll tag

andypost’s picture

+++ b/core/modules/node/src/Plugin/migrate/D6NodeDeriver.php
@@ -129,6 +130,10 @@ public function getDerivativeDefinitions($base_plugin_definition) {
+    catch (ConnectionNotDefinedException $e) {
+      // We may be invoked by general plugin discovery when there is no D6
+      // migration configured.

+++ b/core/modules/node/src/Plugin/migrate/D7NodeDeriver.php
@@ -117,6 +118,10 @@ public function getDerivativeDefinitions($base_plugin_definition) {
+    catch (ConnectionNotDefinedException $e) {
+      // We may be invoked by general plugin discovery when there is no D6
+      // migration configured.

Better to log this error to easy track down

ilya.no’s picture

Status: Needs work » Needs review
StatusFileSize
new2.25 KB

Rerolled.

andypost’s picture

Assigned: Unassigned » mikeryan

@mikeryan what about #12

andypost’s picture

StatusFileSize
new4.7 KB

proper re-roll

Status: Needs review » Needs work

The last submitted patch, 15: make-2700693-15.patch, failed testing.

chx’s picture

mikeryan’s picture

Status: Needs work » Postponed

Yes, 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.

mikeryan’s picture

Status: Postponed » Needs work

Unblocked - first thing, I'll submit a fresh fail test to demonstrate the problem.

mikeryan’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review
StatusFileSize
new1.14 KB

This will fail, complaining that there's no connection named 'migrate'.

Status: Needs review » Needs work

The last submitted patch, 20: make-2700693-20-FAIL.patch, failed testing.

mikeryan’s picture

So, 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:

  1. With no 'migrate' connection defined and no migrate.fallback_state_key set, you die with a complaint about no 'migrate' connection, per above.
  2. 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.
  3. 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 the Drupal 7 migrations. And of course vice versa.

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?

mikeryan’s picture

Title: Make MigratePluginManager::getDefinitions() work cleanly » Make MigratePluginManager::getDefinitions() work cleanly with migrate_drupal enabled
mikeryan’s picture

Issue summary: View changes
mikeryan’s picture

Priority: Normal » Major
Issue tags: +Migrate critical

Discussed on the weekly call, and making this a Migrate-critical. Another angle - blocker for #2796779: drush migrate-import should make use of config-override.

heddn’s picture

Issue tags: +Migrate BC break
mikeryan’s picture

Issue tags: -Migrate BC break

The approach I'm working on should maintain BC - if it turns out we need a break, we can add the tag at that time.

ckaotik’s picture

I was wondering ... Couldn't we use Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase::checkRequirements for 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 ;)

The last submitted patch, 20: make-2700693-20-FAIL.patch, failed testing.

mikeryan’s picture

Status: Needs work » Needs review
StatusFileSize
new8.59 KB

I'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:

  1. Fixes the database configuration prioritization, so now explicit database configuration in the source plugin overrides the global fallback_state_key.
  2. If we fallback all the way to trying a 'migrate' connection, and there is none, we'll now throw a RequirementsException (previously this would throw a ConnectionNotDefinedException).
  3. Inspired by ckaotik's comment, SqlBase now implements checkRequirements() and calls getDatabase() so the RequirementsException will be thrown if no database is configured (and DrupalSqlBase calls parent::checkRequirements() to inherit this behavior).
  4. The node derivers, which previously would blow everything up in getDefinitions() if there was no database connection configured, now call the d6_node_type/d7_node_type checkRequirements() and gracefully return without generating any derivatives if it catches RequirementsException.

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.

Status: Needs review » Needs work

The last submitted patch, 30: make-2700693-30.patch, failed testing.

mikeryan’s picture

Title: Make MigratePluginManager::getDefinitions() work cleanly with migrate_drupal enabled » [meta] Make MigratePluginManager::getDefinitions() work cleanly with migrate_drupal enabled

I think it's better to break this down into manageable chunks - turning this into a meta, I'll create child issues.

mikeryan’s picture

Issue tags: -Migrate critical

Child 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.

saltednut’s picture

Using 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.5

https://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:

message 'No database connection configured for source plugin
d7_taxonomy_vocabulary' in
/home/travis/build/acquia/df/docroot/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php:189
Stack trace:
#0
/home/travis/build/acquia/df/docroot/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php(140):
Drupal\migrate\Plugin\migrate\source\SqlBase->setUpDatabase(Array)
#1
/home/travis/build/acquia/df/docroot/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php(212):
Drupal\migrate\Plugin\migrate\source\SqlBase->getDatabase()
#2
/home/travis/build/acquia/df/docroot/core/modules/taxonomy/src/Plugin/migrate/source/d7/Vocabulary.php(21):
Drupal\migrate\Plugin\migrate\source\SqlBase->select('taxonomy_vocabu...',
'v')
#3
/home/travis/build/acquia/df/docroot/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php(222):
Drupal\taxonomy\Plugin\migrate\source\d7\Vocabulary->query()
#4
/home/travis/build/acquia/df/docroot/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php(250):
Drupal\migrate\Plugin\migrate\source\SqlBase->prepareQuery()
#5
/home/travis/build/acquia/df/docroot/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php(244):
Drupal\migrate\Plugin\migrate\source\SqlBase->initializeIterator()
#6
/home/travis/build/acquia/df/docroot/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php(286):
Drupal\migrate\Plugin\migrate\source\SourcePluginBase->getIterator()
#7
/home/travis/build/acquia/df/docroot/core/modules/taxonomy/src/Plugin/migrate/D7TaxonomyTermDeriver.php(88):
Drupal\migrate\Plugin\migrate\source\SourcePluginBase->rewind()
#8
/home/travis/build/acquia/df/docroot/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php(100):
Drupal\taxonomy\Plugin\migrate\D7TaxonomyTermDeriver->getDerivativeDefinitions(Array)
#9
/home/travis/build/acquia/df/docroot/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php(86):
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDerivatives(Array)
#10
/home/travis/build/acquia/df/docroot/core/modules/migrate/src/Plugin/MigrationPluginManager.php(251):
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDefinitions()
#11
/home/travis/build/acquia/df/docroot/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php(175):
Drupal\migrate\Plugin\MigrationPluginManager->findDefinitions()
#12
/home/travis/build/acquia/df/docroot/core/modules/migrate/src/Plugin/MigrationPluginManager.php(150):
Drupal\Core\Plugin\DefaultPluginManager->getDefinitions()
#13
/home/travis/build/acquia/df/docroot/core/modules/migrate/src/Plugin/MigrationPluginManager.php(103):
Drupal\migrate\Plugin\MigrationPluginManager->expandPluginIds(Array)
#14
/home/travis/build/acquia/df/docroot/core/modules/migrate/src/Plugin/MigrationPluginManager.php(89):
Drupal\migrate\Plugin\MigrationPluginManager->createInstances(Array,
Array)
#15
/home/travis/build/acquia/df/docroot/profiles/df/modules/contrib/scenarios/src/ScenariosHandler.php(219):
Drupal\migrate\Plugin\MigrationPluginManager->createInstance('import_dfs_tec_...')
#16
/home/travis/build/acquia/df/docroot/profiles/df/modules/contrib/scenarios/src/ScenariosHandler.php(299):
Drupal\scenarios\ScenariosHandler->processMigrations('import', Array,
'@self')
#17
/home/travis/build/acquia/df/docroot/profiles/df/modules/contrib/scenarios/scenarios.drush.inc(66):
Drupal\scenarios\ScenariosHandler->scenarioEnable('dfs_tec')
#18 [internal function]: drush_scenarios_enable_scenario('dfs_tec')
#19
/home/travis/build/acquia/df/vendor/drush/drush/includes/command.inc(371):
call_user_func_array('drush_scenarios...', Array)
#20
/home/travis/build/acquia/df/vendor/drush/drush/includes/command.inc(222):
_drush_invoke_hooks(Array, Array)
#21 [internal function]: drush_command('dfs_tec')
#22
/home/travis/build/acquia/df/vendor/drush/drush/includes/command.inc(190):
call_user_func_array('drush_command', Array)
#23
/home/travis/build/acquia/df/vendor/drush/drush/lib/Drush/Boot/BaseBoot.php(67):
drush_dispatch(Array)
#24
/home/travis/build/acquia/df/vendor/drush/drush/includes/preflight.inc(66):
Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#25 /home/travis/build/acquia/df/vendor/drush/drush/drush.php(12):
drush_main()
#26 {main}

The failed test run is publicly available here: https://travis-ci.org/acquia/df/jobs/189320740#L1437

mikeryan’s picture

@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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

Revisiting 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:

 Group: Default (default)    Status  Total  Imported  Unprocessed  Last imported
 block_content_type          Idle    1      0         1
 block_content_body_field    Idle    1      0         1
 user_picture_field          Idle    1      0         1
 d6_upload_field             Idle    1      0         1

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...

andypost’s picture

Version: 8.3.x-dev » 8.5.x-dev

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

heddn’s picture

Re #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.

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.

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.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

quietone’s picture

Status: Needs work » Closed (outdated)
Issue tags: +Bug Smash Initiative

Time 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.