I have a list of migrations which are split by groups. And some of the migrations have cross-group dependencies, i.e.:

groupA 
  migration_A depends on migration_B
groupB
  migration_B

In this case drush_migrate_tools_migration_list() function will break migration dependencies because it arranges them by groups:

  ...
  // Sort the matched migrations by group.
  if (!empty($matched_migrations)) {
    foreach ($matched_migrations as $id => $migration) {
      $configured_group_id = empty($migration->get('migration_group')) ? 'default' : $migration->get('migration_group');
      $migrations[$configured_group_id][$id] = $migration;
    }
  }
  return isset($migrations) ? $migrations : [];

@see: http://cgit.drupalcode.org/migrate_tools/tree/migrate_tools.drush.inc#n490

I personally don't see a great value in running migrations by groups, it's more like an aesthetic thing. I'll try to provide a patch.

CommentFileSizeAuthor
#18 migrate-tools.2830357.18.patch5.45 KBmanarth
#17 migrate-tools.2830357.17.patch5.43 KBmanarth
#14 2830357-14.patch3.9 KBrakesh.gectcr
#12 migrate_tools.2830357.12.patch6.04 KBmanarth
#11 migrate_tools.2830357.11.patch6 KBmanarth
#8 migrate_tools-2830357-8.patch3.97 KBOnkelTem
#3 migrate_tools-2830357.patch2.78 KBOnkelTem

Issue fork migrate_tools-2830357

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

OnkelTem created an issue. See original summary.

OnkelTem’s picture

OnkelTem commented
Issue summary: View changes
OnkelTem’s picture

OnkelTem commented
Status: Active » Needs review
StatusFileSize
new migrate_tools-2830357.patch2.78 KB
heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

I'm not sure how BC we want this to be. If we wanted full BC, we'd add a new argument to ignore by grouping and default it to false. But maybe it isn't that important and we just remove that functionality?

Grayside’s picture

Grayside commented

This might explain why migration dependencies were mysteriously failing across a couple dozen migrations, split across a half-dozen groups.

Since the current behavior is a consistency bug in which dependency is not uniformly enforced, it seems like b/c is unnecessary. There's already --force to ignore dependency if that's needed.

OnkelTem’s picture

OnkelTem commented

After few weeks of testing I can say it works fine for our tasks.

@Grayside
> There's already --force to ignore dependency if that's needed.

Sure, but from my experience I can say that ignoring any dependency is a certain way to miss an error. I've found only one use case when we might need this: it's when two or more migrations depend on one "with a dynamic nature" like XML feed for example, i.e. one which can change during the migration process, making it impossible to finish it to the end.

Grayside’s picture

Grayside commented

OnkelTem I'm arguing that backwards compatibility to the current broken behavior is not needed because --force exists.

OnkelTem’s picture

OnkelTem commented
StatusFileSize
new migrate_tools-2830357-8.patch3.97 KB

The #3 patch is not complete, as it's missing the rollback part.

Updating the patch.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Needs work 
  1. +++ b/migrate_tools.drush.inc
@@ -436,11 +431,14 @@ function drush_migrate_tools_migrate_fields_source($migration_id) {
+ *  A flag indicating whether migration list should be arranged by migration groups. Default: TRUE

    Coding standards - two-space indentation, 80-char line. 

  2. +++ b/migrate_tools.drush.inc
@@ -436,11 +431,14 @@ function drush_migrate_tools_migrate_fields_source($migration_id) {
+ * @return \Drupal\migrate\Plugin\MigrationInterface[][] An array keyed by migration group, each value containing an array of

    Following line appears to be accidentally appended here. 

  3. +++ b/migrate_tools.drush.inc
@@ -489,10 +487,15 @@ function drush_migrate_tools_migration_list($migration_ids = '') {
     foreach ($matched_migrations as $id => $migration) {
       $configured_group_id = empty($migration->get('migration_group')) ? 'default' : $migration->get('migration_group');
       $migrations[$configured_group_id][$id] = $migration;
     }
   }

    This needs to be indented. 

  4. +++ b/migrate_tools.drush.inc
@@ -489,10 +487,15 @@ function drush_migrate_tools_migration_list($migration_ids = '') {
+      $migrations = $matched_migrations;

    This violates the documented return, which is an array of array of migrations (and required the import and rollback commands to change to accommodate it). How about

    $migrations['all'] = $matched_migrations;

    and then the changes (other than passing FALSE where necessary) are isolated here?

Thanks!

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Version: 8.x-3.x-dev » 8.x-4.x-dev
manarth’s picture

manarth commented
Version: 8.x-4.x-dev » 6.0.x-dev
Status: Needs work » Needs review
StatusFileSize
new migrate_tools.2830357.11.patch6 KB

Updated patch against the 6.0.x branch.

  • listMigrations() now returns a single-dimensional array (ungrouped, ordered by dependencies).
  • Grouping is added in the migrate:status command.
  • The --limit property is now treated as a total items to migrate, rather than a limit-per-migration. E.g. drush migrate:import --all --limit=200 will process the migration definitions in dependency order, migrating up to a combined total of 200 items.
manarth’s picture

manarth commented
StatusFileSize
new migrate_tools.2830357.12.patch6.04 KB

Patch updated to also perform migration when --limit is not specified.

Status: Needs review » Needs work

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

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language English
Location Manchester
commented
Version: 6.0.x-dev » 6.0.1
StatusFileSize
new 2830357-14.patch3.9 KB

Re-rolled

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language English
Location Manchester
commented
Status: Needs work » Needs review

Status: Needs review » Needs work

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

manarth’s picture

manarth commented
Status: Needs work » Needs review
StatusFileSize
new migrate-tools.2830357.17.patch5.43 KB

Rerolled patch from 12 (patch is now against release 6.0.1).

manarth’s picture

manarth commented
StatusFileSize
new migrate-tools.2830357.18.patch5.45 KB

Correct the filepath from patch in 17 to include the full module path (not just the bare file).

Status: Needs review » Needs work

The last submitted patch, 18: migrate-tools.2830357.18.patch, failed testing. View results

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

Looks like there are still some failing tests. Back to NW.

baikho made their first commit to this issue’s fork.