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.

CommentFileSizeAuthor
#8 3539186-8.patch3.16 KBcasey
Command icon 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

benabaird created an issue. See original summary.

benabaird’s picture

Status: Active » Needs review

All test pass, marking as needs review!

heddn’s picture

Status: Needs review » Needs work

If we are going to work in this space, can we also adjust SnippetTest so it no longer try/catches MigrateSkipProcessException?

benabaird’s picture

Status: Needs work » Needs review

Thanks, updated that test, which looks to indirectly test the SkipOnValue change as well.

heddn’s picture

Status: Needs review » Needs work

I think we need to also bump the minimum drupal core version to 10.3 at this point too.

benabaird’s picture

Oh 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".

casey’s picture

Priority: Normal » Critical
StatusFileSize
new3.16 KB

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

casey’s picture

achap’s picture

Status: Needs work » Reviewed & tested by the community

I 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!

  • heddn committed 2ec1ccf7 on 6.0.x authored by benabaird
    fix: #3539186 \Drupal\migrate\ProcessPluginBase::stopPipeline calls...
heddn’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for the work on this everyone.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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