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)
Comment | File | Size | Author |
---|---|---|---|
#24 | 2960170-24.patch | 3.79 KB | quietone |
#24 | 2960170-22-24.txt | 2.39 KB | quietone |
#22 | 2960170-22.patch | 3.75 KB | abhisekmazumdar |
#22 | interdiff.2960170.19-22.txt | 1.55 KB | abhisekmazumdar |
#19 | 2960170-19.patch | 3.75 KB | ayushmishra206 |
Comments
Comment #2
ASA DBank CreditAttribution: ASA DBank commentedComment #6
quietone CreditAttribution: quietone as a volunteer commentedFlatten really should validate the input value and throw a MigrateException when it is invalid. Here is a start. The error message needs work.
Comment #7
quietone CreditAttribution: quietone as a volunteer commentedAnd that should have been 9.1.x
Comment #8
quietone CreditAttribution: quietone as a volunteer commentedUpdated 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.
Comment #9
mikelutz\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.
Comment #10
benjifisherThe patch needs a reroll, which is a Novice task, so I am adding the appropriate issue tags.
Comment #11
abhisekmazumdarHey I have re-rolled the patch. Kindly review.
Thank you
Comment #12
mikelutzLooks 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:
The last line, $flattened = .... is what needed to be updated. The remaining changes break the indent. Specifically:
The indentation of this array is incorrect. It should match the patch in #8
Comment #13
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedComment #14
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedComment #15
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedComment #16
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedMade the changes suggested by @mikelutz in #12. Please review.
Comment #17
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedComment #18
mikelutzThis 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.
Comment #19
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedPlease review
Comment #20
mikelutzPerfect, Thanks! On to RTBC!
Comment #21
larowlanshould 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?
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?
Comment #22
abhisekmazumdarI have made the changes. Kindly check.
Comment #24
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #25
mikelutzMuch better, feedback addressed!
Comment #26
alexpottCommitted aa37626 and pushed to 9.1.x. Thanks!
Comment #28
Wim LeersNice improvement! 👏🙏