Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#32 | interdiff.2969551.27-31.txt | 719 bytes | mikelutz |
#32 | 2969551-31.drupal.patch | 2.49 KB | mikelutz |
#27 | 2969551-27.patch | 2.48 KB | quietone |
#27 | interdiff-25-27.txt | 739 bytes | quietone |
#25 | 2969551-25.patch | 2.49 KB | quietone |
Comments
Comment #2
joachim CreditAttribution: joachim as a volunteer commentedComment #7
Wim LeersComment #8
Wim LeersRe-testing against 8.9.
Comment #9
mikelutzI tagged it needs tests, really it just needs the test updated to pass, I believe.
Comment #10
Wim Leers#9: Ooohh! That's exciting! I didn't realize this was so close already 🥳
Comment #12
quietone CreditAttribution: quietone as a volunteer commentedAnd one updated test.
Comment #14
quietone CreditAttribution: quietone as a volunteer commentedThe failure in the test of the patch which should be passing tests is in an unrelated test, Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest
Comment #15
quietone CreditAttribution: quietone as a volunteer commentedRetested and passing now
Comment #16
heddnQuick, easy fix.
Comment #18
adityasingh CreditAttribution: adityasingh as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #19
quietone CreditAttribution: quietone as a volunteer commented@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?
Comment #20
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #22
quietone CreditAttribution: quietone as a volunteer commentedRandom test failure, #3099427: [random test failure] FieldLayoutTest::testEntityView(). Retesting
Comment #23
benjifisherThe test passed. Back to RTBC.
Comment #24
catchI 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.
Comment #25
quietone CreditAttribution: quietone as a volunteer commentedChanged to using __LINE__ to get the line number.
Comment #26
benjifisherJust looking at the interdiff, I notice I notice this comment:
Comment #27
quietone CreditAttribution: quietone as a volunteer commented#26. Comment fixed
Thanks!
Comment #28
mikelutzI like it, this will be helpful. Code LGTM and all feedback is addressed.
Comment #29
larowlanReview credits
Comment #30
larowlanPHPCS 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:
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
Comment #31
mikelutzTo 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.Comment #32
mikelutzActually, here's the patch anyway. I just can't RTBC it now.
Comment #34
larowlanCrosspost with #32
Fixed on commit
Committed c915bfe and pushed to 9.1.x. Thanks!
Comment #36
larowlanBackported this to 9.0.x, waiting on test-run for 8.9.x
Comment #38
larowlanBackported to 8.9.x because the risk of disruption is low and keeping the branches in sync will make things easier for contributors.