In #2625696: Make migrations themselves plugins instead of config entities chx proposes making migrations plugins rather than configuration entities. To fully understand the implications of this change, we should see how it would affect this module.

Comments

mikeryan created an issue. See original summary.

mikeryan’s picture

Status: Active » Needs work
FileSize
5.35 KB

OK, first pass at updating migrate_tools (well, the drush commands) to work with the migrations-as-plugins patch. Replaced some Migration::load($migration_id) calls with \Drupal::service('plugin.manager.migration')->createInstance($migration_id), straightforward enough. The main thing was to replace the EntityQuery to obtain migration configuration entities with plugin discovery to get migration plugins. The problem here is getting too many plugins, but I'll write that up in the main issue...

mikeryan’s picture

Oh, I should note that I haven't looked at the UI yet - that, being based on ConfigEntityListBuilder and EntityForm, is obviously going to need more work...

mikeryan’s picture

Version: 8.x-1.x-dev » 8.x-2.x-dev

Time to make this work for reals...

mikeryan’s picture

This isn't working at all with the plugins patch as it ended up in core

migrate_tools$ drush ms
exception 'Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException' with message 'You have requested a non-existent service      [error]
"plugin.manager.migrate.cckfield". Did you mean one of these: "plugin.manager.migration", "plugin.manager.image.effect",
"plugin.manager.migrate.source", "plugin.manager.migrate.process", "plugin.manager.migrate.destination", "plugin.manager.migrate.id_map",
"plugin.manager.views.field"?' in /Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Component/DependencyInjection/Container.php:162
Stack trace:
#0 /Users/mikeryan/Sites/8.1.x/core/modules/node/src/Plugin/migrate/D6NodeDeriver.php(65):
Drupal\Component\DependencyInjection\Container->get('plugin.manager....')
#1 /Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Core/Plugin/Discovery/ContainerDerivativeDiscoveryDecorator.php(25):
Drupal\node\Plugin\migrate\D6NodeDeriver::create(Object(Drupal\Core\DependencyInjection\Container), 'd6_node')
#2 /Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php(103):
Drupal\Core\Plugin\Discovery\ContainerDerivativeDiscoveryDecorator->getDeriver('d6_node', Array)
...

Of course, cckfield, d6_node, etc. are completely irrelevant to any non-Drupal migrations. This problem was raised at https://www.drupal.org/node/2625696#comment-10955051 and it was suggested #2681869: Provide clean way to merge configuration into migration plugins might address it. Trying to figure out a work-around in the meantime...

mikeryan’s picture

Title: POC of migrations-as-plugins » Make migrate_tools work with Drupal 8.1.x
mikeryan’s picture

Priority: Normal » Critical

  • mikeryan committed 328daf3 on 8.x-2.x
    Issue #2667368 by mikeryan: Make migrate_tools work with 8.1.x.
    
mikeryan’s picture

mikeryan’s picture

Just to note - I'm holding this open until the core patches are in, just in case there's any drift that we'll need to account for here.

willwh’s picture

Hi Mike!

So I rerolled one of the core patches you have linked above, as it wasn't applying cleanly to 8.1.x-dev.

So for anyone following along, if you want to try migrate_tools with 8.1.x-dev, you'll need the following two patches:

https://www.drupal.org/files/issues/2600926-24.patch
https://www.drupal.org/files/issues/source_plugins_have_a-2560795-44.patch

After getting these applied, and checking out/installing the latest migrate_tools and migrate_plus 8.x-2.x-dev, I'm running in to problems.

When I run drush migrate-status

I get the following output:

Argument 3 passed to Drupal\migrate_plus\Event\MigratePrepareRowEvent::__construct() must be an instance of Drupal\migrate\Entity\MigrationInterface, instance of Drupal\migrate\Plugin\Migration given, called in /home/willwh/www/drupal/modules/migrate_plus/migrate_plus.module on line 67 and defined MigratePrepareRowEvent.php:53

Taking a look at this, migrate_plus and tools are both relying on use Drupal\migrate\Entity\MigrationInterface, and this needs to be updated to use Drupal\migtrate\Plugin\MigrationInterface, as well as a number of the supporting classes.

I'm going to start working on this, but I haven't worked with the plugin manager yet. I see some examples of some of this stuff in 8.1.x-dev now, in the migrate module, so hopefully I can get this puppy to fly!

mikeryan’s picture

Strangely, this did work fine for me previously... Anyway, I've now updated the use statements and migrate_plus/migrate_tools now work fine with the latest core patches at https://www.drupal.org/files/issues/2600926-48.patch and https://www.drupal.org/files/issues/source_plugins_have_a-2560795-45.patch, thanks!

  • mikeryan committed 0bda235 on 8.x-2.x
    Issue #2667368 by mikeryan: Update use statements to reference plugins...
mikeryan’s picture

So, I've been primarily working on this from the perspective of non-Drupal-source migrations (thus migrate_drupal not enabled), testing with migrate_example. @willwh reports that things break in interesting ways with migrate_drupal enabled, but not in the context of an upgrade operation:

  1. With a 'migrate' connection in settings.php pointing at a D7 database, he gets "exception 'PDOException' with message 'SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd7.boxes' doesn't exist"
  2. Without the 'migrate' connection defined, he gets "exception 'Drupal\Core\Database\ConnectionNotDefinedException' with message 'The specified database connection is not defined: migrate'"
  3. When I try it locally with migrate_tools and migrate_drupal enabled (but no migrate_upgrade or migrate_example), I get "exception 'Drupal\Component\Plugin\Exception\PluginNotFoundException' with message 'The "d6_field_instance" plugin does not exist.'"

So, there's some work to be done here on those scenarios (how much in migrate_tools and how much in core is TBD...).

mikeryan’s picture

The tests for #2560795: Source plugins have a hidden dependency on migrate_drupal (which of course run with migrate_drupal enabled) fail badly, so fixing those is the first priority.

mikeryan’s picture

I've had no luck trying to debug the core tests, I'll try debugging the problem here and see if that reveals a common root issue.

mikeryan’s picture

All right, the annotations patch is in 8.1.x, and #2560795: Source plugins have a hidden dependency on migrate_drupal is RTBC. The next hurdle is #2700693: [meta] Make MigratePluginManager::getDefinitions() work cleanly with migrate_drupal enabled - this at least gets us to the point that getDefinitions() will return cleanly whether or not migrate_drupal is enabled.

But, following that, we still have trouble. The premise here in getting migrate-status working is that we use getDefinitions() to get a list of all migration plugins in the environment, then calling createInstance() will throw exceptions on the ones that aren't relevant to us (e.g., haven't got a database configured). That's a shaky assumption to begin with - for example, the block_content_type and block_content_body_field migrations simply create stuff out of thin air without any external data source, so we have no way of knowing we want to skip them 99% of the time and are currently explicitly excluding them. Oh, and what about the 1% of the time we might actually be interested in them?

Anyway, a second-line of filtering out migrations we don't want is checkRequirements(). The problem here though is that we can't order the migrations by dependency up front, which means we get a RequirementsException for migrations which we are interested in but whose dependencies haven't yet been created. We can't tell the difference between that and, say, migrations throwing RequirementsException because they have no database connection.

This is all just dealing with the core Drupal migration plugins - contrib modules which provide migration plugins which aren't immediately runnable (i.e., need some configuration such as a database connection) could very well provide distinct challenges.

So, the upshot is - I believe we need to have an explicit registration process for migrations, and the tools could then simply discover registered migrations rather than needing to develop an AI to figure out what migration plugins are relevant.

mikeryan’s picture

agoradesign’s picture

Hi Mike,
thanks for your great work so far.. Did you spoke with any of the core contributors, if the remaining patches can get committed before tonight's rc1, especially #2701257: Create a custom filter for attribute values... would be great!

mikeryan’s picture

@agoradesign: #2560795: Source plugins have a hidden dependency on migrate_drupal just went to RTBC a couple hours ago... alexpott appears to be done for the day (it would be evening his time), so it's not looking likely. Even if that makes it in, it's not enough to make the tools work cleanly in 8.1 - I think we're probably going to be best off pursuing #2701277: Provide a means of explicitly registering migrations to have working tools in 8.1.

mikeryan’s picture

Status: Postponed » Needs review
FileSize
3.31 KB

Here we go - the attached patch works with the migrate_plus patch at https://www.drupal.org/node/2701277#comment-11046379 to enable you to use the tools to develop your own migrations under Drupal 8.1.x with no core patches.

willwh’s picture

I've tested this patch, and it does indeed work well with the patch for migrate_plus. Custom migrations in 8.1x, great work Mike!

  • mikeryan committed 87c930a on 8.x-2.x
    Issue #2667368 by mikeryan: Use migrate_plus configuration entities to...
mikeryan’s picture

Status: Needs review » Fixed

I've gone ahead and committed this - the contrib tools were broken anyway, and this will get the new approach more visibility and review.

Status: Fixed » Closed (fixed)

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