Scalar to array migration returns NULL

I'm not sure this fix is going to be valid. This returns reset($destination_ids) if it's 2 or less -- but some IDs might very well need two values to be considered complete. IMHO, the fact that it passes all the tests proves that our coverage for the Migration plugin is wholly insufficient.

I would suggest the following, at the end of Migration::transform():

if ($destination_ids) {
      if ($scalar) {
        if (count($destination_ids) == 1) {
          return reset($destination_ids);
        }
        else {
          return $destination_ids;
        }
      }
      else {
        return $destination_ids;
      }
    }

I have a distinctly different issue from phenaproxima, with the same root cause. The folks in #drupal-migrate suggested I directly modify this issue.

I'm attempting to use the migrate module to import Paragraphs. After a patch to Entity Reference Revisions, the migration map for a destination plugin of "entity_reference_revisions:paragraph" correctly creates a map with two destid's. Migration::transform *almost* correctly translates the single migration id into target_id and target_revision_id, but it fails horribly in the last 10 lines.

If the migration's input value is a scalar, but the expected return value is an array, Migration:transform currently returns NULL.

$scalar's value is set in this block of code:

    $scalar = FALSE;
    if (!is_array($value)) {
      $scalar = TRUE;
      $value = array($value);
    }

And the return value is determined by this block of code:

    if ($destination_ids) {
      if ($scalar) {
        if (count($destination_ids) == 1) {
          return reset($destination_ids);
        }
      }
      else {
        return $destination_ids;
      }
    }

The problem is, if $value is initially a scalar, but $destinitation_ids is expected to be an array of size > 1, the code goes into the scalar loop, and returns nothing because count($destintation_ids) isn't 1.

Unlike phenaproxmia's solution, I really do need both of those values returned. It's not possible to reference an entity_reference_revision without both keys. I do not want $destination_ids to be reset.

I'm actually submitting two patches that fix this problem. A "quick fix" (#1), that ensures that Migration::transform returns a value in this scalar->array migration case, and a more brutal "Why do we need scalars anyway?" approach (#2) that just discards the $scalar variable and returns a scalar if there's only one return value, but returns an array if there's an array.

I suspect #1 will be too much of a hack to be accepted, and that I'm misunderstanding some fundamental reason why scalars should be preserved, and that #2 goes too far. Hopefully both will provoke discussion and result in a better patch.

Both patches work with my test case.

Setting to "Needs review" - let's let the testbot verify these don't blatantly break things (stipulating as phenaproxima points out that the test coverage is probably inadequate to fully prove nothing will break).

+++ b/core/modules/migrate/src/Plugin/migrate/process/Migration.php
@@ -70,11 +70,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+    $value = array($value);

If $value is already an array, this would seem to wrap an extra array around it. How about (array)$value?

Gah, you're right @mikeryan. Let me reroll a patch to replace #2...

Solution: add an option to switch off the scalar magic completely. One size doesn't fit all.

Needs work because needs tests.

This was briefly discussed in the weekly migrate call last night. There wasn't enough time to reach a consensus on how to address a solution here. However, after thinking about the conversation, I would propose that we work on tests in this area first, then come back to fixing this.

The two strongest motivators for this patch are migrating i18n sites from CSV and other non-Drupal sources and migrating multiple paragraphs into a node, etc. Both of those cases have temporary work-around using the above patches, so let's take a little time and improve test coverage first, then fix this right. If writing tests seems to drag on a long time, then we can revisit trying a quicker fix.

Thoughts on this proposal? I looked for an existing issue for 'improve/modernize of the migration process plugin's tests". I couldn't find one. If one doesn't exist, we'll need to create it.

Postponing this temporarily while we work on tests. Related issues added for the blocking tasks before we can return to this.

This is also needed for #2827644: Fix path alias migration of translated nodes [D6].

Following @benjy's comment #6 in #2826638: Create a KTB for migrate's process plugins I decided to write a unit test here because this bug needs to be fixed in order to move forward with #2827644: Fix path alias migration of translated nodes [D6].

Great job on writing the test and ignoring the dependencies here.

One point of feedback I'd like to see in the test is 3 scenarios. Storing the values into a field in my mind is important because there's no way to know if the format of the returned value from the migration process plugin is valid enough for storage in an ER field without going all the way.

1. Single value from source dumping into a single value field. Ex. Single node reference get migrated into a ER field that has cardinality of 1.
2. Single value from source dumping into a multi-value field. Ex. Single node reference get migrated into a ER field that has cardinality of unlimited.
3. Multiple value from source dumping into a multi-value field. Ex. node references get migrated into a ER field that has cardinality of unlimited.

Still leaving at NR for other reviews.

@heddn: I don't have any idea how to test the storing of values into fields with this unit test. I think we'll need the kernel test for that.

I added some comments in the test to underline what scenarios are tested here.

In short, it's testing that the migration process plugin can handle single (scalar) and multiple (array) values for input and output.

The migration_map_* tables can have one or more source IDs (sourceid1, sourceid2, etc.) and one or more destination IDs (destid1, destid2, et.). The migration process plugin should be able to handle the four possibilities:

1. a single source ID and a single destination ID
2. a single source ID and multiple destination IDs
3. multiple source IDs and a single destination ID
4. multiple source IDs and multiple destination IDs

Thanks for that extra explanation. I've also tested the patch (it is required for err) over in #2809793: EntityReference migrate destination. And over there I'm doing testing of the various field combinations I asked for in in #21. I think unit testing is enough here, so marking this RTBC.

Random fail, back to RTBC.

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!