Postponed on #2976098: MigrateExecutable should add details for the migration & destination property to exceptions that cause a row failure

Problem/Motivation

If a migration is using a lookup with stubbing, and the creation of the stub row has a problem, then the message is saved to the migration you're running, but the message is confusing, because it will typically come from a process plugin that you had no idea was involved in your migration, and will refer to migration mapping properties that are in the lookup migration.

Steps to reproduce

Proposed resolution

Improve the message so it is clear that a stub was being created, to include the destination property and an error message from the initial MigrateSkipRowException.

Remaining tasks

Review
Commit

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#20 Screenshot 2024-02-29 at 10.48.00 AM.png244.82 KBsmustgrave
#6 3197915-6.patch1019 byteskarishmaamin
#2 3197915-2.drupal.skip-row-messages-caused-by-stub-creation-need-more-detail.patch1 KBjoachim

Issue fork drupal-3197915

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

joachim’s picture

joachim CreditAttribution: joachim at TrainingCloud commented
Status: Active » Needs review
FileSize
3197915-2.drupal.skip-row-messages-caused-by-stub-creation-need-more-detail.patch1 KB

At the risk of muddying the waters around various issues about improving migrate messages, here's a quick patch which makes the stub message much clearer.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue summary: View changes
Status: Needs review » Postponed

This will need modification when #2976098: MigrateExecutable should add details for the migration & destination property to exceptions that cause a row failure is committed.

Let's postpone on that.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Postponed » Active
Issue tags: +Needs reroll, +Novice

Needs a reroll.

karishmaamin’s picture

karishmaamin CreditAttribution: karishmaamin at Specbee commented
Status: Active » Needs review
FileSize
3197915-6.patch1019 bytes

Re-rolled patch against 9.3.x

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Needs work
Issue tags: -Needs reroll, -Novice +Needs tests

@karishmaamin, Thanks for the reroll.

I forced an error when running MigrateNodeCompleteTest to see the logged message. It was this, which looks fine except for the case when the original MigrateSkipRowException does not contain an error message. Shall we add an if block and only output the ', with message' when there really is a message.

d7_field_instance:allowed_values: Migration lookup for destination '0' attempted to create a stub using migration d7_taxonomy_vocabulary, which resulted in a row skip, with message ''

Also, needs tests.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dsdeiz’s picture

dsdeiz CreditAttribution: dsdeiz at Skvare commented

Yeah, tried looking at this. Yeah, can I ask what is meant by this?

I forced an error when running MigrateNodeCompleteTest to see the logged message.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Needs work » Needs review
Issue tags: -Needs tests

@dsdeiz, Sorry, I do not recall what I did to force the error.

I took another look here and then went ahead and added the change I proposed in #7. I also updated \Drupal\Tests\migrate\Unit\process\MigrationLookupTest::testTransformWithStubbing to use a data provider with two cases for this change.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Issue summary: View changes

quietone opened merge request !6764

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Title: skip row messages caused by stub creation need more detail » Add details to MigrateSkipRowException messages caused by stub creation
dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Needs review » Needs work
Issue tags: +Bug Smash Initiative

This came up as a random #bugsmash triage target, which is where the new life is coming from. Mostly looks fine. Added a few MR suggestions for some nits.

Tanuj. made their first commit to this issue’s fork.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community
FileSize
Screenshot 2024-02-29 at 10.48.00 AM.png244.82 KB

test

Had to run locally as I couldn't run the test-only feature.

Appears this has already gone through a few rounds of reviews and appears the feedback has been addressed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Reviewed & tested by the community » Needs work

Appears to be one out of scope change in the MR.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Reviewed & tested by the community

Applied my own suggestion to save time here.

Ghost of Drupal Past’s picture

Ghost of Drupal Past
he/him
CreditAttribution: Ghost of Drupal Past commented
Related issues: +#3247718: Allow process plugins to flag a row to be skipped

  • longwave committed 084176b4 on 10.3.x 
    Issue #3197915 by quietone, joachim, Tanuj., karishmaamin, dww:  Add...

  • longwave committed f21bb25c on 11.x 
    Issue #3197915 by quietone, joachim, Tanuj., karishmaamin, dww:  Add...
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 11.x and 10.3.x. Did not backport cleanly to 10.2.x, this is only a minor improvement so not sure it is worth backporting there.

longwave closed merge request !6764

Status: Fixed » Closed (fixed)

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