Problem/Motivation

There are several implicitily required properties to destination, source and process plugins. Rather than waiting for someone to WSOD when they miss one of these implicit requirements, we should throw an exception.

The MigrateSource annotation defines a source_module property which needs to identify the system which contains the source data. In Migrate Drupal's case, this property is used to identify the D6 or D7 module whose data is read by the plugin.

When providing a strong API, it's important to tell consumers when they are doing something wrong. For Migrate Drupal's purposes (our main use case), this property is required in order to inform users whether their D6/D7 data will be migrated into Drupal 8. Therefore, this blocks the completion of the core migration path to Drupal 8, and is Migrate-critical.

Proposed resolution

Throw a BadPluginDefinitionException for any source plugin that is missing the source_module property.

Remaining tasks

Write a patch, with a test, and commit.

User interface changes

None.

API changes

The source_module property will be required on all Migrate source plugins. So this constitutes a BC break.

However, we determined that for non Migrate Drupal use cases, not marking the source_module as required makes more sense. So we added an overridden migrate plugin manager that checks based on certain fairly standard migration_tags and only require source_module on migrations that use one of the tags. The tags are configurable and default to 'Drupal 6', 'Drupal 7'.

Data model changes

None.

CommentFileSizeAuthor
#93 interdiff_92-93.txt3.28 KBheddn
#93 2908282-93.patch27.43 KBheddn
#92 interdiff_90-92.txt2.43 KBheddn
#92 2908282-92.patch26.08 KBheddn
#90 interdiff.txt439 bytesquietone
#90 2908282-90.patch26.08 KBquietone
#88 interdiff.txt3.17 KBquietone
#88 2908282-88.patch25.99 KBquietone
#86 2908282-86.patch24.02 KBquietone
#85 interdiff-2908282-83-85.txt585 bytesphenaproxima
#85 2908282-85.patch25.94 KBphenaproxima
#83 interdiff-2908282-80-83.txt4.11 KBphenaproxima
#83 2908282-83.patch25.11 KBphenaproxima
#80 interdiff-2908282-76-80.txt1.59 KBphenaproxima
#80 2908282-80.patch27.59 KBphenaproxima
#76 interdiff-74-76.txt1.66 KBJo Fitzgerald
#76 2908282-76.patch28.45 KBJo Fitzgerald
#74 interdiff-2908282-68-74.txt5.25 KBphenaproxima
#74 2908282-74.patch28.53 KBphenaproxima
#68 interdiff-2908282-57-68.txt6.89 KBphenaproxima
#68 2908282-68.patch25.59 KBphenaproxima
#57 2908282-57.patch18.05 KBphenaproxima
#53 interdiff-2908282-50-53.txt9.18 KBphenaproxima
#53 2908282-53.patch40.64 KBphenaproxima
#50 2908282-50.patch37.95 KBJo Fitzgerald
#50 interdiff-49-50.txt4.97 KBJo Fitzgerald
#49 2908282-49.patch39.25 KBJo Fitzgerald
#41 interdiff_32-41.txt5 KBheddn
#41 2908282-41.patch39.13 KBheddn
#32 interdiff_28-32.txt2.61 KBheddn
#32 2908282-32.patch39.03 KBheddn
#28 2908282-28.patch39.38 KBJo Fitzgerald
#28 interdiff-24-28.txt10.12 KBJo Fitzgerald
#24 2908282-24.patch36.69 KBheddn
#24 interdiff_22-24.txt7.74 KBheddn
#22 2908282-22.patch27.81 KBJo Fitzgerald
#22 interdiff-16-22.txt3.27 KBJo Fitzgerald
#16 2908282-16.patch27.86 KBJo Fitzgerald
#16 interdiff-14-16.txt533 bytesJo Fitzgerald
#14 2908282-14.patch27.34 KBJo Fitzgerald
#14 interdiff-12-14.txt3.88 KBJo Fitzgerald
#12 2908282-12.patch23.46 KBheddn
#12 interdiff_10-12.txt17.41 KBheddn
#10 interdiff_5-10.txt7.44 KBheddn
#10 2908282-10.patch8.46 KBheddn
#5 2908282-5.patch3.38 KBheddn
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

heddn created an issue. See original summary.

heddn’s picture

Issue summary: View changes

Source/destination are the only questionable requirements. I'd like to make them required so contrib will provide them and they will play nicely with drush or migrate_drupal_ui that needs them. We started to something like this in #2859304: Show field type migrations correctly in Migrate Drupal UI, but I'm wondering if we *do* want to break BC here.

If that's the case, then all of these should change into an exception, instead of trigger_error.

	+  /**
+   * {@inheritdoc}
+   */
+  public function processDefinition(&$definition, $plugin_id) {
+    parent::processDefinition($definition, $plugin_id);
+
+    foreach (['id', 'core', 'source_module', 'destination_module'] as $required_property) {
+      if (empty($definition[$required_property])) {
+        @trigger_error(sprintf('The field plugin should define the %s property.', $plugin_id, $required_property));
+      }
+    }
+  }

MigrateDestination
required:

  • id
  • destination_module

not required:

  • requirements_met

MigrateProcessPlugin
required:

  • id

not required:

  • handle_multiples

MigrateSource
required:

  • id
  • source_module

not required:

  • requirements_met
phenaproxima’s picture

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.

maxocub’s picture

+1 for me too.

heddn’s picture

So, we all seem to agree that this is a BC break and should happen. So, that means it is migrate critical and a bc break. Let's get this going then. BTW, #2859304: Show field type migrations correctly in Migrate Drupal UI covers the field plugin. This catches the rest of them.

heddn’s picture

heddn’s picture

I think we already have the change records, we just need to make sure they are updated to note that these are hard required values.

Status: Needs review » Needs work

The last submitted patch, 5: 2908282-5.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
Issue tags: +Novice
FileSize
8.46 KB
7.44 KB

Fixing some missing source_module source plugins. I'm sure there are more. Tagging novice to have someone run through and add the source module to more things.

Status: Needs review » Needs work

The last submitted patch, 10: 2908282-10.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
17.41 KB
23.46 KB

This should make things easier to find the offending plugins, by printing the class name. And I've gone ahead and fixed several. There's bound to be more.

Status: Needs review » Needs work

The last submitted patch, 12: 2908282-12.patch, failed testing. View results

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
3.88 KB
27.34 KB

Fixing some more missing source_module and destination_module plugins.

Status: Needs review » Needs work

The last submitted patch, 14: 2908282-14.patch, failed testing. View results

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
533 bytes
27.86 KB

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.

Status: Needs review » Needs work

The last submitted patch, 16: 2908282-16.patch, failed testing. View results

heddn’s picture

Instead of migrate_drupal on the source side, we could probably use system.

xjm’s picture

Version: 8.4.x-dev » 8.5.x-dev
Category: Task » Bug report
Priority: Major » Critical

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.

xjm’s picture

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

xjm’s picture

Issue tags: +Needs change record

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.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
3.27 KB
27.81 KB

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

Status: Needs review » Needs work

The last submitted patch, 22: 2908282-22.patch, failed testing. View results

heddn’s picture

This should hopefully fix up the remaining tests and adds some test coverage.

phenaproxima’s picture

Status: Needs review » Needs work

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.

phenaproxima’s picture

maxocub’s picture

Issue tags: +Vienna2017

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

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
10.12 KB
39.38 KB
  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.
heddn’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate/src/Plugin/MigratePluginManager.php
    @@ -69,10 +69,8 @@ public function createInstance($plugin_id, array $configuration = [], MigrationI
    +    if (empty($definition['id'])) {
    

    This is actually required by the plugin system, I found out while working on tests. ID can go away. It is unreachable code.

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/NullDestination.php
    @@ -11,7 +11,7 @@
    + *   destination_module = "-1",
    

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

phenaproxima’s picture

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")

heddn’s picture

Going to work on my nits.

heddn’s picture

Status: Needs work » Needs review
FileSize
39.03 KB
2.61 KB

Added the tests from #30 and picked up the nits from #29.

Since the null destination is actually necessary to be a valid destination, and it cannot be a migrate namespace module (we have tests making sure it isn't). And we are in-fact, mapping the data to a null destination. Therefore, semantically speaking, '/dev/null' is more clear that the data will not get imported.

guilopes’s picture

Status: Needs review » Reviewed & tested by the community

the #32 It looks good for me

heddn’s picture

What about #32 looks good. Be specific.

guilopes’s picture

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

guilopes’s picture

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

phenaproxima’s picture

+++ 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().

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work

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

phenaproxima’s picture

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.

heddn’s picture

Issue tags: -Needs change record

Added a CR. We'll have to update the existing CR to cross-reference once this lands.

For destination... perhaps then it isn't really required? I'm going to try that.

heddn’s picture

Status: Needs work » Needs review
FileSize
39.13 KB
5 KB

I've updated the new CR to only reflect source_module being required. And updated tests and implementation accordingly.

quietone’s picture

Status: Needs review » Needs work

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/

heddn’s picture

Good points in #42. This was too narrow of a focus. This definitely needs more work.

heddn’s picture

Assigned: Unassigned » heddn

Assigning to myself to investigate more this week.

heddn’s picture

Discussed in the migrate maintainers meeting.

So, the preferred location for data is yaml entry (for variables, etc), followed by annotation. We do not recommend using a code based solution on the getters.

I don't know if that means we have to wait until after plugin discovery to test this, but we need to find the place where it is still in discovery mode so we can throw the exception as early as possible. We want to be able to have the exception thrown when you createInstance, etc in a kernel test.

heddn’s picture

I'm hoping to set aside some time this next week to review this again.

phenaproxima’s picture

Status: Needs work » Postponed

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.

Gábor Hojtsy’s picture

Jo Fitzgerald’s picture

Assigned: heddn » Jo Fitzgerald
Status: Needs work » Needs review
FileSize
39.25 KB

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

Jo Fitzgerald’s picture

Assigned: Jo Fitzgerald » Unassigned
FileSize
4.97 KB
37.95 KB
  • Employ the new Migrate-specific exception.
  • Correct some coding standards errors.

The last submitted patch, 49: 2908282-49.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 50: 2908282-50.patch, failed testing. View results

phenaproxima’s picture

Category: Bug report » Feature request
Status: Needs work » Needs review
Issue tags: +Needs issue summary update
FileSize
40.64 KB
9.18 KB

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.

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue rescope, +Needs followup

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
phenaproxima’s picture

Title: Throw exception if required migration plugin properties are missing » Throw exception for source plugins without a source_module property
Issue summary: View changes
Issue tags: -Needs issue summary update

Fixed the IS.

phenaproxima’s picture

Issue summary: View changes

Minor IS fixes.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
18.05 KB

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.

phenaproxima’s picture

Issue tags: -Needs issue rescope

I think the new issue scope is clear.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. I reviewed the code and the assignment to (mainly) system as the source. All the values all make sense. LGTM.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review

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

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs followup
Related issues: +#2923810: Throw exception for destination plugins without a destination_module property
phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review

Dang it: back to NR for change record updates.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record updates

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

mikeryan’s picture

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

phenaproxima’s picture

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.

catch’s picture

Status: Reviewed & tested by the community » Needs review

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.

phenaproxima’s picture

Status: Needs review » Needs work

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.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
25.59 KB
6.89 KB

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

dipakmdhrm’s picture

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

heddn’s picture

Status: Needs review » Needs work

re #69: I had the same thought over the weekend. I chatted briefly with @phenaproxima in IRC this morning and one idea he suggested was to look for the 'Drupal 6' or 'Drupal 7' migration tags. That's a pretty safe bet, and we are really trying to help folks here when they are missing required data. So it doesn't need to be 100% foolproof. Just catch the majority of users and tell them when they are missing required data. And I think checking the tags is a pretty good method.

phenaproxima’s picture

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.

masipila’s picture

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

quietone’s picture

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

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
28.53 KB
5.25 KB

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

heddn’s picture

Status: Needs review » Needs work

Nothing but nits:

  1. +++ b/core/modules/migrate_drupal/src/MigrationPluginManager.php
    @@ -22,6 +23,20 @@
    +  protected $sourceModuleTags;
    

    $enforcedSourceModuleTags?

  2. +++ b/core/modules/migrate_drupal/src/MigrationPluginManager.php
    @@ -32,10 +47,27 @@
    +  protected function getSourceModuleTags() {
    

    Bikeshed: getEnforcedSourceTags is better or just different?

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
28.45 KB
1.66 KB

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

heddn’s picture

Status: Needs review » Reviewed & tested by the community

RTBC then?

xjm’s picture

Category: Feature request » Task
Priority: Critical » Major

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!

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
27.59 KB
1.59 KB

Here we go.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

And back to RTBC.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
  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?

phenaproxima’s picture

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.

quietone’s picture

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

phenaproxima’s picture

Good point. I have removed that migration.

quietone’s picture

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?

Status: Needs review » Needs work

The last submitted patch, 86: 2908282-86.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
25.99 KB
3.17 KB

Try again, assertions were out of order.

Status: Needs review » Needs work

The last submitted patch, 88: 2908282-88.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
26.08 KB
439 bytes

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

heddn’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs issue summary update

Updated IS.

To clarify for the question #86 about should vs must. I think this is a grammar issue. I consider 'should' to be equal to 'must'. However, I guess in theory if you were knowledgeable and wanted to, you could disable exception throwing by altering the config so it didn't flag exceptions on Drupal sourced data. Why you would want to, I'm not sure.

  1. +++ b/core/modules/migrate_drupal/config/install/migrate_drupal.settings.yml
    @@ -0,0 +1,5 @@
    +enforce_source_module:
    

    Let's use the word, "tag" in the name of the config setting.

  2. +++ b/core/modules/migrate_drupal/migrate_drupal.install
    @@ -0,0 +1,16 @@
    +function migrate_drupal_update_8401() {
    

    Should this move to 8.5?

heddn’s picture

heddn’s picture

FileSize
27.43 KB
3.28 KB

And a fix from #86. for the feedback about must vs. should

Our pattern here is commerce, which also uses the language 'must'.

The checkout pane %s must define the %s property.

heddn’s picture

Issue tags: +Migrate January 2017 Sprint
heddn’s picture

Issue tags: -Migrate January 2017 Sprint +Migrate January 2018 Sprint
maxocub’s picture

Status: Needs review » Reviewed & tested by the community

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.

larowlan’s picture

Adding review credits

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

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed as f22b038 and pushed to 8.5.x

Published change record

  • larowlan committed f22b038 on 8.5.x
    Issue #2908282 by heddn, phenaproxima, Jo Fitzgerald, quietone, xjm,...
larowlan’s picture

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

Status: Fixed » Closed (fixed)

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