Migrate messages from caught exceptions need file and line details

Re-testing against 8.9.

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

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

And one updated test.

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

Retested and passing now

Quick, easy fix.

@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?

@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.

Random test failure, #3099427: [random test failure] FieldLayoutTest::testEntityView(). Retesting

The test passed. Back to RTBC.

+++ 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.

Changed to using __LINE__ to get the line number.

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.

#26. Comment fixed

Thanks!

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

Review credits

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

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.

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

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!

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

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