Migration process plugin not working with multiple source IDs

This seems to work fine in general, I've seen no such problem - can you provide more details on your case? What does your .yml look like? What about the source data passed to the migration plugin?

I had the problem. Here are the interesting parts of my migration module.

Dependent migration:

source:
  plugin: billet_node
process:
  mots_cles:
    plugin: migration
    migration:
      - motcle_term
    source_ids:
      motcle_term:
        - cat_ID
        - blog_id

/**
 * Source plugin for billets.
 *
 * @MigrateSource(
 *   id = "billet_node"
 * )
 */
class BilletNode extends SqlBase {
  /**
   * {@inheritdoc}
   */
  public function fields() {
$fields = array(
      'blog_id' => $this->t('Blog ID'),
      'cat_ID' => $this->t('Term ID'),
    );
    return $fields;
  }

Dependency migration:

source:
  plugin: motcle_term

/**
 * Source plugin for mots-clés.
 *
 * @MigrateSource(
 *   id = "motcle_term"
 * )
 */
class MotCleTerm extends SqlBase {
  /**
   * {@inheritdoc}
   */
  public function getIds() {
    return [
      'cat_ID' => [
        'type' => 'integer',
        'unsigned' => FALSE,
        'alias' => 'wpt',
      ],
      'blog_id' => [
        'type' => 'string',
        'max_length' => 128,
        'is_ascii' => TRUE,
        'alias' => 'wpt',
      ],
    ];
  }

The last patch fixed it.

I've seen some of this interesting behavior with the migrate process plugin before. We discuss some of the limitations in: https://www.mtech-llc.com/blog/charlotte-leon/migration-csv-data-paragraphs

However, to get this going, I really think we need a test that demonstrates the problem.

Hmm, I mis-spoke. We didn't find anything interesting with migrate process plugin. It was rather iterator and extract. But we still need tests.

I've had a go at creating a test for this issue, but I've come up short. Here is my attempt so far, perhaps someone can extend it. No point in running the testbot on this (it passes locally, but isn't really testing anything).

I eventually got to the bottom of it - migrations with multiple source_ids may not (do not?) pass a value to transform() (and if they do it is ignored). But with the current location of skipOnEmpy() these rows are skipped, in error. I have tweaked and updated @Stefan Freudenberg's patch and produced a test-only patch to prove the error.

I haven't provided an interdiff because I basically worked from scratch (and suspect that the 7-month old original patch would have required a reroll anyway).

Will review this in the coming week.

Tagging for reroll. Migration process plugin was re-named in 8.4.x. Also tagging for backport, as this is a bug and therefore could make its way back into 8.3.

My one piece of feedback about the test, there's an awful lot of mocking going on. I feel it would be easier to read the test if a Kernel test was developed that used the embedded_data, source plugin. It's a little more effort and would take longer to run, but it would also make the scenario crystal clear. @jofitz, on the other hand, this is a fairly odd one-off. So there isn't a huge need to make all tests readable and I trust this does test the scenario. It is just hard to read.

Re-roll the patch against 8.3.x & 8.4.x branch for the comment #13.

In my case source had hierarchy encoded in two text/string fields.
Don't know enough about migration's inner workings to comment on the solution, but it works fine after the patch.
Taking a stab at the failing test.

Missed a spot.

Add related issues

Some examples this makes possible:

1. item has an owner which comes from one of 2 source tables, depending on type specified

// type=1 item owners table with ids overlapping with PersonNode table
class CompanyNode extends SqlBase {
  public function getIds() {
    return [
      'type' => [
        'type' => 'integer',
        'alias' => 'cm',
      ],
      'id' => [
        'type' => 'integer',
        'alias' => 'cm',
      ],
    ];
  }
}

// type=2 item owners table with ids overlapping with CompanyNode table
class PersonNode extends SqlBase {
  public function getIds() {
    return [
      'type' => [
        'type' => 'integer',
        'alias' => 'pr',
      ],
      'id' => [
        'type' => 'integer',
        'alias' => 'pr',
      ],
    ];
  }
}

class ItemNode extends SqlBase {
  public function fields() {
    $fields = [
      'owner_type' => $this->t('Item owner type'), // integer 1:company or 2:person
      'owner_id' => $this->t('Item owner ID'),
      'brand' => $this->t('Item brand'), // string
      'model' => $this->t('Item model'), // string
    ];
  }
}

source:
  plugin: item_node

field_item_owner:
  plugin: migration
  migration:
    - company_node
    - person_node
  source_ids:
    company_node:
      - owner_type
      - owner_id
    person_node:
      - owner_type
      - owner_id

2. hierarchical taxonomy encoded in source table as two varchar columns

class BrandModelTerm extends SqlBase {
  public function getIds() {
    return [
      'brand' => [
        'type' => 'string',
        'alias' => 'ms',
      ],
      'model' => [
        'type' => 'string',
        'alias' => 'ms',
      ],
    ];
  }
}

source:
  plugin: item_node

field_item_model:
  plugin: migration
  migration:
    - brand_model_term
  source_ids:
    brand_model_term:
      - brand
      - model
  no_stub: true

Assigning to myself to review this week.

Note/disclaimer: my use case may be a bit different than the one described in the comments here but it does fit the title and description of this issue. I'm documenting it here for others that may encounter the same use case.

We have several migrations that migrate from different node types in Drupal 7 to one node type in Drupal 8 (we merge "restaurant", "hotel" & "spot" into "poi").

All these source node types contain a node reference to each other for related items.

This is an example on one of the migrations:

  field_related_poi:
    plugin: iterator
    source: field_related
    process:
      target_id:
        plugin: vg_migration_lookup_poi
        migration:
          - node_spot
          - node_restaurant
          - node_hotel
        source: nid
        stub_id: node_spot

Since the patch in #19 did not solve the issue and using a single stub_id in this case is not an option, we extended MigrationLookup to set the proper stub_id based on a query of the node type in the source database.

Needs re-roll. And one quick note while doing that.

+++ b/core/modules/migrate/tests/src/Unit/process/MigrationLookupTest.php
--- a/core/modules/migrate/tests/src/Unit/process/MigrationTest.php
+++ b/core/modules/migrate/tests/src/Unit/process/MigrationTest.php

This isn't needed. It is now deprecated.

Re-rolled.
Removed changes to MigrationTest.php because Migration.php is deprecated.

Removed too much from MigrationTest.php! Corrected.

This has test coverage. See #10. And it fixes an issue for a fairly complicated scenario. Good work.

Tagging

Adding credit for reviewers and those who provided sample migrations to help get to the bottom of the issue

Committed as f37dea0 and pushed to 8.5.x

This was never committed to the 8.4.x branch. Comment #23 removed the "Needs backport to 8.3.x" tag, but it seems this at least needs to be in 8.4.x.

Thankfully, 2796393-28.patch from #28 applies cleanly to the end of the 8.4.x branch (or the 8.4.3 tag if anyone else needs to deploy this now) and solves the bug.

I was being nailed by this via a CSV migration involving node authorship not being able to lookup UIDs from a user migration that needed multiple keys. With #28 applied, the migration is now working properly.

Back to RTBC.

Thanks!
-Derek

As per https://www.drupal.org/core/release-cycle-overview#current-development-c... 8.4 is in 'critical fixes only', so this can't be backported to 8.4

Sorry, I don't want to make trouble and get in a status pushing match, but either the page you linked to is wrong, or you forgot about January. ;)

Week of January 17, 2018 : 8.5.0-alpha1 released and 8.6.x-dev opened.
February 7, 2018 : 8.5.0-beta1 released. Final patch release window for 8.4.x (criticals only).

As of this writing (Jan 8th), we're still 9 days from the date when 8.5.0-alpha1 is supposed to be released, and we've got a full month before that table claims that 8.4.x is in "criticals only" mode.

Tentatively re-opening this for commit to 8.4.x. It changes no API (other than fixing a migration plugin to work as advertised), it's been multiply reviewed, comes with tests, myself and others have also manually tested, and it's already in 8.5.x.

Thanks,
-Derek

The next patch release window is February 7. As indicated there, is for critical issues only. This is not a critical. It will be included in 8.5.0 on March 7.

Marking back to fixed. Thanks!

wow - glad there is a patch here at least. I thought I was going crazy that the documented definition with 2 source_ids didn't work.

Thanks for the fix!

One issue on the patch: The documentation on lines 63 and 75 of MigrationLookup.php is wrong. 'souce_ids' should not be indented below 'migration'