Get sourceid values sorted from Row

What are the efficiencies gained by moving the logic? Is it performance? Plus needs tests.

For context, I see that the use case is #2698067: Make idlist command support multiple sourceids. Not a matter of efficiency, but generality - it's better to guarantee the proper ordering of multi-value source keys back at the source, than only when hashing them.

Testing if needs re-roll and had some offset. Updating patch.

patching file core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
Hunk #1 succeeded at 183 (offset -5 lines).
patching file core/modules/migrate/src/Row.php
Hunk #1 succeeded at 109 (offset -5 lines).

So far so good, now for testing.

Initial test case added.

Just a little documentation needed, I think.

1. +++ b/core/modules/migrate/src/Row.php
   @@ -109,7 +109,7 @@ public function __construct(array $values = [], array $source_ids = [], $is_stub
       *   An array containing the values of the source identifiers.
   

   Let's specify that the result is ordered according to the order in $this->sourceIds.

2. +++ b/core/modules/migrate/src/Row.php
   @@ -109,7 +109,7 @@ public function __construct(array $values = [], array $source_ids = [], $is_stub
   +    return array_merge(array_flip(array_keys($this->sourceIds)), array_intersect_key($this->source, $this->sourceIds));
   

   Four array functions in one line makes this a bit... dense - it takes running through what's happening here from the inner calls outwards to understand how it works. A comment would be helpful, explaining that array_flip(array_keys()) presents a keyed array in the properly order, and merging with array_intersect_key fills in the values.

Initial documentation added. As I'm not native English speaking person, please help on proper wording. Thanks.

I think that's fine, thanks!

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -183,20 +183,7 @@ public static function create(ContainerInterface $container, array $configuratio
   public function getSourceIDsHash(array $source_id_values) {

Hmmm ugh the docs for this say

It is public only for testing purposes.

We should have used reflection for that. Because this change could break existing migrate plugins. I guess we should try and work out how worth it this change actually is.

Hrrm, yes, "public only for testing purposes" is an abomination not our best work - but, fixing that would be a matter for a separate issue.

So, I was going to say I don't see how this would break any existing migrate plugins. It's Row::getSourceIdValues() whose behavior is being expressly changed, and that change is from an unspecified ordering of the returned array to one that is ordered by the source ID definition, which shouldn't break anything unless it were depending on whatever order it happened to be getting (and note that A. Most often there's only one field, and B. Most of the time even with multiple fields they'll usually be in the right order anyway).

However, turning it over in my mind, there is actually a subtle BC break here - the $source_id_values argument to getSourceIDsHash() previously could be in any order because it sorted them, but now it will require the incoming array to already be sorted. As it happens (as near as I can see), all the core usages either pass through Row::getSourceIdValues() or are inherently sorted, but we can't guarantee that will always be the case. At this point, I'm questioning as @alexpott did whether this is worthwhile - it seems safer to just leave well enough alone and guarantee the source fields are sorted (if somehow the order of fields for a given row differed between runs, that would lead to some *very* difficult to diagnose problems, I think). If we are to go forward, at the very least getSourceIDsHash() needs to document that the input array needs to be properly sorted.

@edysmp/@mitrpaka - What do you think? Do you still want to pursue this?

Oh, but back to the original use case - we could still make Row::getSourceIdValues() sort its return value properly, but leave the sorting in getSourceIDsHash() alone.

Per my last comment, this could still be done if we remove the change to getSourceIDsHash().

Launching tests to confirm, but this likely will need a reroll at this point...

Removed the change to getSourceIDsHash() and updated array syntax.

Looks good, thanks!

1. +++ b/core/modules/migrate/src/Row.php
   @@ -106,10 +106,14 @@ public function __construct(array $values = [], array $source_ids = [], $is_stub
   +    // First, return the array of keys properly ordered with
   +    // array_flip(array_keys()). Then, fill in values by merging with
   +    // array_intersect_key().
   

   This comment should just describe we want to occur rather than be a verbal description of the implementation. Ie. something like Return the source values in the same order as the source identifiers. Mind you we could consider omitting this given the @return just above.

2. +++ b/core/modules/migrate/src/Row.php
   @@ -106,10 +106,14 @@ public function __construct(array $values = [], array $source_ids = [], $is_stub
   +    return array_merge(array_flip(array_keys($this->sourceIds)), array_intersect_key($this->source, $this->sourceIds));
   

   Why not just array_merge($this->sourceIds, array_intersect_key($this->source, $this->sourceIds)); since the keys are already properly ordered and array_merge() will replace the values using the array_interest_key() return?

1. Removed the redundant comment (the @return gives enough information and is immediately above).
2. Simplified the return statement, as recommended by @alexpott and stepped through the code to check that it still made sense (it does).

Changes look good to me.

Re-tested - all fine, back to RTBC.

Does this mean we can remove...

    // When looking up the destination ID we require an array with both the
    // source key and value, e.g. ['nid' => 41]. In this case, $source_id_values
    // need to be ordered the same order as $this->sourceIdFields().
    // However, the Migration process plugin doesn't currently have a way to get
    // the source key so we presume the values have been passed through in the
    // correct order.
    if (!isset($source_id_values[0])) {
      $source_id_values_keyed = [];
      foreach ($this->sourceIdFields() as $field_name => $source_id) {
        $source_id_values_keyed[] = $source_id_values[$field_name];
      }
      $source_id_values = $source_id_values_keyed;
    }

And just do:

return hash('sha256', serialize(array_map('strval', array_values($source_id_values))));

The code that checks if (!isset($source_id_values[0])) { looks super brittle.

Re #26: Yes, we should be able to get rid of that. Do we have tests? I'm sure we do, otherwise things would start falling on their face.

Let's see what the testbot thinks of that change...

And back to RTBC.

Random testbot failure.

Come on, testbot! Back to RTBC.

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -183,20 +183,7 @@ public static function create(ContainerInterface $container, array $configuratio
   public function getSourceIDsHash(array $source_id_values) {
-    // When looking up the destination ID we require an array with both the
-    // source key and value, e.g. ['nid' => 41]. In this case, $source_id_values
-    // need to be ordered the same order as $this->sourceIdFields().
-    // However, the Migration process plugin doesn't currently have a way to get
-    // the source key so we presume the values have been passed through in the
-    // correct order.
-    if (!isset($source_id_values[0])) {
-      $source_id_values_keyed = [];
-      foreach ($this->sourceIdFields() as $field_name => $source_id) {
-        $source_id_values_keyed[] = $source_id_values[$field_name];
-      }
-      $source_id_values = $source_id_values_keyed;
-    }
-    return hash('sha256', serialize(array_map('strval', $source_id_values)));
+    return hash('sha256', serialize(array_map('strval', array_values($source_id_values))));
   }

Hm, so this is a public function but the change affects what it expects as its argument, so at least it would come with a docs change? What about others possibly invoking this method?

#36: Doc change now specifies what is expected.

Hm, as per #15, @mikeryan was leaning to keep the behavior as-is in my reading. It would be nice to get a confirmation form him or to keep the logic in line with his comment.

At this point, I'm questioning as @alexpott did whether this is worthwhile - it seems safer to just leave well enough alone and guarantee the source fields are sorted (if somehow the order of fields for a given row differed between runs, that would lead to some *very* difficult to diagnose problems, I think).

(emphasis mine)

Right, in #26 @alexpott asked if we could remove the sorting code from getSourceIDsHash(), and in response it was removed. However, as I pointed out in #15, although existing calls to getSourceIDsHash() will pass the source in sorted, if anyone in contrib/custom modules is calling it without sorting the keys, they'll get a very subtle break in that the hashes won't necessarily match up. I think we should leave getSourceIDsHash() as-is - it needs to continue guaranteeing that the IDs are sorted before hashing.

#37 has the documentation change but is missing the rest of the patch. Does it just need to be combined with #28?

This issue took a couple of turns so tagging for issue summary update.

Merging patch of #28 and #37

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -183,20 +186,7 @@ public static function create(ContainerInterface $container, array $configuratio
-    // When looking up the destination ID we require an array with both the
-    // source key and value, e.g. ['nid' => 41]. In this case, $source_id_values
-    // need to be ordered the same order as $this->sourceIdFields().
-    // However, the Migration process plugin doesn't currently have a way to get
-    // the source key so we presume the values have been passed through in the
-    // correct order.
-    if (!isset($source_id_values[0])) {
-      $source_id_values_keyed = [];
-      foreach ($this->sourceIdFields() as $field_name => $source_id) {
-        $source_id_values_keyed[] = $source_id_values[$field_name];
-      }
-      $source_id_values = $source_id_values_keyed;
-    }
-    return hash('sha256', serialize(array_map('strval', $source_id_values)));
+    return hash('sha256', serialize(array_map('strval', array_values($source_id_values))));

Removing this change mentioned by Mike.

Thanks for moving this forward. Just some small updates left AFAIS.

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -174,7 +174,10 @@ public static function create(ContainerInterface $container, array $configuratio
-   * It is public only for testing purposes.
+   * It is public only for testing purposes. The source id values should be
+   * ordered in the same order as the data returned by a source plugin's
+   * getIds() method.
+   * @internal

This added part of the comment is not true anymore, so it should be removed. IMHO a newline before @internal should also be added.

Thanks for the update. Also this now only does what the issue summary said, so no need to update the issue summary. The change is backwards compatible given that no ordering was ensured on getSourceIdValues() before. Committed!