Problem/Motivation
The change record Migrate process plugin can now stop the process pipeline after they run examples show returning NULL after \Drupal\migrate\ProcessPluginBase::stopPipeline is called, but the implementations in do not return NULL after the stop call. This leads to the values passed to the file_blob, gate, and skip_on_value plugins being returned by those plugins when the pipeline is stopped, which MigrateExecutable then saves.
Steps to reproduce
Create a migration with the following pipeline:
field_taxonomy:
- plugin: skip_on_value
source: taxonomy
value:
- 'Skip One'
- 'Skip Two'
- plugin: entity_generate
entity_type: taxonomy_term
value_key: name
bundle_key: vid
bundle: vocabulary
ignore_case: true
Note that even if the term matches the values to skip, the value of the taxonomy source field will attempt to be inserted into field_taxonomy causing a database exception.
Proposed resolution
Add return NULL; after calls to \Drupal\migrate\ProcessPluginBase::stopPipeline.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 3539186-8.patch | 3.16 KB | casey |
Issue fork migrate_plus-3539186
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #3
benabaird commentedAll test pass, marking as needs review!
Comment #4
heddnIf we are going to work in this space, can we also adjust SnippetTest so it no longer try/catches MigrateSkipProcessException?
Comment #5
benabaird commentedThanks, updated that test, which looks to indirectly test the SkipOnValue change as well.
Comment #6
heddnI think we need to also bump the minimum drupal core version to 10.3 at this point too.
Comment #7
benabaird commentedOh probably, even if this was already implicitly done in Fix remaining code quality findings. If you want to do that in this issue I can bump it.
There's a few todos in code that should probably be cleaned up in a separate issue which I found after looking through the snippet plugin when implementing the test. Not sure if this is a complete list, I just searched the codebase for "10".
Comment #8
casey commentedSnapshot of the latest state of the MR for safe usage with composer patches.
I also updated the priority to critical, as this change alters the output/result of existing migrations.
Comment #9
casey commentedComment #10
achapI have created a new issue to deal with the version requirements so we can focus on this fix. I tested this in our own CI pipeline as it caught it and it is working fine for me after applying the patch from #8. Thanks!
Comment #12
heddnThanks for the work on this everyone.