Problem/Motivation

I wrote a bunch of migrations which migrate a site from D7 to D8 and I need to duplicate meticulously every migration used by the migrate process plugin in migration_dependencies to make sure they run in the right order.

Proposed resolution

Make the mother migration plugin gather the dependencies from the plugin definition. Make it optional to avoid any headaches.

Remaining tasks

None.

User interface changes

None.

API changes

Less work, yay.

Data model changes

None.

CommentFileSizeAuthor
#19 2750639_13_17_interdiff.txt939 bytesmikeryan
#17 2750639_17.patch2.98 KBchx
#13 2750639_13.patch3.03 KBchx
#10 interdiff.txt1.55 KBchx
#10 2750639_10.patch3.03 KBchx
#8 2750639_8.patch3.04 KBchx
#4 2750639_4.patch3.17 KBchx
#2 2750639_2.patch2.93 KBchx

Comments

chx created an issue. See original summary.

chx’s picture

chx commented
StatusFileSize
new 2750639_2.patch2.93 KB
benjy’s picture

benjy commented
Status: Needs review » Reviewed & tested by the community

Looks good to me, remarkably simple really.

chx’s picture

chx commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2750639_4.patch3.17 KB

Let's add a test for iterator as well.

benjy’s picture

benjy commented
Status: Needs review » Reviewed & tested by the community

More tests :)

The last submitted patch, 2: 2750639_2.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 2750639_4.patch, failed testing.

chx’s picture

chx commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2750639_8.patch3.04 KB

Apparently I forgot to git pull before I posted this...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2750639_8.patch, failed testing.

chx’s picture

chx commented
Status: Needs work » Needs review
StatusFileSize
new 2750639_10.patch3.03 KB
new interdiff.txt1.55 KB

CckMigration overrides getProcess and gets getMigrationDependencies into an infinite loop. Neat. Avoiding that is easy.

benjy’s picture

benjy commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2750639_10.patch, failed testing.

chx’s picture

chx commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2750639_13.patch3.03 KB

Good ole' game: keeping up with HEAD.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Reviewed & tested by the community » Needs work

I like the idea, but... What happens with circular migration references?

article.yml:

process:
  field_related_blog:
    plugin: migration
    migration: blog
    source: field_related_blog

blog.yml:

process:
  field_related_article:
    plugin: migration
    migration: article
    source: field_related_article

The optional dependencies are used in sorting, and I don't know how it would handle circular references - I think we at least need a test for that scenario. And I'm betting we will need to prevent it (do not add a dependency on another migration if that migration already depends on us, directly or indirectly).

chx’s picture

chx commented
Status: Needs work » Needs review 
    // Nothing to do, if we already visited this vertex.
    if (isset($this->graph[$start]['paths'])) {
      return;
    }

in Drupal\Component\Graph\Graph::depthFirstSearch. And no, we do not need a test here. Adding a circular graph test to Drupal\Tests\Component\Graph\GraphTest is not a bad idea but definitely a separate issue.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Needs work

I would feel better testing circular dependencies, but that seems a little more involved than I first imagined... In attempting it, though, I noticed some cruft left over from the original test:

  1. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationTest.php
@@ -33,9 +33,30 @@ public function testGetProcessPlugins() {
-    $migration = \Drupal::service('plugin.manager.migration')->createStubMigration([
...
         'migration_dependencies' => NULL
     ]);

    Unused migration still created. 

  2. +++ b/core/modules/migrate/tests/src/Kernel/Plugin/MigrationTest.php
@@ -33,9 +33,30 @@ public function testGetProcessPlugins() {
     $this->assertNotEmpty($migration->getMigrationDependencies(), 'Migration dependencies is not empty');

    This assertion is now redundant.

chx’s picture

chx commented
Status: Needs work » Needs review
StatusFileSize
new 2750639_17.patch2.98 KB

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Version: 8.3.x-dev » 8.2.x-dev
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 2750639_13_17_interdiff.txt939 bytes

Finally checked back, interdiff looks good, back to RTBC.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes

  • catch committed 9764259 on 8.3.x 
    Issue #2750639 by chx, mikeryan: Gather some migration dependencies...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x and 8.2.x, thanks!

  • catch committed 55d9e71 on 8.2.x 
    Issue #2750639 by chx, mikeryan: Gather some migration dependencies...

Status: Fixed » Closed (fixed)

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