Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
In #2976098: MigrateExecutable should add details for the migration & destination property to exceptions that cause a row failure, the migration ID and destination property was added to migration failure messages.
For a process pipeline with several plugins, that can still leave some detective work finding the source of the problem.
It would be useful to add the process plugin ID.
Steps to reproduce
Trigger a MigrateException in a process plugin. For example, pass an array as the source value to explode.
Proposed resolution
Move the catch in import():
catch (MigrateException $e) {
$this->getIdMap()->saveIdMapping($row, [], $e->getStatus());
$msg = sprintf("%s:%s: %s", $this->migration->getPluginId(), $destination_property_name, $e->getMessage());
$this->saveMessage($msg, $e->getLevel());
$save = FALSE;
}
to processPipeline(), which has the plugin name.
This should then re-throw the exception with the process plugin details prefixed to the message.
Remaining tasks
Review and commit
User interface changes
The plugin id will be displayed on the error messages.
API changes
none
Data model changes
none
Release notes snippet
TBD
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | final.png | 96.06 KB | danflanagan8 |
| #16 | edge-case.png | 41.91 KB | danflanagan8 |
| #16 | lookin-good.png | 43.29 KB | danflanagan8 |
| #7 | index2or3.png | 43.93 KB | danflanagan8 |
| #7 | index1or0.png | 98.96 KB | danflanagan8 |
Issue fork drupal-3254347
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:
changes
Comments
Comment #3
joachim commentedBefore:
> two_images_parallax:_field_parallax_background_image_media: NULL is not a string
In a complex pipeline with several plugins, it's not obvious which plugin has said 'NULL is not a string', and with plugins scattered between core, migrate_plus, and custom code, it's not easy to find the exception message.
After:
> two_images_parallax:_field_parallax_background_image_media:4:explode: NULL is not a string
The message now says it's the 4th plugin in the pipeline, which is 'explode' which produced the message.
Comment #4
danflanagan8I personally like this idea, and the implementation is really simple.
The failing tests are all directly related to the error message being updated here. Those assertions should be easy to update. Setting to NW to get those tests updated.
Comment #6
murilohp commentedMoving back to needs review! I hope the tests pass.
Comment #7
danflanagan8I took a look at this in my new favorite toy, Migrate Sandbox, and found something funny. The way that migrate adds a
getplugin to the pipeline anytimesourceis specified can lead to some strange things. First and foremost, I expect the first process plugin to be index 0 but it gets called index 1.And if I happen to declare
sourcemultiple times in a pipeline (which is rare but allowed), I get an index of 3 where I might be expecting a 1.I'm going to set back to NW. I think subtracting 1 from the index would be good since my second screenshot is something of an edge case.
Comment #8
joachim commentedI did spot the way the plugins were 1-based rather than 0-based, but shrugged at it and figured it just made it easier to understand the output.
Thanks for figuring it out!
> I think subtracting 1 from the index would be good since my second screenshot is something of an edge case.
I think we can handle that though.
If instead of relying on the numeric index of the plugin array here:
we instead manually increment a counter in each iteration of the loop, unless the $plugin is a 'get'.
Comment #9
danflanagan8we instead manually increment a counter in each iteration of the loop, unless the $plugin is a 'get'.I think that would cover almost all real use cases. It's most unusual to explicitly add
getto the yml.Comment #10
joachim commented> I think that would cover almost all real use cases. It's most unusual to explicitly add get to the yml.
Yes, probably, since specifying 'source' does an implicit 'get'.
I can see how we couldd handle that too -- add an internal configuration property to the 'get' plugin called something like 'implicitly_added', set it on all the 'get' plugins the system adds automatically, and pick that up in the count.
But I think that's overkill for a case that might never happen.
Comment #11
joachim commentedComment #12
danflanagan8@joachim, it looks like you did the first part of this, but not the part after the comma: unless the $plugin is a 'get'. There's no check for
getand the behavior appears to be unchanged.Comment #13
joachim commentedOops! Fixed.
Comment #14
quietone commentedSetting to NW for failing tests.
Comment #15
murilohp commentedI updated the tests, it was basically what was mentioned on #4, hope the tests pass.
Thanks!
Comment #16
danflanagan8This looks good to me. The indexes behave much more nicely now. Here's a screenshot (using Migrate Sandbox) to compare with the second one in #7:
The only problem with this implementation, which we have already noted in #10 is that if you happen to explicitly add
getin your pipeline, then the index gets funny. Example:I'm going to tag this for subsystem maintainer review because I think we need buy-in.
Comment #17
quietone commentedAdding the plugin ID to the error message is useful, no doubt about that. The index however is confusing, as the last example shows. I think the index should be removed to avoid that. I guess the worst case scenario is a long pipeline that uses, say, process plugin X multiple times and it is X that has the error. But how often is that? Still with the destination property name and the process plugin it will be much easier to debug. Or one could break up the pipeline and use pseudo properties. I am more concerned about migrations created with migrate-upgrade that have 'get' and are then modified so that some property pipelines have a visible 'get' and others do not.
edit: s/not/no in first sentence.
Comment #18
joachim commentedOk, let's take out the index from this MR.
I'll maybe make a follow-up for only counting visible 'gets' if I have time as per #10.
Comment #19
danflanagan8Thanks for the review, @quietone!
Comment #20
murilohp commentedHey everyone, I removed the index to address #17, and also updated the tests. I'll move this to needs review but I think we need to update the issue summary right? Just because the index will not be displayed anymore.
Thanks!
Comment #21
quietone commented@murilohp, having an update to date issue summary helps well, everyone. And for committers anything we can do to make their task easier is a good thing in my book. Setting to NW for the issue summary update.
Comment #22
murilohp commentedComment #23
murilohp commentedHey @quietone, thanks for the response, updated the issue summary, now we can move to needs review.
Comment #24
murilohp commentedComment #25
danflanagan8I think we're ALMOST done here. The index is no longer part of the message. Here's a screenshot from Migrate Sandbox.
There's still a reference to the index in a code comment though. I'll make a comment on the MR. Back to NW.
:(
Comment #26
murilohp commentedThanks for checking the comments @danflanagan8, now I think we're done.
Comment #27
danflanagan8Thanks, @murilohp. The tests are still running, but we know they'll pass. RTBC!
Comment #28
alexpottCommitted and pushed f21dc6690ee to 10.0.x and 09c149e887d to 9.4.x. Thanks!
This looks helpful information. Nice work. I considered whether we need a CR here and I don;t think so - maybe a contrib test will break but the fix will be obvious and there should not be runtime code relying on exception message strings as an API.