Problem/Motivation

The useful-but-intimidating migration process plugin gets overzealous when it completely fails to look up a destination entity -- it throws MigrateSkipRowException. This is a) outside the bounds of what it should be allowed to do, and b) wholly unnecessary, because the skip_on_empty plugin can be used to implement that behavior explicitly on a case-by-case basis.

Proposed Resolution

The migration plugin should never throw MigrateSkipRowException. If it fails the lookup, it's good enough for it to implicitly return NULL.

Remaining Tasks

The usual: patch, review, commit.

API Changes

When the migration plugin fails a lookup, the appropriate course of action will have to be declared explicitly by the migration. (Skip row, skip processing, do further processing, ...?)

CommentFileSizeAuthor
#7 2560671-7.patch5.91 KBphenaproxima
#4 interdiff-2560671-2-4.txt4.44 KBphenaproxima
#4 2560671-4.patch5.57 KBphenaproxima
#2 2560671-2.patch512 bytesphenaproxima

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2560671-2.patch512 bytes

Initial patch.

Status: Needs review » Needs work

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

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
StatusFileSize
new 2560671-4.patch5.57 KB
new interdiff-2560671-2-4.txt4.44 KB

Fixing the test failures and deleting the now-defunct unit test of the Migration plugin.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Migration.php
@@ -145,7 +145,6 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
-    throw new MigrateSkipRowException();

    If we're removing the skip exception, we should remove the associated use from the top of the file. 

  2. +++ /dev/null
@@ -1,85 +0,0 @@
-<?php
-
-/**
- * @file
- * Contains \Drupal\Tests\migrate\Unit\process\MigrationTest.
- */
-

    Its sad this was the only thing this was testing... I asked phena to make a follow up to add it back.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Related issues: +#2561049: Add proper unit tests for the Migration process plugin

Follow-up issue created.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
StatusFileSize
new 2560671-7.patch5.91 KB

Fixed #5.1.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Reviewed & tested by the community

Code looks good to me, RTBC assuming tests pass.

  • webchick committed 4cc0d6d on 8.0.x 
    Issue #2560671 by phenaproxima, neclimdul: The Migration process plugin...
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

I asked Adam why we want to do this, and he laid out the following scenario:

phenaproxima
Let's say we're migrating users. As part of that, we handle user pictures. We try to look up the previously-migrated picture by fid, using the migration plugin. So far, reasonable
10:51 But if the file cannot be found -- Migration throws the exception, and BAM! The entire user account is skipped. Because its picture could not be found.
Then imagine, later on, we run a node migration that expects that skipped user to be its author
10:51 That node will be pointing to a user which doesn't exist
10:52 And this is how things end up going KABOOM
10:52 The solution is for the migration itself to say, hey -- user pictures aren't important. We can just skip this field.

Makes sense.

I also asked about the test coverage removal, but that is because the only thing the tests were proving is that the behaviour we're removing works, so... yeah. :) We still do need more robust testing for this behaviour in general—for example, the exact scenario laid out above—but since we don't currently have such test coverage it doesn't really make sense to hold this significant UX improvement up on that. Let's make sure there's a follow-up for it though, if one doesn't already exist.

Committed and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

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