Problem/Motivation

In #2935951-27: Copy migrate source plugin from migrate_drupal_d8 into migrate_drupal, it was caught that the source_module wasn't correctly set for this new source plugin. Which brought up a good question, why wasn't this caught in tests. Let's investigate and fix.

Proposed resolution

Investigate

Remaining tasks

  • Investigate
  • Fix
  • Review

User interface changes

n/a

API changes

n/a

Data model changes

n/a

CommentFileSizeAuthor
#9 interdifft.txt1.73 KBquietone
#9 2937045-9.patch1.92 KBquietone
#5 2937045-5.patch1.77 KBquietone
#5 2937045-5-fail.patch2.36 KBquietone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn created an issue. See original summary.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented

We should also check on empty source module.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Assigned: Unassigned » quietone

The problem is that testProvidersExist() get all the migrations and then checks the source plugins on each migration and thus misses any source plugin that is not given in a migration. I've got a draft patch working but it is too late to finish it now.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Active » Needs review
FileSize
2937045-5-fail.patch2.36 KB
2937045-5.patch1.77 KB

And the original test missed all the derived versions as well.

This version gets all the source plugin definitions and tests for source_module against all the definitions. The test checks if it is the 'empty' plugin and tests that that plugin does not have a source_module definition.

The fail patch is with content_entity set to source_provider, which is what is was when the problem was identified.

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

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Assigned: quietone » Unassigned
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented 
+++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationProvidersExistTest.php
@@ -31,16 +31,21 @@ public function testSourceProvider() {
+    // List of source plugins that do not require a source_module definition.
+    $source_module_not_required = ['empty'];
...
+      if (!in_array($id, $source_module_not_required)) {
+        $this->assertArrayHasKey('source_module', $definition, "No source_module property in '$id'");

This is all fine and well, but that means the empty source plugin cannot be used as a Drupal source as we will throw an exception in that case. Could/should we add a source_module of 'migrate' to specify its source data is coming from the migrate system itself? Or somehow add more documentation on it for when it should be used. I'm fine if we opt for a follow-up if all we plan to do is add docs.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
2937045-9.patch1.92 KB
interdifft.txt1.73 KB

Could/should we add a source_module of 'migrate' to specify its source data is coming from the migrate system itself?

Yes, we already have embedded_source with a source_module = 'migrate' and source_module is now required so it must be required everywhere. I made this patch with that in mind.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
Status: Needs review » Reviewed & tested by the community

Perfect. Let's get this merged.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented

+1 for RTBC.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented

Soft blocking #2923810: Throw exception for destination plugins without a destination_module property on this issue.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2937045-9.patch, failed testing. View results

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
Status: Needs work » Reviewed & tested by the community

Temporary testbot hiccup.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2937045-9.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review

Retesting

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Rerun test to be sure and it is still passing tests. Back to RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented

Crediting @heddn for reviewing the patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 9d04cb49bd to 8.6.x and f4d44ba1ab to 8.5.x. Thanks!

  • alexpott committed 9d04cb4 on 8.6.x 
    Issue #2937045 by quietone, heddn: source plugin source_module testing...

  • alexpott committed f4d44ba on 8.5.x 
    Issue #2937045 by quietone, heddn: source plugin source_module testing...

Status: Fixed » Closed (fixed)

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