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.

Comments

maxocub created an issue. See original summary.

maxocub’s picture

Status: Active » Needs work
StatusFileSize
new616 bytes

Let's see how much it breaks.

maxocub’s picture

Status: Needs work » Needs review

Wrong status...

Status: Needs review » Needs work

The last submitted patch, 2: 2827656-2.patch, failed testing.

aditya.n’s picture

@maxocub, You might have changed the code in wrong position hence the fails, submitting patch with changes.

aditya.n’s picture

For test bot. :)

aditya.n’s picture

aditya.n’s picture

Status: Needs work » Needs review
maxocub’s picture

Assigned: Unassigned » maxocub
Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks @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.

maxocub’s picture

Title: Once a process plugin returns multiple values, all following plugins are expected to handle multiple values » [PP-1] Once a process plugin returns multiple values, all following plugins are expected to handle multiple values
Status: Needs work » Postponed

This will need a Kernel test, so it is postponed on #2826638: Create a KTB for migrate's process plugins

quietone’s picture

@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

maxocub’s picture

Title: [PP-1] Once a process plugin returns multiple values, all following plugins are expected to handle multiple values » Once a process plugin returns multiple values, all following plugins are expected to handle multiple values
Status: Postponed » Needs review
Issue tags: -Needs tests
StatusFileSize
new3.34 KB
new3.9 KB

Following @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().

The last submitted patch, 12: 2827656-12-test-only.patch, failed testing.

mikeryan’s picture

Assigned: maxocub » mikeryan
maxocub’s picture

StatusFileSize
new4.58 KB
new2.17 KB

I 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).

heddn’s picture

Can 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.

mikeryan’s picture

Assigned: mikeryan » Unassigned
Status: Needs review » Reviewed & tested by the community

I think the testing is fine for the narrow fix being made here.

  • catch committed 4ac6c3b on 8.2.x
    Issue #2827656 by maxocub, aditya.n: Once a process plugin returns...
catch’s picture

Version: 8.3.x-dev » 8.2.x-dev
Status: Reviewed & tested by the community » Fixed

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

Anonymous’s picture

Awesome. Just ran into this bug last week. Thanks all!

  • catch committed 3b62ac7 on 8.3.x
    Issue #2827656 by maxocub, aditya.n: Once a process plugin returns...

Status: Fixed » Closed (fixed)

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

rodrigoaguilera’s picture

This 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...

maxocub’s picture