Problem/Motivation
While working on #2827644: Fix path alias migration of translated nodes [D6], I tried using this process pipeline:
translation_source:
-
plugin: explode
source: src
delimiter: /
-
plugin: extract
index:
- 1
-
plugin: migration
migration: d6_node_translation
The source are node paths (node/1) and I need to find the new nid of the translation source.
The problem is that once a plugin return a multiple value, all following plugins have to be able to handle multiple values.
In this example, the explode plugin returns multiple values, then the extract plugin is able to handle them and return a single value. But then, I get an error that the migration plugin is expecting an array.
I think th problem is located in core/modules/migrate/src/MigrateExecutable.php line 401:
$multiple = $multiple || $plugin->multiple();
$multiple will always be TRUE once a previous plugin has returned TRUE.
Proposed resolution
I think it should just be:
$multiple = $plugin->multiple();
So $multiple would be TRUE only if the previous plugin returns multiple values.
Remaining tasks
See if it breaks anything. Discuss. Write a test. Review.
User interface changes
None.
API changes
None.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | interdiff-12-15.txt | 2.17 KB | maxocub |
| #15 | 2827656-15.patch | 4.58 KB | maxocub |
| #12 | 2827656-12.patch | 3.9 KB | maxocub |
| #12 | 2827656-12-test-only.patch | 3.34 KB | maxocub |
| #6 | drupal-core-process-plugin-multiple-2827656-4-8.3.x.patch | 575 bytes | aditya.n |
Comments
Comment #2
maxocub commentedLet's see how much it breaks.
Comment #3
maxocub commentedWrong status...
Comment #5
aditya.n commented@maxocub, You might have changed the code in wrong position hence the fails, submitting patch with changes.
Comment #6
aditya.n commentedFor test bot. :)
Comment #7
aditya.n commentedComment #8
aditya.n commentedComment #9
maxocub commentedThanks @aditya.n, you're right, I don't know why I moved the line.
I'm going to see if I can write a test to underline the bug and the fix.
Comment #10
maxocub commentedThis will need a Kernel test, so it is postponed on #2826638: Create a KTB for migrate's process plugins
Comment #11
quietone commented@maxocub, thanks for taking this on. Adding two related issues where I encountered this problem. Although one is marked fixed, the test in that issue has two extracts in the pipeline and not and explode and extract
Comment #12
maxocub commentedFollowing @benjy's comment #6 in #2826638: Create a KTB for migrate's process plugins I decided to write a kernel test here without a base class.
I don't think it's possible to write a unit test for a local variable of preprocessRow().
Comment #14
mikeryanComment #15
maxocub commentedI just added some comments explaining the test.
One thing to note about this "multiple" thing is that many process plugins sometimes return multiple values and sometimes they don't (migration, callback, extract, etc.). I noticed that the "get" plugin test the value it returns and then it's multiple() method returns TRUE or FALSE accordingly. Maybe we should do the same for the other process plugins? But then other plugins might not be able to handle multiple (with the handle_multiple annotation).
Comment #16
heddnCan we test an explode that chains into another explode? Would that make sense? So we test passing a multi-valued return from explode into something that requires a string right away. I think to do that, we'd have to use iterator, no? So maybe the test isn't necessary.
Comment #17
mikeryanI think the testing is fine for the narrow fix being made here.
Comment #19
catchCommitted/pushed to 8.3.x and cherry-picked to 8.2.x, thanks!
Comment #20
Anonymous (not verified) commentedAwesome. Just ran into this bug last week. Thanks all!
Comment #23
rodrigoaguileraThis issue appears on google when searching on how to handle multiple values.
Since this fix once a pipe is multiple all the following plugins are supposed to handle that.
I wanted to use the entity_generate process_plugin with a string that have been exploded.
My intuition told me to use the "iterator" process plugin but that requires an array of arrays(explode creates a simple array).
What I ended up doing was "resetting" the "multiple" state of the pipe by creating a new pipe starting with the result of the old one like is described in the get plugin docs.
https://www.drupal.org/docs/8/api/migrate-api/migrate-process/process-pl...
Comment #24
maxocub commented