MigrationLookup plugin overrides source values for multiple migrations

Hey Matroskeen,

have you tried this solution?

foo:
    plugin: migration_lookup
    migration:
      - migration_1
      - migration_2
      - migration_3
    source_ids:
      migration_1:
        - foo_bar
        - bar_baz
      migration_2:
        - bar
      migration_3:
        - bar

As a review swap Matroskeen asked me to, well, review this issue.

After reading the issue I understand the desire to make the yml simpler and I have added that bit to the proposed resolution. Because migration_lookup is ubiquitous we need to be very careful about changes and ultimately consult with mikelutz because he drove the work on converting it to a service.

I want to think through this slowly, If we have a process like this it will work well.

plugin: migration_lookup
  source: user
  migration: users

Then we add a lookup to another migration, it is reasonable that someone would do this and it would work.

plugin: migration_lookup
  source: user
  migration:
    - users
    - members
  source_ids:
    members:
      - id

But lets say the did this, have the members migration first. This would not work. The lookup would be done on 'members' first and the source values set to ['id']. If that fails a lookup on 'users' is done and the source values are not changes so ['id'] is used when it should be 'user'.

plugin: migration_lookup
  source: user
  migration:
    - members
    - users
  source_ids:
    members:
      - id

For this to work 'users' must be added to the source_ids, as is done in the example in MigrationLookup.

Is it a bug that this does not work? Possibly. At the very least it is a bug in the documentation that it is not explicit that when using multiple migrations with different source_ids they all must be listed. The suggested change in this patch will also require documentation changes so it seems fine to leave the status of this as a Bug.

I don't see any BC issues with this change, so that is good. I have not looked at the tests, but I do think a fail test is needed.

Changing to NW for tests and documentation.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -58,14 +58,31 @@
   + * source identifier for migration is not specified, default source value will
   + * be used. In the example below, 'author' source property will be used to
   + * do a lookup in 'users' migration, and 'uid' property in 'members' migration.
   

   This could use some minor language tightening.

2. +++ b/core/modules/migrate/tests/src/Unit/process/MigrationLookupTest.php
   @@ -251,4 +251,37 @@ public function testMultipleSourceIds() {
   +    $this->migrateLookup->lookup('foobaz', [1])->willReturn([[2]]);
   +    $this->migrateLookup->lookup('foobaz', [2])->willReturn([]);
   +    $this->migrateLookup->lookup('foobar', [1, 2])->willReturn([]);
   +    $this->migrateLookup->lookup('foobar', [3, 4])->willReturn([[5]]);
   

   Nit: $this->migrateLookup is not defined as a class property.

   I looked at the wrong file 🙈

Restoring status after HEAD was broken.

Re-testing — random fail in CKEditor tests.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -58,14 +58,32 @@
   + * It's not required to describe source identifiers for each migration. If the
   + * source identifier for migration is not specified, the default source value
   

   Supernit🧐: I think this should read 'If the source identifier for a migration'

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -178,22 +196,23 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   -      if (!is_array($value)) {
   -        $value = [$value];
   +      if (!is_array($lookup_value)) {
   +        $lookup_value = [$lookup_value];
   

   could this just be $lookup_value = (array) $lookup_value if we're touching these lines?

I applied the MR and read the patch and code. The improvements suggested in #10 and #17 have been done. The added documentation in MigrationLookup reads well and is easy to understand. The test is testing what it needs to.

Back to RTBC.

Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!