Problem/Motivation

Running a migration with the Migrate module (/upgrade) resulted in many failures. These typically resulted in partial migrations of the data and then it would get an error, stop what it was doing and move onto the next step. We tracked the problem down to the Flatten plugin being sent empty strings or NULL values. Upon adding a check to the Flatten plugin we were able to complete the migration successfully. I could see adding to this to include checks for other types of data being sent, rather than just empty(), but since it is expecting an array, anything else is already bad data.

Index: /core/modules/migrate/src/Plugin/migrate/process/Flatten.php
===================================================================
--- /core/modules/migrate/src/Plugin/migrate/process/Flatten.php (revision)
+++ /core/modules/migrate/src/Plugin/migrate/process/Flatten.php (working copy)
@@ -49,6 +49,7 @@
* For example, [[1, 2, [3, 4]]] becomes [1, 2, 3, 4].
*/
public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
+ if (empty($value)) {return array();}
return iterator_to_array(new \RecursiveIteratorIterator(new \RecursiveArrayIterator($value)), FALSE);
}

Proposed resolution

Validate the input value before using it. Throw a MigrateException if it is not an array or object.

Remaining tasks

  • Reroll (Novice)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ASA DBank created an issue. See original summary.

ASA DBank’s picture

Title: Migrate's Flatten plugin cannot handle NULL » Migrate's Flatten plugin being passed empty strings causing migration failure
Issue summary: View changes

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

Status: Active » Needs review
FileSize
3.47 KB

Flatten really should validate the input value and throw a MigrateException when it is invalid. Here is a start. The error message needs work.

quietone’s picture

Version: 8.9.x-dev » 9.1.x-dev

And that should have been 9.1.x

quietone’s picture

Title: Migrate's Flatten plugin being passed empty strings causing migration failure » Add validation to Flatten process plugin
Issue summary: View changes
FileSize
2.68 KB
3.75 KB

Updated the error message using #3148959: Improve migrate messages from the extract plugin as a guide and then updated the test.

And change the title to what we are doing.

mikelutz’s picture

Status: Needs review » Reviewed & tested by the community

\RecursiveArrayIterator::__construct() throws an \InvalidArgumentException if the input value is not an array or an object. This change detects that and preemptively throws a MigrateException instead. This is good because the Migration system can gracefully handle migrate exceptions, but not other types of exceptions, which is why we aim to have all migration plugins internally handle or avoid general Exceptions, and only throw the various migrate exceptions that the executable can handle.

benjifisher’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll, +Novice

The patch needs a reroll, which is a Novice task, so I am adding the appropriate issue tags.

abhisekmazumdar’s picture

Status: Needs work » Needs review
FileSize
3.74 KB

Hey I have re-rolled the patch. Kindly review.
Thank you

mikelutz’s picture

Status: Needs review » Needs work
Issue tags: -migrate, -Needs reroll

Looks like the patch needed a reroll due to #3160031: Fix 18 spelling errors for migrate specific terms, which fixed a spelling error in the test class. I did a diff between #11and #8 and got:

24c24
< index 899644f5ae..03f5e0f28b 100644
---
> index 496cf92216..1e23d35e25 100644
68,69c68,69
< +    [1, 2, [3, 4, [5]], [], [7, 8]],
< +    [1, 2, 3, 4, 5, 7, 8],
---
> +        [1, 2, [3, 4, [5]], [], [7, 8]],
> +        [1, 2, 3, 4, 5, 7, 8],
73c73
< +      ['test', '1.2', 'NULL'],
---
> +        ['test', '1.2', 'NULL'],
94c94
< -    $flattened = $plugin->transform([1, 2, [3, 4, [5]], [], [7, 8]], $this->migrateExecutable, $this->row, 'destination_property');
---
> -    $flattened = $plugin->transform([1, 2, [3, 4, [5]], [], [7, 8]], $this->migrateExecutable, $this->row, 'destinationproperty');

The last line, $flattened = .... is what needed to be updated. The remaining changes break the indent. Specifically:

+++ b/core/modules/migrate/tests/src/Unit/process/FlattenTest.php
@@ -11,13 +12,89 @@
+    return [
+      'array' => [
+    [1, 2, [3, 4, [5]], [], [7, 8]],
+    [1, 2, 3, 4, 5, 7, 8],
+      ],
+      'object' => [
+        $object,
+      ['test', '1.2', 'NULL'],
+      ],
+    ];

The indentation of this array is incorrect. It should match the patch in #8

sarvjeetsingh’s picture

Assigned: Unassigned » sarvjeetsingh
sarvjeetsingh’s picture

Assigned: sarvjeetsingh » Unassigned
ayushmishra206’s picture

Assigned: Unassigned » ayushmishra206
ayushmishra206’s picture

Status: Needs work » Needs review
FileSize
683 bytes
3.75 KB

Made the changes suggested by @mikelutz in #12. Please review.

ayushmishra206’s picture

Assigned: ayushmishra206 » Unassigned
mikelutz’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate/tests/src/Unit/process/FlattenTest.php
@@ -11,13 +12,89 @@
+    $this->plugin->transform($value, $this->migrateExecutable, $this->row, 'destinationproperty');
...
-    $flattened = $plugin->transform([1, 2, [3, 4, [5]], [], [7, 8]], $this->migrateExecutable, $this->row, 'destination_property');

This reverts the spelling change of 'destinationproperty' to 'destination_property' that was fixed the the other issue I linked. It should be destination_property, not destinationproperty.

ayushmishra206’s picture

Status: Needs work » Needs review
FileSize
806 bytes
3.75 KB

Please review

mikelutz’s picture

Status: Needs review » Reviewed & tested by the community

Perfect, Thanks! On to RTBC!

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Bug Smash Initiative
  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Flatten.php
    @@ -49,6 +50,9 @@ class Flatten extends ProcessPluginBase {
    +      throw new MigrateException(sprintf('Input should be an array or an object, instead it was %s', print_r($value, TRUE)));
    

    should we be using https://www.php.net/manual/en/function.gettype.php instead? The value could get big here - or failing that - var_export instead of print_r so we get the type as well?

  2. +++ b/core/modules/migrate/tests/src/Unit/process/FlattenTest.php
    @@ -11,13 +12,89 @@
    +        '1',
    ...
    +        '1.2',
    ...
    +        'TRUE',
    

    should this be 1.2 and not '1.2', 1 instead of '1' and TRUE instead of 'TRUE' so it is actually a float/integer/boolean?

abhisekmazumdar’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
3.75 KB

I have made the changes. Kindly check.

Status: Needs review » Needs work

The last submitted patch, 22: 2960170-22.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
2.39 KB
3.79 KB

@abhisekmazumdar, thanks for progressing this. I see that the test Drupal\Tests\migrate\Unit\process\FlattenTest is failing. If your not already doing so it good practice to run the test locally before uploading and relying on the testbot. And the tests for the migrate process plugins require very little setup. There is lots of information for Running PHPUnit tests in the handbook.

21.1 Instead of outputting the value, lets just output the data type. The message will be of the type, 'Input should be an array or an object, instead it was of type 'boolean''. I also made changes so the spellcheck passes.

mikelutz’s picture

Status: Needs review » Reviewed & tested by the community

Much better, feedback addressed!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed aa37626 and pushed to 9.1.x. Thanks!

  • alexpott committed aa37626 on 9.1.x
    Issue #2960170 by quietone, ayushmishra206, abhisekmazumdar, mikelutz,...
Wim Leers’s picture

Nice improvement! 👏🙏

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.