MigrateLookup plugin has inconsistent return values.

Adding related issues.

Found another one.

I fully understand the issue here. If we could provide a test of this, I think the fix proposed is a really great suggestion.

As a work around, can we use callback process plugin and call array_shift/array_pop?

I don't think so. At minimum it would trigger a warning, since callback uses call_user_func, which passes parameters by value, and array_shift/pop expect references.

This is blocking progress on #2911244: Field collections deriver and base migration.

I'm curious as to what would break if the plugin simply returned an array with the results returned twice, under both keys. Struggling with a BC way to fix this other than adding a new optional config parameter to select the return keys, and leaving the current behavior alone if it is absent.

rebase against new 8.7

Let's try this again using the correct keys......

And one other option with a new configuration value instead.

Alright, 13 breaks too much, not surprised, so let's throw 14 out for feedback.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -31,6 +31,8 @@
   + * - return_associative: Returns multivalue keys in an associative array if true
   + *   and in an indexed array if false.
   

   This should be defined as optional, partly for BC, and also because it's easier to simply omit something rather than declare FALSE.

   Should also say what it's keyed by if you request associative.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -150,6 +152,8 @@ public static function create(ContainerInterface $container, array $configuratio
   +    $force_assoc = isset($this->configuration['return_associative']) && $this->configuration['return_associative'] == TRUE;
   

   (Can't wait for https://www.drupal.org/project/drupal/issues/2937177 to get in and remove the need for issets() all the time!)

In regards to both points, the issue is that (at least the way I structured this patch) omitting the configuration key is different than setting it to false. Omitting the key retains the current functionality (associative array if a stub is created and indexed if the lookup succeeds), while setting it to false returns an indexed array in either case.

I'm not a fan of this for several reasons, but I'm completely open to a better way to preserve BC while improving the functionality. In a perfect world, the plugin should return an associative array by default, and the extra configuration shouldn't be necessary, but I can't see a way to do that without breaking BC.

Another option would be to have two new configuration bools, force_associtive and force_indexed. This would allow the legacy behavior if both were false, but you have to deal with the error case where someone sets them both true.

You could have a configuration option that takes one of three predefined string keys, like return_array: associative|indexed|legacy. That effectively follows the same logic path as this patch, but perhaps with a slightly better DX. You could set the default to legacy and deprecate it, which basically returns us to this patch, where the param isn't really optional, because omitting it returns the current broken behavior that we want to get away from.

Plus all of this only comes into play if there are more than one destination id anyway, since (in yet another inconsistency), this plugin returns an array if there are multiple keys, and a single value if there is only one. Fortunately, you tend to know what your migrations destination ids are when you call this and admittedly, in the common case of just getting an entity_id back it's nice to be done in your process without going through an extract. But I wonder with new content moderation and workspaces initiatives, if it's not going to be more common to need an entity_id and revision id back.

Finally, we could combine this along with #2891073: MigrationLookup doesn't create stub when there's multiple migrations., #2890844: Migration Lookup plugin does not return multiple values when matched, #2804893: MigrationLookup process plugin not extensible, and possibly support for a lookup by migration tag, which I've also wanted to add, and create a brand new functional migrate_map_lookup plugin that works in a sane way and deprecate migration_lookup. Obviously a fair amount of work to update all the usages and tests in core, and I hate having 2 deprecated migrate_lookup plugins, but it might be worth it to have a consistent return array

I'd love to come up with a long term plan, I'm happy to try to do some work on it if we can figure out where we want to end up.

Discussed in migrate maintainers meeting with @phenaxproxima, @mikelutz, @maxocub, @quietone and @heddn. We suggested we'd like to use a config option for the process plugin to always return as a associative array and if that option isn't set, to trigger_error.

Well, lets see how bad this is....

This fixes one oops...

I don't know that this is as cut and dried as I thought it would be. I was working on the assumption that all stubs return an associative array, but that isn't true. I'm not even sure that any core destination returns an associative array. most destinations extend off of the Entity destination, which returns [$entity_id]. ERR in contib returns an associative array of id and revision id, but that's the only one I see.

There's definitely a core issue here where the return value of stubbed and lookup values are different, but I'm suddenly less certain of the best way to resolve the discrepancy. I do like the way the migrations look when I switch them over to expecting a keyed array back from this plugin. it adds a little to the migration yml, but I think it becomes readable and more obvious what values you are looking at

At the end of the day, migrate_lookup doesn't get to control what DestinationPluginInterface::import() returns, we almost need to create the stub, and then call lookup destination ids again to guarantee the return values are indistinguishable, which may be what I have to do..

Let's see what the testbot says.

I've found several things failing since I posted that one. Not much point in running the testbot against it until I get a new version.

A few more fixes..

Bah, wrong files...

Needed a reroll

I've been using this patch, only for the change on class SubProcess, which seems to work as expected.

Very quick review of the other parts.

1. +++ b/core/modules/aggregator/migrations/d6_aggregator_item.yml
   @@ -9,9 +9,15 @@ source:
   +      return_associatve: TRUE
   

   There's a typo in a few places: return_associatve should be return_associative

2. +++ b/core/modules/aggregator/migrations/d7_aggregator_item.yml
   @@ -9,9 +9,15 @@ source:
   +      - fid
   

   Indentation issue.

Rerolled the patch & comments addressed in #35

The following point about the patch is not a bug but an enhancement so it might seem out of scope for the issue but I'd like to hear some thoughts on this and we can always create a follow-up.

For me, the overall change makes sense here but there is a disconnect between the input and the output. MigrateIdMapInterface::lookupDestinationId() allows associate array for IDs and currently there is no way to send the associated values array using MigrateLookup plugin to MigrateIdMapInterface::lookupDestinationId(). If we allowing retruned value to be an associate array then we should also allow MigrateLookup plugin to pass associate array for IDs MigrateIdMapInterface::lookupDestinationId() using $this->configuration or $value.

FYI, most of this is getting abstracted/refactored away in #3004927: Create Migration Lookup and Stub services

Migration from D7 site with translated nodes and no_stub: true:

MigrationLookup returns:

• an entity_id if source langage of node matches source language of node in entity reference field.
• an indexed array with an entity_id and the language prefix of the node in the entity reference field if the source languages do not match. The migration fails with [error] Value is not a valid entity. from EntityReference class.

Not sure whether this is a same, related or child issue.

A quick fix for this case (but not a solution) is to always return reset($destination_ids) in MigrationLookup.

I did a best effort rebase and committed it to a new MR. A lot changed since 8.8 so some things might not be right, but let's see what the test are saying. I tested manually with a custom migration and that seems to be working.

We aren't going to make changes here, closing this in favor of #3246666: Deprecate migrate_lookup and replace with cleaner process plugin

@DieterHolvoet, thanks for updating the patch but I don't see why a new MR was created after a migrate maintainer set the issue status to "won't fix". Credit stays because of the work in good faith on the initial patch update.

@Rajeshreeputra, this issue has a status of "won't fix". Adding an MR is of no help and causes unnecessary testing. Therefor, credit has been removed per How is credit granted for Drupal core issues.