Source plugins have a hidden dependency on migrate_drupal

I'm preferential, overall, to option #3, which is to introduce a new annotation type, provided by Migrate Drupal, used specifically for Drupal sources. It could be called MigrateDrupalSource, and it would provide the added benefit of giving us an official place to document hidden-but-crucial annotation properties like source_provider and minimum_schema_version, which is already its own issue: #2559345: Document source_provider and minimum_schema_version annotation properties

But if that doesn't pan out, I'm also OK with option #1.

This sounds like something we need to figure out in order to declare the API complete. Marking as Migrate critical.

I prefer option #3. Moving DrupalSqlBase to migrate means that you won't get actual errors in discovery of source plugins, but you will be discovering source plugins that don't actually make sense without migrate_drupal enabled.

Discussed on IRC with @benjy, and we devised what seems to both of us like a reasonable solution: standardize the source_provider annotation property and use it to filter out plugins which cannot be instantiated for dependency reasons.

Example: consider the d6_user source plugin. With this proposed solution, its annotation would look like this:

/**
 * @MigrateSource(
 *   id = "d6_user",
 *   source_provider = "drupal.6.user"
 * )
 */

With this kind of tagging, the plugin manager can know ahead of time that the plugin is built on top of Migrate Drupal (the drupal part of the source_provider value), that it's relevant to Drupal 6, and that it's part of the User module's migration path. The hierarchy helps with grouping plugins (i.e., in the Views-like UI that @benjy's working on), to any level of specificity.

We could apply this same logic to non-Drupal sources:

/**
 * @MigrateSource(
 *   id = "wp_posts",
 *   source_provider = "wordpress.4"
 * )
 */

There are a lot more details to be fleshed out here, but so far this strikes me as workable. Thoughts?

If we move forward with #6, does that subsume #2559345: Document source_provider and minimum_schema_version annotation properties?

Also note #2569805: For Drupal migration, identify the source module - for the UI we want to be able to identify the source module for all Drupal migrations, including variables, and the source_provider annotation by itself doesn't get us there, can we work a solution for that in here? E.g., allow the source provider to be overridden in the configuration:

source:
  plugin: variable
  variables:
    - actions_max_stack
  provider: system

and have a getProvider() API?

@benjy and I talked more about this on IRC. We still think this is workable, but the main issue is how to make the system aware that a source_provider value like drupal.6.node or drupal.7.aggregator is a dependency on the Migrate Drupal module.

My thought is we could add something like this to Migrate Drupal:

class MigrateDrupalSourceProvider implements ServiceModifierInterface {

    public function alter(ContainerBuilder $container) {
        $service = $container->get('plugin.manager.migrate.source');
        $service['tags'][] = array(
            'name' => 'migrate.source_provider',
            'pattern' => 'drupal.*',
        );
    }

}

This would explicitly mark the plugin.manager.migrate.source service as being responsible for plugins whose source_provider annotation matches the drupal.* pattern. By comparing source_provider annotations to service tags, @benjy's UI would easily be able to tell which plugins can be instantiated.

Took a first pass at this:

• Migrate Drupal implements a service modifier which tags the plugin.manager.migrate.source service with the migrate.source_provider tag. This tag contains a list of regular expressions, meaning that the plugin manager is responsible for any source plugin whose source_provider annotation matches any of those expressions.
• DrupalSqlBase's checkRequirements() is totally rewritten. It parses the source_provider annotation, which now has the form of drupal.MODULE.SCHEMA_EXPRESSION. MODULE is obviously the name of the module, and SCHEMA_EXPRESSION is an optional regular expression which the installed schema version must match.
• Added a test of the new checkRequirements() logic.
• Changed all existing source_provider annotations to conform to the new format.

I done screwed up. Maybe this will be better...

Wanna play again?

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
@@ -101,13 +101,25 @@ public static function create(ContainerInterface $container, array $configuratio
+                throw new RequirementsException('Incompatible schema version ' . $installed_version);

How will the user know which module has an incompatible schema version?

OK, screw it. PIFR is randomly failing the patch, repeatedly, for no very good or obvious reason. DrupalCI, thy results are canonical.

Eurgh!!! I give up.

Last I heard, annotation properties were not in short supply. You do not even need to define them on the annotation class if you do not want. So: if you want to depend on a module, say so! If you want a version regex, say so! Magic strings mixing these into one mess of a property: nope.

Of course the best would be to roll the migration templates back into migrate_drupal and also to only use templates when necessary and stick with config entities when not but I guess those ships have sailed. (These are the two why I try not to participate much.)

I filed an issue for putting them where they belong because it is impossible to find migrations these days. #2589805: Add documentation on how to find the menu migrations

I spoke with @EclipseGc and @jhodgdon about this at BADCamp and I believe I've found a much more elegant solution.

We can define an entirely new annotation type -- MigrateDrupalSource -- which extends MigrateSource and lives in Migrate Drupal. We can then apply that annotation to all plugins which depend on DrupalSqlBase. This will have the effect that, if Migrate Drupal is disabled, any MigrateDrupalSource-annotated plugins will be completely invisible to the plugin system. Boo-yah!

Not only does this solve our dependency issue, it's also easy to implement and it gives us the flexibility to add Migrate Drupal-specific annotation properties that are documented in a single, sensible place.

I will try to create the patch for this ASAP. It's such a beautiful solution that I could almost weep.

Rejoice!

Great solution! More comments on the new annotation to explain why it's necessary would be welcome though.

This will not work until #2600926: Allow annotations to inherit across namespaces is in core. Postponing.

Also removing the Migrate Critical tag since this is only revealed by @benjy's migration UI, which is not open to the public yet :) Under 98% of circumstances, users will not run into this problem.

Also removing the Migrate Critical tag since this is only revealed by @benjy's migration UI, which is not open to the public yet :) Under 98% of circumstances, users will not run into this problem.

Are we still using the "Migrate critical" tag? At any rate, with the plugin patch in, it is also revealed by migrate_tools, and will affect anything trying to do straight-forward discovery on migration plugins. Under 98% of non-Drupal migration circumstances, users *will* run into this problem.

  $manager = \Drupal::service('plugin.manager.migration');
  $plugin_definitions = $manager->getDefinitions();

Without migrate_drupal enabled:

exception 'Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException' with message 'You have requested a non-existent service "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

With migrate_drupal enabled, but no "migrate" connection to a Drupal database configured:

exception 'Drupal\Core\Database\ConnectionNotDefinedException' with message 'The specified database connection is not defined: migrate' in
/Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Core/Database/Database.php:366

Applied both patches (with a trivial reroll of this one, not finding the d7_field_instance place to patch). I still get the attempt at the missing cckfield service. I believe the node deriver is triggering that, continuing to dig...

Capturing where I am with this, quitting for the night:

1. The node derivers crap out because they're trying to get the cckfield service, which lives in migrate_drupal. I can get around this by having them catch the exception and return NULL when it's not there. This lets getDefinitions() succeed.
2. I can then catch and ignore PluginNotFoundException when creating instances, which works for filtering out most of the irrelevant migrations, but...
3. I get PluginException (instance class "Drupal\migrate_drupal\Plugin\migrate\CckMigration" does not exist) on the d6_field* migrations, which specify that as their migration class. I could silently catch PluginException to get by this, but then might miss exceptions that matter in "real" migrations.
4. Catching and ignoring PluginException gets me close to what I want - I do get my four beer migrations in migrate_example (unsorted, which is another issue to address), with only two "extraneous" migrations, block_content_type and block_content_body_field. The problem here is that these migrations have no inherent dependencies on any Drupal source (they simply configure a default block type with a body field) - there is no red flag raised that a general tool can use to know that these are really only relevant in the context of a Drupal-to-Drupal migration.

OK, here's a version of the patch that I've got working with migrate_tools (don't exactly know what's up with actions in the interdiff...). Basically, just having the node derivers sanity check the cckfield service let's me get to the point where the tools can handle the rest.

I've rerolled this patch, because it's no longer applying against the current 8.1.x-dev branch. I've never made an interdiff before, so this was fun :)

The last patch dropped the MigrateDrupalSource annotation, which is kinda important. Restoring here - this (along with the annotations patch) works with migrate_plus/migrate_tools in contrib.

Hi Mike,
it seems that your patch is not complete (anymore?). I've setup a site with the current dev of core 8.1.x, migrate_tools 8.2.x and migrate_plus 8.2.x, and applied this and other patch, that is also needed for migrate_tools to work. But when I run drush migrate-status, I get this error:

Error: Class 'Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase' not found in
/var/www/drupalvm/web/core/modules/language/src/Plugin/migrate/source/Language.php, line 18

-> this class still has the "MigrateSource" annotation, changing it to "MigrateDrupalSource" helps.

@agoradesign: Thanks for picking that up, I've added it in.

#2600926: Allow annotations to inherit across namespaces has been committed, this is now unblocked - let's give the bots a shot at it...

Let's see what happens when we use 8.2.x? I'm not expecting much better luck.

The testbot is not reporting anything useful, but locally running the action tests:

core$ ../vendor/bin/phpunit --group=action
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

....E

Time: 8.94 seconds, Memory: 240.25Mb

There was 1 error:

1) Drupal\Tests\action\Kernel\Migrate\d7\MigrateActionsTest::testActions
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "d6_field_instance" plugin does not exist.

/Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php:57
/Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryCachedTrait.php:30
/Users/mikeryan/Sites/8.1.x/core/modules/migrate/src/Plugin/MigratePluginManager.php:61
/Users/mikeryan/Sites/8.1.x/core/modules/migrate/src/Plugin/Migration.php:359
/Users/mikeryan/Sites/8.1.x/core/modules/migrate/src/Plugin/MigrationDeriverTrait.php:34
/Users/mikeryan/Sites/8.1.x/core/modules/node/src/Plugin/migrate/D6NodeDeriver.php:90
/Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php:105
/Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php:91
/Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php:279
/Users/mikeryan/Sites/8.1.x/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php:179
/Users/mikeryan/Sites/8.1.x/core/modules/migrate/src/Plugin/MigrationPluginManager.php:146
/Users/mikeryan/Sites/8.1.x/core/modules/migrate/src/Plugin/MigrationPluginManager.php:99
/Users/mikeryan/Sites/8.1.x/core/modules/migrate/src/Plugin/MigrationPluginManager.php:85
/Users/mikeryan/Sites/8.1.x/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php:239
/Users/mikeryan/Sites/8.1.x/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php:152
/Users/mikeryan/Sites/8.1.x/core/modules/action/tests/src/Kernel/Migrate/d7/MigrateActionsTest.php:27

FAILURES!
Tests: 5, Assertions: 28, Errors: 1.

Note that it's the D7 MigrateActionsTest causing D6NodeDeriver to be invoked (and ultimately try to use d6_field_instance). I have no idea what's changed in the current patch that would cause this.

Once upon a time, on a former MacBook, I had figured out how to get PhpUnit tests to stop at breakpoints in PhpStorm, having trouble now (I've got a debug configuration that will run the tests within PhpStorm, but my breakpoints are ignored)...

Wait, WTF, the action tests actually pass in PhpStorm but not at the command line...

OK, debugging in migrate_tools:

1. We do getDefinitions() to get all migration plugins, expecting that we'll be able to catch the non-useful ones at createInstance()/checkRequirements() time.
2. However, derivers are applied at getDefinitions() time.
3. D6NodeDeriver does getSourcePlugin('d6_field_instance')
4. d6_field_instance has a source_provider of 'content' (i.e., it expects to have a connection to a D6 database which contains an enabled 'content' module).
5. We don't even have a database connection, let alone one to a D6 database, let alone one with the content module enabled.
6. You might think we'd see a complaint about no connection, but DrupalSqlBase::getSystemData() swallows all exceptions so all that ends up propagating up is the fact that we can't create the source plugin.

There's not much we can do about it from migrate_tools - the exception entirely aborts getDefinitions() so we get no migration plugins to work with, not even the ones that would work if we could only discover them...

Perhaps the derivers can catch PluginNotFoundException on the getSourcePlugin() calls, but it feels like the problem is deeper than that. I may throw that into the patch though just to see if it makes the testbot any happier.

The same error happened to me on my 8.1.x test install last weekend, as soon as I enabled migrate_drupal. Without migrate_drupal, I had no problems

In IRC, @dawehner was asking why we didn't take the approach of adding migrate_drupal as a module dependency to the migration .yml files - didn't have an answer off the top of my head.

As for the failure of this annotation-based patch to work, looking at the tests for #2600926: Allow annotations to inherit across namespaces I realize that somewhere we need to inject that additional namespace for MigrateDrupalSource to be picked up. Looking at that now...

OK, got the additional annotation namespace in (also fixed up one more source plugin which wasn't using MigrateDrupalSource). In the migrate_tools context we're now getting ConnectionNotDefinedException, but let's see how the testbot does with this.

That's better - now just MigrationTest is failing, with "The "d6_field" plugin does not exist.". It's actually bogosity in the test, which is using a Drupal source plugin without enabling migrate_drupal. It got away with this before - all it's doing is getting the source plugin and fetching its ID, so the lack of migrate_drupal or any source connection didn't bit it - but now that the source plugin uses the MigrateDrupalSource annotation, the hidden dependency of the d6_field plugin on migrate_drupal is revealed. Let's see if we can find a source plugin not dependent on migrate_drupal...

OK, I expect this one to pass.

In the meantime - this patch is necessary but not sufficient for the tool use case (in general, being able to do getDefinitions() on the migration plugin manager without crashing and burning). It addresses the case where migrate_drupal is not enabled yet source plugins implicitly dependent on it exist in enabled modules. We've still got the problem of having migrate_drupal enabled and trying to get all plugins, where exceptions will go flying - either because there's no Drupal source database connection configured, or there is one configured, but plugins targeting a Drupal version other than the one that database contains will be... confused. I'll open a separate issue for that.

Following up in #2700693: [meta] Make MigratePluginManager::getDefinitions() work cleanly with migrate_drupal enabled

OK, both branches have passed - ready for manual review.

So the secret sauce here was Annotations on the Source Plugin Manager. This is working for me :)

Is there a chance to get this in for today's 8.1-rc1???

Changing this back to version 8.1.x, as it was changed to 8.2.x in comment #54 only for the test bot

(Edit):

PS: Info for changes for 8.1.0
http://mikeryan.name/blog/mikeryan/migration-update-for-drupal-8-1

I'm using this patch for a while now with the version 2.x-dev source plugins and migrate tools. Should be part of the next Drupal minor.

#72 still needs addressing.

@ayalon: The tools no longer require this fix (nevertheless, it still should be fixed).

I've been trying to use 'migrate' to import the demo data in Panopoly (we did this in D7 too) per #2732417: Port Panopoly Demo to Drupal 8!, and without this patch, I need to enable 'drupal_migrate' even though we're importing data using the 'embedded_data' source plugin and not importing anything from an older Drupal version. The error I get without the patch is:

Uncaught PHP Exception Symfony\\Component\\DependencyInjection\\Exception\\ServiceNotFoundException: "You have requested a non-existent service "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"?" at /var/www/drupal/core/lib/Drupal/Component/DependencyInjection/Container.php line 157

Restoring the "Migrate critical" tag, it's really a big problem for any migration scenario outside of the core upgrade (e.g., working on the D8 version of wordpress_migrate, it has to include a logically-unnecessary dependency on migrate_drupal for plugin discovery to work).

I'm using the patch from #64, but still cannot get migrate_tools' drush ms working without enabling migrate_drupal.

They use the \Drupal::service('plugin.manager.config_entity_migration')->createInstances([]) to fetch a list of migrations and die on d6_field_formatter_settings (core/modules/field/migration_templates/d6_field_formatter_settings.yml), because it explicitly declares the use of class: Drupal\migrate_drupal\Plugin\migrate\CckMigration. Seems we missed this one.

exception 'Drupal\Component\Plugin\Exception\PluginException' with message 'Plugin (d6_field_formatter_settings) instance class "Drupal\migrate_drupal\Plugin\migrate\CckMigration" does not exist.' in /srv/www/htdocs/PROJECT/html/core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php:97
Stack trace:
#0 /srv/www/htdocs/PROJECT/html/core/lib/Drupal/Core/Plugin/Factory/ContainerFactory.php(17): Drupal\Component\Plugin\Factory\DefaultFactory::getPluginClass('d6_field_format...', Array, 'Drupal\migrate\...')
#1 /srv/www/htdocs/PROJECT/html/core/modules/migrate/src/Plugin/MigrationPluginManager.php(99): Drupal\Core\Plugin\Factory\ContainerFactory->createInstance('d6_field_format...', Array)
#2 /srv/www/htdocs/PROJECT/html/modules/contrib/migrate_tools/migrate_tools.drush.inc(438): Drupal\migrate\Plugin\MigrationPluginManager->createInstances(Array)
#3 /srv/www/htdocs/PROJECT/html/modules/contrib/migrate_tools/migrate_tools.drush.inc(140): drush_migrate_tools_migration_list(NULL, '')
#4 [internal function]: drush_migrate_tools_migrate_status()
#5 /usr/share/drush/drush-8.0.5/includes/command.inc(366): call_user_func_array('drush_migrate_t...', Array)
#6 /usr/share/drush/drush-8.0.5/includes/command.inc(217): _drush_invoke_hooks(Array, Array)
#7 [internal function]: drush_command()
#8 /usr/share/drush/drush-8.0.5/includes/command.inc(185): call_user_func_array('drush_command', Array)
#9 /usr/share/drush/drush-8.0.5/lib/Drush/Boot/BaseBoot.php(67): drush_dispatch(Array)
#10 /usr/share/drush/drush-8.0.5/includes/preflight.inc(66): Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#11 /usr/share/drush/drush-8.0.5/drush.php(12): drush_main()
#12 {main}

I just wish we had a way to overwrite/decide ourselves which migrations we want to use, instead of just always loading anything we can find... But that's another issue for another day ;)

Plugins do have a means of filtering on a module dependency - the provider key. So, I added "provider: migrate_drupal" to all the Drupal migrations. That's not quite enough, though, because derivers are invoked before the provider is checked. So, I extended ContainerDerivativeDiscoveryDecorator to pre-check the provider.

I borrowed the base (migrate_drupal not installed) test from #2700693: [meta] Make MigratePluginManager::getDefinitions() work cleanly with migrate_drupal enabled - this still leaves the other angle (migrate_drupal is installed but no database is configured) broken, but we can pick up on that one in the other issue.

@benjy: It's not clear to me what that patch changes in behavior? The provider check is still too late to prevent us from invoking an invalid deriver.

If we do want to make this a broader fix, we could simply put the provider check in ContainerDerivativeDiscoveryDecorator... like so.

Oh, duh, that's going to break, because anything else using ContainerDerivativeDiscoveryDecorator would need to pass in a module handler...

OK - patch in #82 needs review, we'll pretend #85 didn't happen...

Draft change record added.

+++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php
@@ -0,0 +1,68 @@
+    // Test with all modules containing Drupal migrations.

Isn't there any way of getting this list from Drupal core? I'm guessing we will forget to edit this test if new migrations are added to modules that don't have any already.

I'm not sure if I like overriding 'provider' this way. By default the 'provider' of each plugin is the module that defines it. Presumably Drupal uses this information in some way? Are there any other situations where provider is overridden? Any documentation of what this means, semantically?

Also, we already have a 'Drupal 6' or 'Drupal 7' tag that indicates that migrations are for migrate_drupal. I don't like having to say this same thing in two places, it provides one more way for people to mess up.

Isn't there any way of getting this list from Drupal core? I'm guessing we will forget to edit this test if new migrations are added to modules that don't have any already.

We could use the MigrationPluginManager... Oops;)! Kind of a chicken-and-egg problem there... I suppose we *could* use YamlDiscovery to dynamically set $modules, but I'm not sure that's worth it - we would have to do that in the constructor, wouldn't we? Or could it be done in setUp()?

I'm not sure if I like overriding 'provider' this way. By default the 'provider' of each plugin is the module that defines it. Presumably Drupal uses this information in some way? Are there any other situations where provider is overridden? Any documentation of what this means, semantically?

Core does explicitly support providing a provider in configuration (it checks for a configured provider before setting the provider to the current module), although I can't find any example of it being used in core outside of tests. I think it does make sense, in that our precise problem here is that migrate_drupal is logically a provider of these migrations, but we've chosen to have them live in their relevant modules (for reasons detailed in #2533886: [meta] Move module-specific migration support into the particular modules supported).

As near as I can tell, the meaning of "provider" is not documented, but the one thing it is used for is to exclude plugins whose provider is not enabled (exactly what we need).

The behavior we're looking for is that no Drupal migration plugin should be instantiated by the plugin manager unless both the module it contains and migrate_drupal are enabled. The Yaml discovery ensures the former, and making migrate_drupal a provider ensures the latter. I don't see a cleaner way to handle this.

Rerolled #82 after #2691659: d7 filter weights not migrating to d8 landed. Two phpcs raised issues fixed, see interdiff.

When I use this patch I get error message: Migration migration_name does not exist.
Do you have maybe any idea? Thanks!

When I use this patch I get error message: Migration migration_name does not exist.
Do you have maybe any idea? Thanks!

@animaci: That's not much to go on. Exactly what context do you see that message? What operation are you performing?

This situation continues to be a major PITA for non-upgrade migration scenarios, and blocks cleaner contrib integration: #2752335: Properly integrate configuration-entity-based migrations with the core plugin manager.

@mikeryan: Thanks, I think it was my fault, it was not caused by the patch. Sorry for confusion. Just forget my comment :) Thanks!

Looks pretty decent to me. I have a few minor issues, but overall this seems like a good, clean solution.

1. +++ b/core/modules/migrate/src/Plugin/Discovery/MigrateContainerDerivativeDiscoveryDecorator.php
   @@ -0,0 +1,56 @@
   +class MigrateContainerDerivativeDiscoveryDecorator extends ContainerDerivativeDiscoveryDecorator {
   

   Now THAT is a class name :) It's a mouthful but it doesn't really explain what this class does. Can a doc comment be added for that?

2. +++ b/core/modules/migrate/src/Plugin/Discovery/MigrateContainerDerivativeDiscoveryDecorator.php
   @@ -0,0 +1,56 @@
   +   * Creates a new instance.
   

   Should be "Constructs a new MigrateContainerDerivativeDiscoveryDecator."

3. +++ b/core/modules/migrate/src/Plugin/Discovery/MigrateContainerDerivativeDiscoveryDecorator.php
   @@ -0,0 +1,56 @@
   +   *   The parent object implementing DiscoveryInterface that is being
   

   It's pretty obvious from the parameter type that it's implementing DiscoveryInterface ;-)

4. +++ b/core/modules/migrate/src/Plugin/Discovery/MigrateContainerDerivativeDiscoveryDecorator.php
   @@ -0,0 +1,56 @@
   +  protected function getDeriver($base_plugin_id, $base_definition) {
   

   $base_definition should be type hinted if possible.

5. +++ b/core/modules/migrate/src/Plugin/Discovery/MigrateContainerDerivativeDiscoveryDecorator.php
   @@ -0,0 +1,56 @@
   +    if (isset($base_definition['provider']) && !in_array($base_definition['provider'], array('core', 'component')) && !$this->providerExists($base_definition['provider'])) {
   

   providerExists() is just a call to the module handler, so do we really need it in its own method?

6. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php
   @@ -0,0 +1,63 @@
   +    $migration_plugins = \Drupal::service('plugin.manager.migration')->getDefinitions();
   

   We need a comment here explaining the nature of this test. What would happen if the test broke? Also...for correctness' sake, kernel tests should use $this->container->get(), not \Drupal::service().

Addressed all the comments above, except

$base_definition should be type hinted if possible.

This function signature matches the parent.

Anyone out there willing to review the latest patch and either give it an RTBC or identify any other changes needed?

The current approach is not a BC breaker.

I think the approach is valid but I do share some of the concerns of @vasi in #92. However if migrate_drupal is installed then existing contrib and custom plugins that depend on it will continue to work - so this is not a migrate BC break per se. And therefore I agreed with @mikeryan that we should commit this to 8.1.x as well.

I discussed how to inform developers that they need to add the provider key for migrate plugins that depend on migrate_drupal - we considered triggering a user recoverable error but that doesn't really buy us anything because it valid for a migration plugin to not have a provider.

Given that this patch achieves what we want it to and we can continue to evaluate it as solution once committed I'm happy to see this go in.

Committed and pushed 89815ba33923ecd0018b62afb393d05d987a40fa to 8.2.x and e98b14a to 8.1.x. Thanks!

What on earth have happened. This needs to be rolled back and properly fixed! Any solution that involves editing every migration is a hack. Not for the first time we are hacking migrate to fix a core bug. :(

A) roll back, that's why I set it to RTBC

B) I will post a patch that fixes this in the plugin subsystem and then the source plugins. The real problem is that the source plugins have a dependency on more than one provider. Currently the plugin system uses one provider which is the module its namespace is in but it's entirely possible as the issue shows that a plugin depends on more than one provider and so the plugin needs to be able to have a list of providers it depends on. That's a plugin system limitation I thought I have made this clear but since I really badly don't want to post any non migrate patches I didn't but apparently I need to. Also, all the decorator needs to do is inherit the provider part of the definition and then DefaultPluginManager::findDefinitions() will throw the definition out if the providers are not there. Already explicit provider definitions are honored (see AnnotatedClassDiscovery::prepareAnnotationDefinition) so the plugin system just needs to allow for an array as I mentioned previously and that's all.

Actually, AFAIK DerivativeDiscoveryDecorator::mergeDerivativeDefinition already inherits everything including providers. Here's a tentative patch with one plugin edited for demo, non-testing because it first needs a rollback. I need to rename ProviderDiscovery to ProviderDecorate and perhaps add a wrapper method for the service call but it shows the way.

A couple of points:

1. It's not just the source plugins - there are migrations such as block_content_body_field which make no sense outside of a Drupal-to-Drupal migration and are not using a migrate_drupal-dependent source plugin. We don't want the plugin manager returning those.
2. findDefinitions() is too late to prevent the node derivers from blowing up in the absence of migrate_drupal.

Edit: deleted.

I can't let this go and just edit a comment in a fixed issue. Let me reiterate:

The fundamental flaw of this issue is people hacking migrate when the shortcoming is in core. This is not the first time this happens. Can we stop doing this?

It is entirely possible to come up with a plan that covers almost all of the migration to find their dependencies automatically and only require the source or migration plugins to use an explicit provider when the automated process is inadequate. Mike points out the three migrations using the embedded_data source need to do this. That's fine. That's three.

What core misses here

1. Add support for multiple providers in both findDefinitions and the deriver. Don't try to derive if one if the providers are missing. I have absolutely no idea why this (at least for the singular case) is in the migrate deriver. If I put there, that was a mistake.
2. Add an annotation property which allows automated multiple provider finding based on use statements. We have the pieces for this just need to stitch them together: the parser has a getUseStatements() call so if we were to pass the parser to prepareAnnotationDefinition then we could just call getProviderFromNamespace on all the use statements and add them to the provider list.

End. Note that most of this is already coded either above or in https://www.drupal.org/node/2488258#comment-11402375 -- that patch persists use statements but processing them instead is better.

Once this is done, we will need to use the code above to copy source plugin providers into the Migration plugin providers. And then just opt in MigrateSource into this new mechanism and mostly be done:

namespace Drupal\node\Plugin\migrate\source\d6;

use Drupal\migrate\Row;
use Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase;

so this plugin depends on node, migrate and migrate_drupal. Yes, a few plugins will need explicit provider lists (VocabularyPerType , NodeRevision twice, CommentVariablePerCommentType, TermNodeRevision, ViewMode , FieldInstancePerViewMode, UrlAlias twice) but most of them won't. And most definitely you won't need to edit every migration.

Okay discussed with @mikeryan in IRC - we agreed to revert and go for a fix that does not require updates to migrations whilst also being simple.

So here's an approach built on @vasi's suggestion of re-using tags. It decorates discovery to remove any definitions with missing definitions. I think this is better than the previous approach because no migrations have to change and it affects all definitions not just derivatives. Improved the test as well. I think it would be great to a test module with a migration that does not depend on Drupal.

No interdiff because new approach.

We have such migrations already in a test module...

I am coding as fast as I can. Wait an hour or two, I am almost done. Teaser trailer:

namespace Drupal\Core\Plugin\Discovery;
use Doctrine\Common\Reflection\StaticReflectionParser;
use Drupal\Component\Annotation\AnnotationInterface;
class AnnotatedClassDiscoveryAutomatedProviders extends AnnotatedClassDiscovery {
  protected function prepareAnnotationDefinition(AnnotationInterface $annotation, $class, StaticReflectionParser $parser) {
    $provider_set = $annotation->getProvider();
    parent::prepareAnnotationDefinition($annotation, $class, $parser);
    if (!$provider_set) {
      $namespaces = array_map(['Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery', 'getProviderFromNamespace'], $parser->getUseStatements());
      $namespaces[] = $annotation->getProvider();
      $annotation->setProvider(array_filter(array_unique($namespaces)));
    }
  }
}

Progress report.

Try #2. So this patch

1. Adds an annotated class discovery variant which automatically discovers provider dependencies based on use statements.
2. Uses this in the new MigrationSourcePluginManager
3. Adds a migrate specific decorator and uses it in MigrationPluginManager to copy the source plugin providers into the migration plugin provider. How fortunate source now has rich provider information...
4. Moves the provider logic from findDefinitions into a decorator and while at it expands it into multiple provider support.
5. Uses this decorator in MigrationPluginManager before calling the deriver decorator.

Eh.

It's a bit annoying I can't delete comments and at the same time I'd like to show where I am. It's minimal fixes :/

Interdiff is against #120 just to show how inconsequential the change is: the problem was passing in the module list instead of the module handler. This is what I get for trying to simplify the code from findDefinitions.

WIN! MigrationTest tries to use d6_field source plugin but the patch removes that because migrate_drupal is not enabled. That's fun.

The rest seems to be fixed by not even passing in the module handler but the providerExists method itself.

After talking to alexpott the inherit decorator is not needed because the source plugin will be gone already so what's needed instead is a plugin which filters out migrations if their source plugin is missing.

AnnotatedClassDiscoveryAutomatedProviders now always merges.

Also, the ProviderFilterDecorator is only for a very rare case: where the migration explicitly specifies providers and uses a deriver. No such thing currently.

Added all the necessary provider notations to sources (9) and migrations (3) which makes the test written by alexpott pass. Still much, much better than changing 147 migration YAMLs (plus all the custom ones using core sources).

So the problem we have is class UrlAlias extends UrlAliasBase { - where the use on migrate_drupal is in UrlAliasBase and not UrlAlias. I think this is unexpected. I guess the question is how tricky / expensive is it to determine the full class chain and it's use statements.

And this is where doctrine's limited static reflection is not going to help :(

+++ b/core/lib/Drupal/Core/Plugin/Discovery/ProviderFilterDecorator.php
@@ -0,0 +1,86 @@
+  /**
+   * Passes through all unknown calls onto the decorated object.
+   */
+  public function __call($method, $args) {
+    return call_user_func_array(array($this->decorated, $method), $args);
+  }
+
+

I don't think this code is necessary.

  /**
   * Gets the name of the provider of the annotated class.
   *
   * @return string
   */
  public function getProvider();

  /**
   * Sets the name of the provider of the annotated class.
   *
   * @param string $provider
   */
  public function setProvider($provider);

The patch in #130 is changing this... (core/lib/Drupal/Component/Annotation/AnnotationInterface.php)

> So the problem we have is class UrlAlias extends UrlAliasBase { - where the use on migrate_drupal is in UrlAliasBase and not UrlAlias. I think this is unexpected. I guess the question is how tricky / expensive is it to determine the full class chain and it's use statements.

I would love to leave that to a followup. It's not exactly tricky, the scaffolding is in place: currently we use the MockFileFinder but Psr0FindFile exists and StaticReflectionParser::parse has code for finding the parent already, the optimization would need to be changed to include any extends or simply we would need to switch off the optimization. But I can do it in this issue if necessary.

> I don't think this code is necessary.

Please open an issue to remove it from InfoHookDecorator and from here. I copy-pasted. This I do not know. Edit: based on our discussion, the decorated object might implement additional interfaces and then the decorator shouldn't remove that functionality. Nevermind the decorated object will fail any type hints at least the method calls work...

I will amend the doxygen tomorrow to reflect it can be a string or an array.

Here we go. interdiff is only the relevant parts, I didn't interdiff reverting every source plugin. Doxygen later.

Edit: base on discussions with alexpott the next version will do away with reflection and use a class rather. I will post when I have written doxygen.

I think title and the component of this issue should be changed now after the new direction.

@jibran, how's the new title?

Patch removes some cruft from #137 and tightens AnnotatedClassDiscoveryAutomatedProviders , adds doxygen where it was missing.

Reverted providerExists for even better BC. Interdiff against #137. This is beginning to look like done, there's very little change in existing classes outside of migrate and a few new classes. BC is now complete. The next step will be #2767483: Refactor the process definition normalizer into a decorator so process plugins are in a uniform format while discovering and then another followup which uses this fact to change NoSourcePluginDecorator into NoPluginDecorator .

While #142 is ready, this version reads slightly better in my opinion. And it's a bit shorter, too.

One semicolon. No change.

Some minor review points. Totally loving the new title.

1. +++ b/core/lib/Drupal/Component/Annotation/AnnotationInterface.php
   @@ -13,16 +13,16 @@
   +   * @return string|array
   ...
   +   * @param string|array $provider
   

   Can we add a desc why it can be string and array as well?

2. +++ b/core/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php
   @@ -19,6 +19,11 @@ class AnnotatedClassDiscovery implements DiscoveryInterface {
   +  const CLASSANNOTATIONOPTIMIZE = TRUE;
   
   +++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
   @@ -0,0 +1,31 @@
   +  const CLASSANNOTATIONOPTIMIZE = FALSE;
   

   Can we make it SCREAMING_SNAKE_CASE?

3. +++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
   @@ -0,0 +1,31 @@
   +    $providers = array_flip((array) $annotation->getProvider());
   ...
   +      $providers += array_flip(array_filter(array_map([$this, 'getProviderFromNamespace'], $parser->getUseStatements())));
   ...
   +    $annotation->setProvider(array_keys($providers));
   

   Is there a reason for using array_flip first then array_keys?

4. +++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
   @@ -0,0 +1,31 @@
   +    do {
   ...
   +    } while ($parser = StaticReflectionParser::getParentParser($parser, $finder));
   

   Now I know who has been adding do while to the core :P. I personally don't like it as coder but meh!

5. +++ b/core/lib/Drupal/Core/Plugin/Discovery/ProviderFilterDecorator.php
   @@ -0,0 +1,81 @@
   +   * @var callable
   

   Desc missing.

6. +++ b/core/lib/Drupal/Core/Plugin/Discovery/ProviderFilterDecorator.php
   @@ -0,0 +1,81 @@
   +   * Remove plugin definitions with non-existing providers.
   

   Removes.

7. +++ b/core/lib/Drupal/Core/Plugin/Discovery/ProviderFilterDecorator.php
   @@ -0,0 +1,81 @@
   +  public static function filterDefinitions(array $definitions, callable $provider_exists) {
   +    // Besides what the caller accepts, we also accept core or component.
   +    $provider_exists = function ($provider) use ($provider_exists) {
   +      return in_array($provider, ['core', 'component']) || $provider_exists($provider);
   +    };
   +    return array_filter($definitions, function ($definition) use ($provider_exists) {
   +      $definition = (array) $definition + ['provider' => []];
   +      $providers = (array) $definition['provider'];
   +      return count($providers) == count(array_filter($providers, $provider_exists));
   +    });
   

   A lot of magic and I love it.

8. +++ b/core/lib/Drupal/Core/Plugin/Discovery/StaticReflectionParser.php
   @@ -0,0 +1,25 @@
   +  public static function getParentParser(BaseStaticReflectionParser $parser, $finder) {
   

   Desc block missing.

> A lot of magic and I love it.

Where?? I have contemplated quite a few versions and this seemed like the most readable.

I removed the array_keys / array_flip trickery. Let's not get tricky.

Edit: crossposted with benjy's review but yes the MigrateSourcePluginManager uses the new discovery so it's totally necessary, yes.

Edit2: jibran and benjy both confirmed the closures seemed like a bit magic but they are OK just need to get used to it. Good I haven't used the double array_filter I was planning...

Random bot failure.

git.drupal.org failed while the bot was checking out...

#2767723: Git server outage and reduced availability

Finally!

With my submission for the most superficial review of 2016:

+++ b/core/lib/Drupal/Component/Annotation/AnnotationInterface.php
@@ -13,16 +13,18 @@
+   *   Can be str4ing or array because of backwards compatibility reasons.
...
+   *   Can be str4ing or array because of backwards compatibility reasons.

s/str4ing/string/

I see that this is committed to 8.3.x - what about 8.2.x (at least)?

@mikeryan those status messages are an unfortunate consequence of creating the new 8.3.x branch - nothing (other than that has occurred) it has both the original commit and then revert. So it's still not committed anywhere.

1. +++ b/core/lib/Drupal/Component/Annotation/AnnotationInterface.php
   @@ -13,16 +13,18 @@
       *
   -   * @return string
   +   * @return string|array
   +   *   Can be str4ing or array because of backwards compatibility reasons.
       */
      public function getProvider();
   ...
   -   * @param string $provider
   +   * @param string|array $provider
   +   *   Can be str4ing or array because of backwards compatibility reasons.
       */
      public function setProvider($provider);
   

   This sounds like a BC break for me. As a caller you can no longer be sure what you can, and there could be code breaking it. Can we do something about it?

2. +++ b/core/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php
   @@ -19,6 +19,11 @@ class AnnotatedClassDiscovery implements DiscoveryInterface {
      /**
   +   * Whether the parser should return only wants class annotations.
   +   */
   

   Is this actual english?

3. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -277,19 +278,7 @@ protected function findDefinitions() {
   +    return ProviderFilterDecorator::filterDefinitions($definitions, function ($provider) { return $this->providerExists($provider); });
   

   Nitpick: Afaik we don't do this inline

4. +++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
   @@ -0,0 +1,32 @@
   +    $finder = \Drupal::service('class_loader');
   

   Why can't we inject the class loader here?

5. +++ b/core/lib/Drupal/Core/Plugin/Discovery/ProviderFilterDecorator.php
   @@ -0,0 +1,82 @@
   +      $definition = (array) $definition + ['provider' => []];
   +      $providers = (array) $definition['provider'];
   

   Can we document why we need this casting to array here? This is probably because plugin definitions can be arrays as well

6. +++ b/core/lib/Drupal/Core/Plugin/Discovery/ProviderFilterDecorator.php
   @@ -0,0 +1,82 @@
   +      return count($providers) == count(array_filter($providers, $provider_exists));
   

   IMHO using array_diff would be more semantic here. An alternative could be to provide a $provider_not_exists and check whether the result is not empty.

7. +++ b/core/lib/Drupal/Core/Plugin/Discovery/StaticReflectionParser.php
   @@ -0,0 +1,27 @@
   +
   +namespace Drupal\Core\Plugin\Discovery;
   ...
   + * Allows getting the reflection parser for the parent class.
   + */
   +class StaticReflectionParser extends BaseStaticReflectionParser {
   

   This could be moved to the plugin component, couldn't it?

+++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php
@@ -0,0 +1,108 @@
+    // Enable migrate_drupal to test that the plugins can now be discovered.
+    $this->enableModules(['migrate_drupal']);
+    // Set up a migrate database connection so that plugin disocovery works.
...
+    Database::addConnectionInfo('migrate', 'default', $connection_info['default']);

s/disocovery/discovery/

Let's also test with migrate_drupal enabled and no database connection (yet) configured.

I agree with @dawehner wrt to #164.1. Imo we should add new methods on a new interface something like getProviders() and setProviders(). It should be documented that getProvider() returns the "main" provider. Main is something that a plugin manager with allows plugins to have multiple providers can decide.