Properly integrate configuration-entity-based migrations with the core plugin manager

Yes, looking for a better way to integrate configuration entities with the core plugin manager - I want to get fixes for #2560795: Source plugins have a hidden dependency on migrate_drupal and #2700693: [meta] Make MigratePluginManager::getDefinitions() work cleanly with migrate_drupal enabled in so the plugins in core aren't actively interfering with other migrations first though.

I do want to eliminate the separate plugin manager if I can. I think we need at least #2560795: Source plugins have a hidden dependency on migrate_drupal fixed first though. If anyone could find it in their hearts to review and perhaps RTBC that....

I am building several third party migrations to ingest massive amounts of data into Drupal. These include migrate_youtube (https://www.drupal.org/sandbox/dpalmer/2779885), migrate_instagram, and several more in the future.

One of my core requirements for these modules is to be able to dynamically define 1 to many migrations through the user interface and programmatically when triggered by say the creation of another metadata node. (in my case, I am migrating one "brand" (of private data) that creates a metadata object storing that "brand", and then it is dynamically creating youtube and instagram migrations for that particular "brand".

Fortunately, I have gotten all of this to work thanks to the help of Mike Ryan. It was suggested to look at the wordpreess_migrate to see how those migrations were registered from a UI form. I followed the same approach as the WordpressMigrationGenerator and created YouTubeMigrationGenerator's and InstagramMigrationGenerator's.

These are working well though there is one major issue. These generators use the "MigrationPluginManager" service (plugin.manager.migration) to create the instance of the migration plugin. This MigrationPluginManager stores the migration plugins available in a migration_plugins record in the cache_discovery record in its "cache backend". See the constructor and getDsicovery of MigrationPluginManager to see how this is stored there.

Unfortunately, the migrate_plus module has a MigrationConfigEntityPluginManager that extends the MigrationPluginManager that overrides the values saved by the MigrationPluginManager since it has its own getDiscovery method, but then uses the same "cache backend" from the parent MigrationPluginManager's constructor.

This caused some strange behavior:

- Creating migration through UI fails because the "plugin_id" is not found in the migration_plugins.
- Cache Rebuild (migration_plugin record cleared)
- Resubmit form, it works and creates an intance of that migration.
- Run "drush migrate-status" (migrate_tools) - this calls a migrate tools migration_lister function that uses the MigrationConfigEntityPluginManager service. This then overrides the migration_plugins values in the cache_discovery "cache backend"
- Try to create another migration through the UI, and it fails again. Rinse & Repeat.

I was able to resolve this issue by overriding the constructor in the "MigrationConfigEntityPluginManager" and defining its own, unique cache backend. Now, I can dynamically create my migrations without clearing cache, the migration_plugins cache backend stores all the plugins, and the new "migrations" cache backend stores all the migrations. It works really well. I do believe that migrations and migration_plugins should be stored seperately.

Perhaps I overlooked something, but this was the only way I could get this to work.

I have a patch of my changed code. Please review.

Thanks,
Donovan

Berdir, my process plugins are still working just fine with the separate cache backend.

Just to point out - my plan is to revisit the plugin manager situation and come up with a better means of integrating migration configuration entities with the core plugin manager (ideally scrapping the migrate_plus-specific plugin manager entirely) once #2560795: Source plugins have a hidden dependency on migrate_drupal and #2700693: [meta] Make MigratePluginManager::getDefinitions() work cleanly with migrate_drupal enabled are fixed in core.

> Unfortunately, the migrate_plus module has a MigrationConfigEntityPluginManager

I do not know why. I never knew. I had the config deriver coded and posted in #2625696-66: Make migrations themselves plugins instead of config entities and suggested it to be used. It still could be used. There's no need to wait for the unicorn issue.

Forgot about that - I actually have been thinking about derivers for this anyway, I'll look at what was in that patch. We do still need to have the core plugin manager issues resolved though, because the tools (drush migrate-status, web dashboard) blow up simply trying to get a list of migrations from the core plugin manager. That problem was what drove me to the alternate plugin manager approach.

Although this began with the cache aspect of the dual-plugin-manager situation, the scope of this issue is clearly at this point the overall fragility of that situation. Core issues #2560795: Source plugins have a hidden dependency on migrate_drupal and #2700693: [meta] Make MigratePluginManager::getDefinitions() work cleanly with migrate_drupal enabled have been impediments to a proper fix, but fortunately a fix to the first one has now been committed (for 8.2.x and up). I plan on working on this shortly, hand-in-hand with working out an answer to the second core issue.

OK, I've got a preliminary patch which uses the core plugin manager as-is (thus enabling both pure migration plugins and plugins based on configuration entities to work with migrate_tools), with the following caveats:

1. This works with 8.2.x only. Before committing, I will be cutting an 8.x-3.x branch compatible with 8.2.x and above, and making the 8.x-2.x branch require 8.1.x.
2. Until #2796953: [regression] Plugins extending from classes of uninstalled modules lead to fatal error during discovery is fixed, to make this work at all you need the attached core hack (which will probably break some admin forms).
3. Until #2700693: [meta] Make MigratePluginManager::getDefinitions() work cleanly with migrate_drupal enabled is fixed, this will not work when migrate_drupal is enabled.

Next steps: fix those two core bugs.

Actually, this version sets the .info.yml dependencies to enforce #1 above...

https://www.drupal.org/files/issues/core-hack-17.patch seems to fix the issue that I needed migrate_drupal as a dependency.

Hey Mike,

could you please clarify one thing:

Is that change needed to be able to run migrations on 8.2, or is it just an improvement, that relies on 8.2?

I've currently two projects on 8.1 with migrate_plus and migrate_tools enabled, both having custom CSV/XML based imports.

If I'd update core to 8.2, without patching Migrate Plus, would the import configurations still work?

Many thanks in advance,
Andreas

Just rerolled the patch.
The test fails were due to the changed dependency name (old: system + migrate, new:drupal:migrate).

8.x-3.x branch, compatible with Drupal 8.2.x and above, is now open. I'll be returning focus to this issue shortly, in particular working on it in conjunction with #2700693: [meta] Make MigratePluginManager::getDefinitions() work cleanly with migrate_drupal enabled.

Updated the patch, including modifying migrate_example to demonstrate how the tools will be able to handle both core configuration-based migrations, and migrate_plus configuration-entity-based migrations.

I'm still struggling with #2700693: [meta] Make MigratePluginManager::getDefinitions() work cleanly with migrate_drupal enabled, so with this patch you will get a lot of noise if you have migrate_drupal enabled.

Hi Mike,

I have my dynamic migrations working with 8.2.0 of drupal and 8.x-3.x of migrate_plus with this patch. I am experiencing some strange behavior with the "migration template" though. It is recognizing it as a migration, rather than a template. When I dynamically create a migration using that template, the migration config deriver works well and finds my migration template and creates a migration with this settings. The problem is the migration template is showing up as a migration.

I guess the easiest way to explain this is to show my drush migrate-status output:

$ drush migrate-status
 Group: YouTube Migrations (youtube)  Status  Total  Imported  Unprocessed  Last imported
 youtube_videos                       Idle    N/A    0         N/A
 YoutubeMigrationKendrickLamarVevo    Idle    46     46        0            2016-11-07 20:28:41
 YoutubeMigrationSelenaGomezVevo      Idle    110    110       0            2016-11-07 20:28:53

youtube_videos is my migration_template and the other YouTubeMigrations were created through the UI and use that template, and...work. :)

Anyways, how can I keep my migration template config from showing up as a migration?

Thanks for all the work on this Mike!

Hey @dpalmer,

Have you supplied the deriver key in your original template? An example my_plugin_id.yml:

id: my_plugin_id
label: My plugin label
deriver: Drupal\my_module\Plugin\migrate\MyDeriverClass

migration_dependencies: {}

source:
  plugin: my_source_plugin
destination:
  plugin: entity:my_entity_type

process:
  field_new: field_old

If your problem persists, can you post the relevant files on pastebin or the likes?

Hey @ckaotik,

I am actually not using a "deriver" currently.

Here is a public version of my module "migrate_youtube" https://www.drupal.org/sandbox/dpalmer/2779885.

The key parts to look for are the ./migration_templates/youtube_videos.yml, ./src/YouTubeMigrationGenerator.php, ./src/Form/MigrateYouTubeSource.php, ./src/Plugin/migrate/source/YouTube.php, ./src/Plugin/migrate_plus/data_fetcher/YouTubeFetcher.php, ./src/Plugin/migrate_plus/data_parser/YouTubeParser.php.

You can add 1 to many youtube migration sources through the UI that use the YouTubeMigrationGenerator that is used by my other migration. (I have one migration, dynamically creating youtube, instagram, spotify, bands in town, and twitter migrations for each brand it migrates, essentially 1 migration, dynamically creating 0-5 migrations for thousands of brands).

You'll need a youtube client api key to try the youtube one.

Here is an example of the code used to dynamically generate the migrations:

if (isset($values['field_youtube_username']) && $values['field_import_from_youtube']) {
    if (!empty($values['field_youtube_username'][0]['value']) && $values['field_youtube_username'][0]['value'] != ' ') {
      $configuration = [];
      $configuration['author'] = 1;
      $configuration['youtube_channel'] = $values['field_youtube_username'][0]['value'];
      $configuration['artist_term'] = _umg_brand_get_vocabulary_term($values['title'][0]['value'], 'artists');
      $generator = new YouTubeMigrationGenerator($configuration);
      $generator->createMigrations();
    }
  }

This is a similar approach to what was done in https://www.drupal.org/project/wordpress_migrate.

Once you enable the module (only dependencies are migrate_plus migrate_tools and field_group) and add your api key. If you run drush ms you'll see it trying to run the migration for the template. It was not doing this in 8.x-1.x. Once you add a source or 2 through the UI, those work fine but the template still causes some issues.

Thanks!
Donovan

Here's an updated patch - to really use it you'll need the latest migrate_tools patch at #2795447: Use the core plugin manager (to be uploaded momentarily) and my next core patch at #2700693: [meta] Make MigratePluginManager::getDefinitions() work cleanly with migrate_drupal enabled (to be uploaded... less immediately).

@dpalmer: It's important to note that the migrations defined in the migration_templates folder are not (officially) "templates" any more - the folder name is a holdover from when they were templates. On the other hand, if you're putting "incomplete" migrations in there (migrations requiring more configuration than is in the .yml file, such as database connections), they kind of function like templates, and that's how you're using them here. At any rate, the point we're driving at here is to let the tools handle both kind of migrations - those defined as configuration entities, and those defined in the 'migrations' or 'migration_templates' folders. And the challenge I'm working on here and in #2700693: [meta] Make MigratePluginManager::getDefinitions() work cleanly with migrate_drupal enabled is to distinguish those latter types of migrations between those that are fully configured and manageable by the tools, and those which require further configuration and should be ignored by the tools. The approach I'm working on, which should deal with the bulk of these cases (notably most of the core Drupal-to-Drupal migrations) is that the "template" migrations should throw RequirementsException from their source plugin if they are, say, missing a database connection - the migrate_tools patch linked above will catch that and ignore the migration that threw it. For this to work will require the core patch, which will throw that exception when it tries to fallback to the 'migrate' connection and there is no such connection.

So, if your youtube_videos migration generates the configuration entities by adding a database connection to them, you should get what you want when the dust settles.

Well, I've played with that test, and I have no idea why it's failing - my best guess it has to do with the fact that configuration-entity-based migrations are now obtained via a deriver, and maybe derivations are cached somewhere that doesn't get invalidated properly...

With #2830036: MigrationPluginManager::getDefinitions() blows up in node derivers fixed, we can proceed with making this change in migrate_plus. The issue now is timing - that fix will appear in 8.2.6/8.3.0. I really didn't want to create yet another branch (8.x-4.x) of migrate_plus, so I want to see if I can figure out a way for this to take advantage of that fix when present but fall back to the current behavior if it's not.

8.3.0 and soon 8.3.1 is now released. Let's see if we can sprint on this in Baltimore.

The necessary core support to do this is in Drupal 8.3.x - so, I've opened an 8.x-4.x branch which will be compatible with core 8.3.x and above, and the 8.x-3.x branch will be only compatible with 8.2.x.

Thanks @mikeryan, setting status to re-test

Re-roll, it applied, but couldn't restart the test on 4.x

Thanks Joel, but I'm on it... Patch coming.

Not sure why the upload form won't let me test with 8.x-4.x...

git add FTW.

Found one small but tricky thing.

+++ b/src/Plugin/MigrationConfigDeriver.php
@@ -0,0 +1,27 @@
+    if (empty($this->derivatives)) {

This should be !isset, because if there are no config entities in the system, $this->derivatives will be an empty array, which will pass the empty() check and run this code over and over and over.

@phenaproxima: Actually, it does need to be empty(), because DeriverBase initializes it as [] - using isset(), we will never ever do the derivatives.

Back to the previous version of the patch.

Let's try reuploading the right patch, shall we?

1) Drupal\Tests\migrate_plus\Kernel\MigrationConfigEntityTest::testCacheInvalidation
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-Label B
+Label A

/Users/mikeryan/Sites/8.3.x/modules/contrib/migrate_plus/tests/src/Kernel/MigrationConfigEntityTest.php:48

So, what we're testing is that if we create and save a migration plugin defined with a config entity, modify and save it, and do a getDefinition() from the plugin manager, we get the change. That's not working, we're getting the original value - even if we do clearCachedDefinitions() first. The configuration entity has the right value, but the plugin derived from it does not get updated. This is what we expect to be doing this for us:

  protected function invalidateTagsOnSave($update) {
    parent::invalidateTagsOnSave($update);
    Cache::invalidateTags(['migration_plugins']);
  }

but it's somehow not working - the original plugin values still seem to be cached... somewhere.

Any thoughts?

@joelpittet asked me to take a look at this. I checked out the code, and this is what I noticed:

1. It's possible that the MigrationPluginManager is not working correctly.
2. \Drupal\Tests\migrate_plus\Kernel\MigrationConfigEntityTest is a KernelTestBase… which means that it's possible that the plugin manager it's using gets out of sync with the one that's in the container.

OK, I've figured out the cause, it's the deriver ($this->derivatives) which is retaining the original definitions. Ideally Migration::invalidateTagsOn*() would decache them, but I don't see a way to get at it (if MigrationPluginManager::getDiscovery() were public instead of protected, maybe)...

My inclination at the moment, unless someone has a suggestion for getting around this without a change to core, is to go ahead with this patch - the problem here is so narrow (basically doing getDefinitions() both before and after changing a Migration configuration entity) that I'd be happy to punt dealing with that to a follow-up, we'd still have a big net gain.

Although, now I have a simple thought - change the deriver to not cache the derivatives. This would be a performance hit if the deriver were called multiple times within a process, but I think it's worth taking the hit for it to be correct...

Yeah, let's go with that...

One small nit and a question.

1. +++ b/migrate_plus.module
   @@ -23,6 +23,17 @@ function migrate_plus_migration_plugins_alter(array &$migrations) {
   +    // For derived configuration entity-based migrations, strip the deriver
   +    // prefix.
   

   Is this a stray set of changes? Or a comment for why this type of alter is necessary would help.

2. +++ b/migrate_plus.services.yml
   --- /dev/null
   +++ b/migrations/migration_config_deriver.yml
   
   +++ b/migrations/migration_config_deriver.yml
   @@ -0,0 +1,5 @@
   +id: migration_config_deriver
   

   Nit: maybe a better name is migration_embedded_data_config_deriver.

Updating commit credits. Added myself (for reviews) and Wim Leers (for reviews)

Is this a stray set of changes? Or a comment for why this type of alter is necessary would help.

Comment expanded.

Nit: maybe a better name is migration_embedded_data_config_deriver.

Nope. The embedded_data source is only there because things blow up if no source configuration exists here - it will get overridden by the real source in the derived migration. Added a comment.

Waiting on tests to finish, but this looks good to go.

At long last, our long national nightmare is over!