Throw exception for source plugins without a source_module property

I am in favor of this, because I like active, explicit error detection. APIs like this should not be tolerant of bad input, especially if it's liable to cause weird, hard-to-debug errors and wackiness further (much further) down the line. Garbage in, exception out: that's my motto. I would much rather bail out with an error (plus clear documentation on how to correct it) than dance around bad input and deal with the support requests and bugs that then arise.

So, although this is a BC break, +1 from me.

+1 for me too.

Fixing some more missing source_module and destination_module plugins.

Fix one more missing source_module source plugins.

I suspect we should not set a source_module of "migrate_drupal" because that is not enabled on the source site, but I'm not sure what it should be instead.

Per discussion with @catch, we're promoting the "Migrate critical" issues to critical given the importance of a stable migration path at this point in the release cycle. (The framework and release managers will confirm this in a later triage for individual issues.)

We discussed whether we were concerned about the BC break here, since this could potentially disrupt contrib. However, since the migrations would already fail eventually, this issue will just make them fail earlier and explicitly. So we'll just need to add a change record update and make the change only in the minor branch. So, moving the issue to 8.5.x.

@heddn and @phenaproxima say there's a CR for this already somewhere but we need to edit it and add this issue to the references on it.

https://www.drupal.org/node/2831566 is the CR for the previous change, so I added the reference to it and we should update it for this issue. However, we should also create a new change record draft for throwing the exceptions since it is its own BC break.

Changed all 6 instances of source_module = "migrate_drupal" to source_module = "system".

I like this patch a lot -- explicit error checking is absolutely better DX, so this is a welcome improvement. I do have some questions, though.

1. +++ b/core/modules/migrate/src/Plugin/MigrateDestinationPluginManager.php
   @@ -61,4 +62,17 @@ public function createInstance($plugin_id, array $configuration = [], MigrationI
   +    foreach (['destination_module'] as $required_property) {
   +      if (empty($definition[$required_property])) {
   +        throw new InvalidPluginDefinitionException($plugin_id, sprintf('The %s plugin should define the %s property.', $definition['class'], $required_property));
   +      }
   +    }
   

   Can we remove the foreach? I understand why it's there, but it's extraneous right now since we're only checking for one property.

   Additionally, I feel like it would be preferable if we gave the plugin ID here, rather than the plugin class. The ID is the canonical way to refer to a plugin.

2. +++ b/core/modules/migrate/src/Plugin/MigratePluginManager.php
   @@ -62,4 +63,17 @@ public function createInstance($plugin_id, array $configuration = [], MigrationI
   +    foreach (['id'] as $required_property) {
   +      if (empty($definition['id'])) {
   +        throw new InvalidPluginDefinitionException($plugin_id, sprintf('The %s plugin should define the %s property.', $definition['class'], $required_property));
   +      }
   +    }
   

   Same here.

3. +++ b/core/modules/migrate/src/Plugin/MigrateSourcePluginManager.php
   @@ -70,4 +73,17 @@ protected function findDefinitions() {
   +    foreach (['source_module'] as $required_property) {
   +      if (empty($definition[$required_property])) {
   +        throw new InvalidPluginDefinitionException($plugin_id, sprintf('The %s plugin should define the %s property.', $definition['class'], $required_property));
   +      }
   +    }
   

   And here.

4. +++ b/core/modules/migrate/src/Plugin/migrate/destination/NullDestination.php
   @@ -10,7 +10,8 @@
   + *   destination_module = "null",
   

   Forgive me for getting paranoid edge-casey, but if there was a module named "null"...unlikely as that is...could this not have bizarre side effects? How about using a non-falsy, never-could-be-a-module-name value here, like -1?

5. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
   @@ -16,49 +18,58 @@ class MigrationProvidersExistTest extends MigrateDrupalTestBase {
   +    $this->enableAllModules();
   +    $this->enableModules(['migration_provider_test']);
   

   Doesn't the call to enableAllModules() also enable migration_provider_test?

6. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
   @@ -16,49 +18,58 @@ class MigrationProvidersExistTest extends MigrateDrupalTestBase {
   -      if ($migration_id == 'migration_provider_test') {
   -        $this->assertFalse($source_module, new FormattableMarkup('Source module not found for @migration_id.', ['@migration_id' => $migration_id]));
   -        $this->assertFalse($destination_module, new FormattableMarkup('Destination module not found for @migration_id.', ['@migration_id' => $migration_id]));
   -      }
   -      elseif ($migration_id == 'migration_provider_no_annotation') {
   -        $this->assertFalse($source_module, new FormattableMarkup('Source module not found for @migration_id.', ['@migration_id' => $migration_id]));
   -        $this->assertTrue($destination_module, new FormattableMarkup('Destination module found for @migration_id.', ['@migration_id' => $migration_id]));
   -      }
   -      else {
   -        $this->assertTrue($source_module, new FormattableMarkup('Source module found for @migration_id.', ['@migration_id' => $migration_id]));
   -        $this->assertTrue($destination_module, new FormattableMarkup('Destination module found for @migration_id.', ['@migration_id' => $migration_id]));
   -      }
   

   I don't quite understand why this was removed? It doesn't seem to have anything to do with the problem at hand.

7. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
   @@ -16,49 +18,58 @@ class MigrationProvidersExistTest extends MigrateDrupalTestBase {
   +  protected function enableAllModules() {
   +    // Install all available modules.
   +    $module_handler = $this->container->get('module_handler');
   +    $modules = $this->coreModuleListDataProvider();
   +    $modules_enabled = $module_handler->getModuleList();
   +    $modules_to_enable = array_keys(array_diff_key($modules, $modules_enabled));
   +    $this->enableModules($modules_to_enable);
   +  }
   

   I'm not sure about this. Is it possible to test without enabling *all* modules? If not, we should at least explain why we need to do this.

8. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
   @@ -147,11 +147,9 @@ public function testMigrateUpgrade() {
   +    $this->assertSession()->pageTextContains(t('Resolve the issue below to continue the upgrade'));
   +    $this->assertSession()->pageTextContains(t('NoSourceModule plugin should define the source_module property.'));
   

   Nit: For readability, let's just call assertSession() once and reuse the object it returns.

Adding a tag so we can see this issue in the kanban.

1. Removed foreach and replaced $definition['class'] with $definition['id'].
2. Removed foreach and replaced $definition['class'] with $definition['id'].
3. Removed foreach and replaced $definition['class'] with $definition['id'].
4. Changed destination_module from "null" to "-1".
5. enableAllModules() does not enable migration_provider_test, it enables anything in core/modules while migration_provider_test is in a sub-directory.
6. The asserts for migration_provider_test and migration_provider_no_annotation have been removed because those modules are not enabled by enableAllModules() so these conditions will never be met. I have re-instated the else condition asserts.
7. Can you suggest a simpler method for testing the source/destination plugins for all modules? I can't think of one.
8. Set assertSession() to a re-used variable.

Good, except two (optional) nitpicks:

+++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
@@ -50,8 +50,11 @@ public function testProvidersExist() {
+      $this->assertTrue($source_module, new FormattableMarkup('Source module found for @migration_id.', ['@migration_id' => $migration_id]));
+      $this->assertTrue($destination_module, new FormattableMarkup('Destination module found for @migration_id.', ['@migration_id' => $migration_id]));

Can these be $this->assertInternalType('string', $source_module)?

This we want/need to stay at null. At least here.

Okay, but that should be true null (destination_module = null), not string "null" (destination_module = "null")

the #32 It looks good for me

@heddn sorry, I mean that this address all the issues reported before ;)

I also agree that "/dev/null" is better than just "null" or -1

+++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
@@ -54,7 +54,9 @@ public function testProvidersExist() {
+      $this->assertInternalType('string', $source_module);
       $this->assertTrue($destination_module, new FormattableMarkup('Destination module found for @migration_id.', ['@migration_id' => $migration_id]));

My only nit -- and it is a nit, doesn't change RTBC -- is that I was thinking we'd replace the assertTrue() with the assertInternalType().

Ugh, sorry, @maxocub just reminded me -- I have to rescind RTBC because we still need change record stuff to happen.

I also have to kick this back again because this change affects the logic in DestinationBase::getDestinationModule():

public function getDestinationModule() {
    if (!empty($this->configuration['destination_module'])) {
      return $this->configuration['destination_module'];
    }
    if (!empty($this->pluginDefinition['destination_module'])) {
      return $this->pluginDefinition['destination_module'];
    }
    if (is_string($this->migration->provider)) {
      return $this->migration->provider;
    }
    else {
      return reset($this->migration->provider);
    }
  }

As of this change, $this->pluginDefinition['destination_module'] will never be empty, so the last two logic branches are effectively unreachable.

Please explain why destination_module are label are not required. The UI form needs source_module, destination_module and label.

      if ($source_module && $destination_module) {
        $table_data[$source_module][$destination_module][$migration_id] = $migration->label();
      }

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/Variable.php
   @@ -13,7 +13,8 @@
   + *   source_module = "system"
   

   Sorry, this won't work. Not all variable table variables are supplied by the 'system' module. The ui won't know it is missing data.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/VariableMultiRow.php
   @@ -11,7 +11,8 @@
   + *   source_module = "system"
   

   Ditto.

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/i18nVariable.php
   @@ -8,7 +8,8 @@
   + *   source_module = "system"
   

   The source module for i18n_variable plugin is not the system module. The sources are various submodules of i18n, but some of those issues are not yet committed.

+++ b/core/modules/migrate/src/Plugin/MigrateSourcePluginManager.php
@@ -70,4 +73,15 @@ protected function findDefinitions() {
+  public function processDefinition(&$definition, $plugin_id) {

Two things here. Most important is that this is only looking for the source_module on the definition. The source_module can also be declared in the migration yml as well. Checking by source plugin alone is insufficient. See the implementation of SourcePluginBase::getSourceModule.

And second, at first I thought this was about a process plugin because of the name, processDefinition. Don't know if it makes sense to change it, just noting my temporary confusion.

Also, destination_module can be declared in the multiple places, DestinationBase.php::getDestinationModule and Config::getDestinationModule.

edit: s/variables/variable table variables/

I am postponing this on #2859304: Show field type migrations correctly in Migrate Drupal UI, because that issue introduces a new Migrate-specific exception that can be thrown when a plugin does not define a required property. Perfect for this issue.

#2859304: Show field type migrations correctly in Migrate Drupal UI landed.

Patch in #41 no longer applies. Re-rolled.

• Employ the new Migrate-specific exception.
• Correct some coding standards errors.

Hopefully this will pass the tests.

Honestly, though, I'm hugely confused by the scope of this issue. The IS is very vague regarding what plugin definition properties we consider sacrosanct, and the tests in the patch are inconsistent about it too. What exactly do we want to do here? We need to discuss this, then fix the IS, and implement a patch that fulfills all of the requirements we decide upon.

I'm also re-categorizing this issue. This is not a bug, it's an API addition to improve DX. I don't think that exactly counts as a "feature request", but that's the closest thing I can come up with. Chances are that, due to the BC break this introduces, it won't land in 8.4.x anyway.

Discussed on IRC with @heddn and @maxocub.

We agreed that this patch and issue are too fuzzy. We will re-scope this issue, and refactor the patch, to only deal with the source_module property of source plugins.

We will also open a follow-up issue to do something similar, where applicable, to the destination_module property of destination plugins. So now that we have a clear path forward, let us:

• Rewrite the issue summary
• Open a follow-up for destination_module
• Remove all destination_module stuff from the current patch

Fixed the IS.

Minor IS fixes.

Okay, I ripped out all the destination_module stuff from the patch. Now we just have to open a follow-up to reconstitute all of that code in another patch, and I think this can proceed to actual review...

No interdiff because this shed so much code that it might as well be a totally new patch.

I spent forty minutes frying my brain in front of "Santa Claus Conquers the Martians" (MST3K version, of course) while waiting for this patch to pass all of the Migrate tests locally. So I'm pretty sure this is going to pass tests.

I think the new issue scope is clear.

Thanks for reviewing, @heddn! Putting this back to NR so I remember to open the follow-up, then will return it to RTBC.

Follow-up opened: #2923810: Throw exception for destination plugins without a destination_module property. So back to RTBC as per #59.

Dang it: back to NR for change record updates.

Updated this change record, so this is back to RTBC as per #59.

(still keeping one eye open on issues)

source_module is only relevant to migrations from a Drupal database, is it not? But this forces ALL source plugins (CSV, XML, etc.) to provide it, even when meaningless (I see dummy "migrate" values provided for Empty and EmbeddedData)?

This also applies (less strongly) to destination plugins (there's no reason you couldn't, now, write a destination plugin that writes to an external API).

source_module is only relevant to migrations from a Drupal database, is it not? But this forces ALL source plugins (CSV, XML, etc.) to provide it, even when meaningless (I see dummy "migrate" values provided for Empty and EmbeddedData)?

This also applies (less strongly) to destination plugins (there's no reason you couldn't, now, write a destination plugin that writes to an external API).

This is why, in retrospect, I am sad that source_module and destination_module were added to the MigrateSource and MigrateDestination annotations. They're only relevant to Migrate Drupal, but they're in all source/destination plugins. It sucks.

One possible way to make this cleaner is for Migrate Drupal to override the source and destination plugin managers, to impose the source_module and destination_module plugin definitions. That way we can prevent MIgrate from being further poisoned by Migrate Drupal's idiosyncrasies. But that said, we don't have a lot of time to bikeshed on this -- we still have entirely too many release-blocking criticals open to get caught up for long on any particular one.

Moving this back to CNR to discuss that a bit more.

I also get a bit confused by the exception handling with these, at least when running migrations via drush. The modules providing the source migrations may well be enabled on sites that don't need to migrate content from these sources (but might need to migrate content from sources they do have), however this always shows up as an error if you just do a drush mi.

Discussed briefly with @heddn on IRC, and we decided that the best idea here might be to have Migrate Drupal override the plugin.manager.migration service with a version that enforces the source_module property, since Migrate Drupal is really the only thing that flat-out requires it. This keeps the wonkiness cleanly confined to Migrate Drupal and does not further pollute the Migrate API with Migrate Drupal-specific stuff.

Back to NW for implementation.

Here's an implementation of the idea in #67.

@phenaproxima In patch #68, the check for source_module property is done in MigrateSourcePluginManager of migrate module and not by some override in migrate_drupal:

+  public function processDefinition(&$definition, $plugin_id) {
+    parent::processDefinition($definition, $plugin_id);
+
+    if (empty($definition['source_module'])) {
+      throw new BadPluginDefinitionException($plugin_id, 'source_module');
+    }
+  }
+

Wouldn't that still apply for all source plugins instead of just the drupal ones?

Am I missing something in my assumption?

Let's also ensure that the migration tags to which the source_module check should apply are configurable -- I'll add some simple config to Migrate Drupal for this purpose.

+1 for the 'Drupal 6' or 'Drupal 7' migration tag approach!

Me too! Let's use the tags to identify a drupal source and then check the source_module.

Okay, here we go. An implementation of everything we've discussed since #68.

I agree with both of @heddn's nits. Both changes made.

I thought I posted on this issue already but my comment is mysteriously missing... the framework and release managers discussed this issue awhile back and agreed that it can be major, not critical, since we can add new exceptions in minor releases. So downgrading to major, but also changing it to a task since a robust API isn't just a feature. Hopefully it doesn't matter in any case since this one is RTBC again. :)

Thanks everyone!

+++ b/core/modules/migrate/src/Plugin/MigrateSourcePluginManager.php
@@ -70,4 +71,15 @@ protected function findDefinitions() {
+  /**
+   * {@inheritdoc}
+   */
+  public function processDefinition(&$definition, $plugin_id) {
+    parent::processDefinition($definition, $plugin_id);
+
+    if (empty($definition['source_module'])) {
+      throw new BadPluginDefinitionException($plugin_id, 'source_module');
+    }
+  }

Oops! This should all be gone. Will fix in a bit.

Here we go.

1. +++ b/core/modules/migrate/src/Plugin/migrate/source/EmbeddedDataSource.php
   @@ -38,7 +38,8 @@
   + *   source_module = "migrate"
   
   +++ b/core/modules/migrate/src/Plugin/migrate/source/EmptySource.php
   @@ -19,7 +19,8 @@
   + *   source_module = "migrate",
   
   +++ b/core/modules/migrate/tests/modules/migrate_high_water_test/src/Plugin/migrate/source/HighWaterTest.php
   @@ -8,7 +8,8 @@
   + *   source_module = "migrate_high_water_test",
   
   +++ b/core/modules/migrate/tests/modules/migrate_query_batch_test/src/Plugin/migrate/source/QueryBatchTest.php
   @@ -8,7 +8,8 @@
   + *   source_module = "migrate_query_batch_test",
   

   Are these still needed now - because they're not tagged as Drupal 6/Drupal 7 and outside migrate_drupal - shouldn't be needed anymore right?

2. +++ b/core/modules/migrate_drupal/src/MigrationPluginManager.php
   @@ -0,0 +1,99 @@
   + * Manages migration plugins.
   

   Can we get a comment here indicating why we're doing this overriding (to enforce the source module property etc)

3. +++ b/core/modules/migrate_drupal/src/MigrationPluginManager.php
   @@ -0,0 +1,99 @@
   +      // Throw a loud exception if the source plugin definition does not define
   

   Do we need the word 'loud' here?

All fixed except for removing source_module from EmbeddedDataSource. That one is used in a D6/7 migration, and therefore must carry the annotation property.

+++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
@@ -18,50 +17,43 @@ class MigrationProvidersExistTest extends MigrateDrupalTestBase {
   public function testProvidersExist() {

I think the changes here have mean that the test migration, migration_provider_test.yml, is no longer needed.

Good point. I have removed that migration.

Patch no longer applies, so I rerolled it and fixed a typo. The changes were in MigrateUpgradeTestBase and MigrationPluginManager (the typo).

+++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
@@ -18,50 +17,43 @@ class MigrationProvidersExistTest extends MigrateDrupalTestBase {
+    $this->setExpectedException(BadPluginDefinitionException::class, 'The no_source_module plugin should define the source_module property.');

It is disconcerting for me to get a message that says 'should' when it is really a 'must'.

Since enforce_source_module is a setting in config, does that mean that it can be changed so that the enforcing doesn't take place?

The IS needs an update to include the decision to create a new Migrate Drupal MigrationPluginManager. And what does that mean for developers?

Try again, assertions were out of order.

The failing test is unrelated, but this adds a file doc block to migrate_drupal.install.

I like the new approach with the dedicated migration plugin manager. I don't see anything wrong in the code, the IS & CR are up to date, so RTBC.

Adding review credits

@xjm for CR updates, @dipakmdhrm, @mikeryan and myself for reviews that changed the patch

Committed as f22b038 and pushed to 8.5.x

Published change record

For posterity, the BC implications here were discussed in #19