Problem/Motivation

Follow up on initial error messages in #3197915: Add details to MigrateSkipRowException messages caused by stub creation

When migration_lookup fails to create a stub entity (due to MigrateSkipRowException being thrown during stub creation), the error message does not include the lookup value that caused the failure.

Steps to reproduce

Create the following test migrations using the embedded_data source plugin:

Migration 1: Page Migration (Source)

id: migrate_page
label: Test Migrate Page
source:
  plugin: embedded_data
  data_rows:
    -
      nid: 1
      title: 'Lorem'
      mid: 101
    -
      nid: 2
      title: 'Ipsum'
      mid: 102
  ids:
    nid:
      type: string
process:
  title: title
  field_media/target_id:
    plugin: migration_lookup
    migration: migrate_media
    source: mid
  status:
    plugin: default_value
    default_value: 1
destination:
  plugin: 'entity:node'
  default_bundle: page

A target migration that could initiate a skip row exception (ie. with Migrate Plus skip_on_value process plugin):

Migration 2: Media Migration (Complex Target)

id: migrate_media
label: Test Migrate Media (Complex Target)
source:
  plugin: embedded_data
  data_rows:
    -
      mid: 101
      title: 'Dolor'
      field: 'Image'
  ids:
    mid:
      type: string
process:
  # This process will fail if field is missing
  _skip_if_wrong_value:
    plugin: skip_on_value
    method: row
    source: field
    value: 'Image'
    not_equals: true
  title: title
  status:
    plugin: default_value
    default_value: 1
destination:
  plugin: 'entity:media'
  default_bundle: image

Test case execution

drush mim migrate_media
drush mim migrate_page

Test case result:

  • Row 1 (mid: 101): Lookup succeeds
  • Row 2 (mid: 102): Lookup fails, stub creation throws exception, error message lacks lookup value of (mid: 102), source node (nid: 2) is not migrated

The error message doesn't tell us which specific lookup value (mid: 102) caused the failure, making debugging difficult in large datasets. (See the error message in the "Proposed resolution" section.)

Also beyond the error message issue, there appears to be questionable behavior where stub creation failures cause the entire source row to be skipped. In the example above, the entire page "Ipsum" is lost because media ID 102 couldn't be found. This seems disproportionate and the content should arguably be preserved even if the media reference fails.

Proposed resolution

Modify the error message generation in MigrationLookup::transform() to include the original lookup value:

Current Error Message (Problem):

Migration lookup for destination 'field_media/target_id' attempted to create a stub using migration migrate_media, which resulted in a row skip

Suggested Error Message (With Lookup Value):

Migration lookup for value '102' and destination 'field_media/target_id' attempted to create a stub using migration migrate_media, which resulted in a row skip

Remaining tasks

Discuss, test and review the change

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#7 3532092-nr-bot.txt1.27 KBneeds-review-queue-bot

Issue fork drupal-3532092

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:

About issue forks

Comments

baikho created an issue. See original summary.

baikho opened merge request !12462

baikho closed merge request !12462

baikho’s picture

baikho commented
Version: 11.2.x-dev » 11.x-dev

baikho opened merge request !12463

baikho’s picture

baikho commented
Status: Active » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3532092-nr-bot.txt1.27 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

baikho’s picture

baikho commented
Issue summary: View changes
Status: Needs work » Needs review
Related issues: +#3197915: Add details to MigrateSkipRowException messages caused by stub creation
benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Status: Needs review » Needs work

@baikho:

Thanks for working on this issue!

I would like to have more complete steps to reproduce (STR).

Technically "Attempt to do an entity lookup which has no entity to resolve to" is not enough. I am pretty sure that the migration lookup (not an entity lookup) has to fail, and then there has to be an exception when trying to create the stub.

The bigger problem is that I want to know how to test and see what difference the change makes in the context of an actual migration. The update to the unit test only confirms that the re-thrown exception has the expected changes. It does not show me what actually gets logged in the message table.

Good STR would include at least one migration YAML file, and probably two or three. The migration(s) could use the embedded_data source plugin. The migration(s) that trigger(s) the new code should contain rows that cover all possibilities:

  • The lookup succeeds.
  • The lookup fails, but the stub gets creates.
  • The lookup fails, and the stub throws an exception.
baikho’s picture

baikho commented
Issue summary: View changes
baikho’s picture

baikho commented

#9 Thanks @benjifisher, I have updated the IS accordingly.

baikho’s picture

baikho commented
Issue summary: View changes
Status: Needs work » Needs review
benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Issue summary: View changes

@baikho:

Thanks for the additions to the issue summary. That should make testing a lot easier. It may be a week or two before I have time to test, but maybe someone else will pick it up before I do.

Also beyond the error message issue, ...

That looks like it could be a separate issue. Maybe the SkipRow exception should be re-thrown as a SkipProcess exception. Or maybe we can stop using exceptions after #3247718: Allow process plugins to flag a row to be skipped is fixed.

baikho’s picture

baikho commented

#13

Maybe the SkipRow exception should be re-thrown as a SkipProcess exception. Or maybe we can stop using exceptions after #3247718: Allow process plugins to flag a row to be skipped: Allow process plugins to flag a row to be skipped is fixed.

Yes, or something along these lines seems to work too:

+ $migrate_executable->saveMessage($new_message, MigrationInterface::MESSAGE_INFORMATIONAL);
+ $this->stopPipeline();
+ return NULL;
- throw new MigrateSkipRowException($new_message, 0);
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Based on the output examples in the IS and from what I can see in the code this seems like a good minor update to the message.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

Tagging needs follow-up for a new issue to explore #13/14.

Committed/pushed to 11.x, thanks!

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

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

Maintainers, please credit people who helped resolve this issue.

  • catch committed 2921fd9e on 11.x 
    Issue #3532092 by baikho, benjifisher: Add Migration Lookup value to...

catch closed merge request !12463

Status: Fixed » Closed (fixed)

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