The MigrateExecutable class catches exceptions and turns then into messages for the migration.

But in doing so, it loses information, as it only takes the exception message and not the place where it happened.

Sometimes that's enough, if the exception message is quite specific. But often it's not, as you've no idea where something like an 'Undefined offset' error can have happened.

For example, when running migration tests, I got this output from PHPUnit, which is pretty unhelpful:

1) Drupal\Tests\commerce_migrate_ubercart\Kernel\Migrate\d6\ProfileBillingTest::testProfileBilling
Migration failed with source plugin exception: Undefined offset: 0
Failed asserting that false is true.

/Users/joachim/Sites/drupal-8-commerce/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:23
/Users/joachim/Sites/drupal-8-commerce/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php:195
/Users/joachim/Sites/drupal-8-commerce/core/modules/migrate/src/MigrateExecutable.php:275
/Users/joachim/Sites/drupal-8-commerce/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php:169
/Users/joachim/Sites/drupal-8-commerce/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php:183
/Users/joachim/Sites/drupal-8-commerce/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php:184
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

joachim’s picture

Status: Needs review » Needs work

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Wim Leers’s picture

Wim Leers’s picture

Status: Needs work » Needs review

Re-testing against 8.9.

mikelutz’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

I tagged it needs tests, really it just needs the test updated to pass, I believe.

Wim Leers’s picture

#9: Ooohh! That's exciting! I didn't realize this was so close already 🥳

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
571 bytes
719 bytes
2.11 KB

And one updated test.

Status: Needs review » Needs work

The last submitted patch, 12: 2969551-12.patch, failed testing. View results

quietone’s picture

Issue tags: +Bug Smash Initiative

The failure in the test of the patch which should be passing tests is in an unrelated test, Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest

quietone’s picture

Status: Needs work » Needs review

Retested and passing now

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Quick, easy fix.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 2969551-12.patch, failed testing. View results

adityasingh’s picture

Status: Needs work » Needs review
FileSize
2.11 KB
quietone’s picture

Status: Needs review » Needs work

@adityasingh, thanks for taking an interest in this issue. To assist reviewers please provide an interdiff when uploading a patch, There are instructions Creating an interdiff in the handbook. I chose to do that locally and the patch in #19 is the same as in the patch in #12. What was the reason for uploading an identical patch?

quietone’s picture

Status: Needs work » Reviewed & tested by the community

@adityasingh, ah, I forgot that this was the patch with a random test failure. Maybe you wanted to retest the patch? If so, use the 'Add test/retest' link to restart the test. Since the patch is a duplicate of an existing one I removing credit.

This patch was RTBC and then had a random test fail. The test is now passing so setting back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2969551-18.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
benjifisher’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Global2020

The test passed. Back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
@@ -72,6 +72,7 @@ public function testImportWithFailingRewind() {
 
     // Ensure that a message with the proper message was added.
+    $exception_message .= " in " . __FILE__ . " line 68";
     $this->message->expects($this->once())

I don't think we should add the line number in the test, this will make even docs changes in the first 67 lines of MigrateExecutable cause test failures.

quietone’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
2.49 KB

Changed to using __LINE__ to get the line number.

benjifisher’s picture

Status: Needs review » Needs work

Just looking at the interdiff, I notice I notice this comment:

+    // The exception message contains the line number where it is thrown. Save
+    // it for testing the testing the exception message.
quietone’s picture

Status: Needs work » Needs review
FileSize
739 bytes
2.48 KB

#26. Comment fixed

Thanks!

mikelutz’s picture

Status: Needs review » Reviewed & tested by the community

I like it, this will be helpful. Code LGTM and all feedback is addressed.

larowlan’s picture

Review credits

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

PHPCS is complaining about "A unary operator statement must not be followed by a space" on line 71 of MigrateExecutableTest, but I think that is actually an error in PHPCS.

I guess we can do this:

diff --git a/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
index 751367f8a4..f1a855de1f 100644
--- a/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
+++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
@@ -68,6 +68,7 @@ public function testImportWithFailingRewind() {
       ->will($this->throwException(new \Exception($exception_message)));
     // The exception message contains the line number where it is thrown. Save
     // it for the testing the exception message.
+    // phpcs:ignore
     $line = __LINE__ - 3;

     $this->migration->expects($this->any())

But then we don't have a trail to go back if it is fixed later.

Wondering if we can rewrite this to do the -3 later on and avoid needing to block this on a coder/Php_Code_Sniffer fix?

We should be filing an issue for them though

mikelutz’s picture

To make it more complicated, if you go with the first option, you have to make it __LINE__ - 4, and know to change it back to 3 if you remove the extra comment.

I found that $line = (__LINE__) - 3; works fine if you want to just fix it on commit, if not I'll push a patch up tomorrow and get someone else to RTBC it.

mikelutz’s picture

Actually, here's the patch anyway. I just can't RTBC it now.

  • larowlan committed c915bfe on 9.1.x
    Issue #2969551 by quietone, mikelutz, joachim, benjifisher, catch:...
larowlan’s picture

Title: Migrate messages from caught exceptions need file and line details » [backport] Migrate messages from caught exceptions need file and line details
Version: 9.1.x-dev » 9.0.x-dev
Status: Needs review » Reviewed & tested by the community

Crosspost with #32

Fixed on commit

	diff --git a/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
		index 751367f8a4..68bf7d02d6 100644
		--- a/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
		+++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
		@@ -68,7 +68,7 @@ public function testImportWithFailingRewind() {
		       ->will($this->throwException(new \Exception($exception_message)));
		     // The exception message contains the line number where it is thrown. Save
		     // it for the testing the exception message.
		-    $line = __LINE__ - 3;
		+    $line = (__LINE__) - 3;
		 
		     $this->migration->expects($this->any())
		       ->method('getSourcePlugin')

Committed c915bfe and pushed to 9.1.x. Thanks!

  • larowlan committed caec1a5 on 9.0.x
    Issue #2969551 by quietone, mikelutz, joachim, benjifisher, catch:...
larowlan’s picture

Version: 9.0.x-dev » 8.9.x-dev

Backported this to 9.0.x, waiting on test-run for 8.9.x

  • larowlan committed 3cd84c3 on 8.9.x
    Issue #2969551 by quietone, mikelutz, joachim, benjifisher, catch:...
larowlan’s picture

Title: [backport] Migrate messages from caught exceptions need file and line details » Migrate messages from caught exceptions need file and line details
Status: Reviewed & tested by the community » Fixed

Backported to 8.9.x because the risk of disruption is low and keeping the branches in sync will make things easier for contributors.

Status: Fixed » Closed (fixed)

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