The entity_lookup plugin should preserve the source order

Hi,

I faced the same issue on one of our migrations. I came up with the almost same solution like you. (Unfortunately I did not find this issue when taking a look if anybody run into it already.)

As you have done the work already I will try to do my first review, hopefully it is constructive:

1. +    if ( !$multiple ) {
   +      $value = array($value);
   +    }
   

   You can avoid that check completely if you do a typecast in the foreach below (see below).
   Otherwise if you want to keep that code, codestyle nitpick: between brackets there should be no spaces. if (!$multiple)




2. +    foreach( $value as $aValue ) {
   

   1. As mentioned above you can do the typecast here:

   foreach ((array) $value as $aValue)

   2. Codestyle nitpick: No spaces between brackets but after "foreach"




3. +      foreach ($result as $key => $info ) {
   

   Codestyle nitpick: no space before closing bracket
   Maybe also newline above?

Thoughts about your solution (which is the same than I came up with):
The only thing which bothers me a bit is that the performance of the 'IN' solution might be better on huge arrays. The above solution causes some overhead as we do a DB query for each item (dunno if this really is a problem, but it might be on huge datasets). I checked EntityQuery if there is another way to fetch multiple entities but I found none - maybe someone with more D8 knowledge has some hints.

One solution might be to add support for 'ORDER BY FIELD' to EntityQuery:
http://stackoverflow.com/questions/2609986/mysql-where-in-ordering

But maybe there is already a solution baked into D8, would be great if anybody with more insight could chip in.

One solution might be to add support for 'ORDER BY FIELD' to EntityQuery

This seems unlikely to be supported, since FIELD() is MySQL-specific.

One possibility to save queries is to implement static caching for each queried result, which at least prevents the same query from being executed over and over. Ultimately though, I'm less concerned about the performance aspects, since migrations generally don't happen in production environments.

+++ b/src/Plugin/migrate/process/EntityLookup.php
@@ -213,14 +213,29 @@ class EntityLookup extends ProcessPluginBase implements ContainerFactoryPluginIn
+      $result = $query->execute();
+
+      if (empty($result)) {
+       continue;
+      }
+      foreach ($result as $key => $info ) {
+        $results[$key] = $info;
+      }

$query->execute() always returns an array here, and since we're just concatenating key/value pairs together, we can use array arithmetic:
$results += $query->execute();

New patch attached for that and the style fixes in #3.

Great fix. Thanks for the contributions.

Can we add a quick test on this so we don't ever get a regression? This seems like something we'd accidentally loose at some point if we don't test it.

Updated patch migrate_plus-2833260-5.patch to work with 8.x-4.1

Reverting status.

Updating the patch to work after changes to version 4.2.0

Resetting the status again; the status "Patch (to be ported)" is meant for work that has already been merged into one branch, and now needs the patch backported to other versions/branches. This issue is still technically in "Needs work", since it still requires written tests.

I've ported the patch to work with Migrate Plus 5.1 (well, 5.x as of writing).

Worked for me.

What kind of test coverage do we need / expect to get this moved forward?

When this plugin was made, it was supposed to make it easy to import simple values into simple destinations. If we have complicated sort orders that make the use of this module hard, perhaps re-thinking how to import the data is needed. Split migration into 2 parts. One for the complicated terms structure. Then import that into nodes via migration_lookup.

Patch #11 works for me as well. I'll use the patch until this issue has been solved.

Maybe we could do a similar trick as in EntityStorageBase::loadMultiple():

// Ensure that the returned array is ordered the same as the original
// $ids array if this was passed in and remove any invalid IDs.
if ($flipped_ids) {
  // Remove any invalid IDs from the array and preserve the order passed in.
  $flipped_ids = array_intersect_key($flipped_ids, $entities);
  $entities = array_replace($flipped_ids, $entities);
}

Anyway, let's take a look into it and obviously add some test coverage.

Rerolled #11 for 6.0.x.