The MigrateExecutable::import() loop over rows to import calls processRow() to run the process plugins, and only catches MigrateSkipRowException. This means that process plugins have no means to signal errors. We should catch MigrateException, saving the provided status in the map table and logging the provided message to the front end.

The fact that MigrateSkipRowException is caught here and marks the row IGNORED in the map table seems a bit of a WTF to me, given the processing pipeline seems a little late to be deciding a row isn't "officially" part of the source (it's meant for use in prepareRow()). I want to review any existing uses of SkipRow from process plugins and see how they're using this.

CommentFileSizeAuthor
#19 interdiff.txt1.16 KBmikeryan
#19 handle_migrateexception-2559571-19.patch4.13 KBmikeryan
#10 interdiff.txt617 bytesmikeryan
#10 handle_migrateexception-2559571-10.patch4.18 KBmikeryan
#9 interdiff.txt2.02 KBmikeryan
#9 handle_migrateexception-2559571-9.patch4.16 KBmikeryan
#7 interdiff.txt3.38 KBmikeryan
#7 migrateexception_should-2559571-7.patch4.34 KBmikeryan
#2 migrateexception_should-2559571-2.patch1.7 KBmikeryan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeryan created an issue. See original summary.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
migrateexception_should-2559571-2.patch1.7 KB

Quick first pass - obviously needs tests, and as I said I want to look at the SkipRow handling here as well, but the weekend beckons...

Status: Needs review » Needs work

The last submitted patch, 2: migrateexception_should-2559571-2.patch, failed testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Title: MigrateException should be caught when executing process plugins » Handle MigrateException consistently during import

I'll need to update the existing tests to reflect the removal of the redundant display() call. Broadening the scope to reflect that change.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs work » Postponed
Related issues: +#2560435: Need an API for retrieving migration messages

Fixing the tests for the display() removal requires #2560435: Need an API for retrieving migration messages.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Postponed » Active

Unblocked.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Active » Needs review
Issue tags: -Needs tests
FileSize
migrateexception_should-2559571-7.patch4.34 KB
interdiff.txt3.38 KB

Here it is, with tests.

A word on removing the $this->message->display() (and not adding it to the new catch())... display() is meant for messages that go directly to the front-end (drush terminal or web UI page), and my feeling is that the detailed per-row messages should not be sent through display(). My experience is that in many cases, the volume there can be overwhelming and hide more immediately useful messages (like a migration being skipped if dependencies aren't met) - these detailed messages are best reviewed at leisure (yes, I need to implement message viewing in migrate_tools). If there's a strong feeling otherwise, then we should not be expected to always pair calls to saveMessage() and display(), but rather have saveMessage() itself call display().

Better yet, it would be best if the id map plugin had a boolean option whether to display logged messages... Hmmm...

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented

Looks good overall.

  1. +++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -255,6 +255,12 @@ public function import() {
+        $save = $level == MigrationInterface::MESSAGE_INFORMATIONAL || $level == MigrationInterface::MESSAGE_NOTICE;

    What is this doing? 

  2. +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
@@ -281,6 +278,54 @@ public function testImportWithValidRowWithMigrateException() {
+      ->will($this->returnValue(array()))
+      ->will($this->throwException(new MigrateException($exception_message)));

    Not crucial, but I think that for readability we can use willReturn() and willThrow() everywhere in this test.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
FileSize
handle_migrateexception-2559571-9.patch4.16 KB
interdiff.txt2.02 KB

What is this doing?

The idea was that we shouldn't abort the row on mere informational/notice messages. However, since we're in an exception handler, if the exception was thrown some of the processing was skipped, so we don't know the integrity of the row, so I've taken that out. Process plugins should not throw informational/notice-level exceptions, they should call saveIdMapping themselves.

Not crucial, but I think that for readability we can use willReturn() and willThrow() everywhere in this test.

Done!

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
FileSize
handle_migrateexception-2559571-10.patch4.18 KB
interdiff.txt617 bytes

Oops.

The last submitted patch, 9: handle_migrateexception-2559571-9.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented

So one thing that I realize -- because of this patch, it looks like we may no longer be saving rows that throw MigrateExceptions. Is this consistent with what we're already doing? If not, I can see that causing all sorts of pain...

Then again, it does pass all the tests, so maybe my fear is misplaced.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented

We weren't previously catching MigrateException here, so it would get caught somewhere farther up the chain and not only would the current row not be saved, the whole thing would be aborted. No one's throwing MigrateException at this time from a process plugin anyway (although I believe @neclimdul is planning on doing it, the initial inspiration for this whole look at messages and exceptions).

benjy’s picture

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

given the processing pipeline seems a little late to be deciding a row isn't "officially" part of the source

I think this was so that if a process plugin came across something bad, it could skip the row rather than letting the destination try to import it and cause a fatal error. The source doesn't do any validation of it's data and unfortunately neither do our destinations so process can be dealt a bad hand. This issue helps log the exception but wouldn't prevent fatals if we removed MSRE from process plugins?

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -255,6 +255,12 @@ public function import() {
+        $level = $e->getLevel();
+        $this->saveMessage($e->getMessage(), $level);

why do we save $level to a variable but right below just do $e->getMessage() ?

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented

Also, I think it was mainly the migrate process plugin that needed it and we solved that here #2560671: The Migration process plugin should not skip rows

quietone’s picture

quietone CreditAttribution: quietone commented

Applying this patch fixes a problem with Leading slashes not added to visibility paths. In that case we don't want to migrate a block when the block visibility contains php code and the php module is not installed. That case is determined in the BlockVisibility process plugin and throwing a MigrateException with this patch works.

SkipRow didn't work because an empty visibility array is valid, and thus some blocks weren't migrated. And SkipProcess gave an error of Plugin ID '0' was not found, which I didn't investigate because mikeryan1 pointed me to this issue. Thx!

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Issue tags: +blocker
Related issues: +#2532550: Leading slashes not added to visibility paths

This issue helps log the exception but wouldn't prevent fatals if we removed MSRE from process plugins?

The $save = FALSE prevents the destination import from being called.

why do we save $level to a variable but right below just do $e->getMessage()

See @phenaproxima's comment a few back - I had been using $level to decide how to set $save, but then decided to just set $save FALSE. I can inline $e->getLevel() if you'd like.

Tagging "blocker", we're now blocking quietone's patch at #2532550: Leading slashes not added to visibility paths.

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented
Status: Needs review » Reviewed & tested by the community

Inline would be nicer but it isn't a blocker. Rest looks good to me, one question.

+++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
@@ -281,6 +278,54 @@ public function testImportWithValidRowWithMigrateException() {
+      ->willReturn(array())
+      ->willThrowException(new MigrateException($exception_message));

How can a method throw an exception and have a return value? Is the exception meant to bubble up? If so, we usually put the @expectsException on the test method doc.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Reviewed & tested by the community » Needs review
FileSize
handle_migrateexception-2559571-19.patch4.13 KB
interdiff.txt1.16 KB

How can a method throw an exception and have a return value? Is the exception meant to bubble up? If so, we usually put the @expectsException on the test method doc.

The willReturn() was leftover copypasta, I think - removed. The exception is not meant to bubble up, it's meant to be caught in import() (the point of this patch, actually).

Inlined the $e->getLevel() while I was at it.

Thanks!

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented
Status: Needs review » Reviewed & tested by the community

Yeah I didn't think it was, but the return implied otherwise :)

Looks good.

  • webchick committed 32a5344 on 8.0.x 
    Issue #2559571 by mikeryan, benjy, phenaproxima, quietone: Handle...
webchick’s picture

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

Nothing to complain about here. :)

Committed and pushed to 8.0.x. Thanks!

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Issue tags: -blocker

Status: Fixed » Closed (fixed)

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