Add Migration Lookup value to error messages

baikho → closed merge request !12462

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

@baikho:

Thanks for working on this issue!

I would like to have more complete steps to reproduce (STR).

Technically "Attempt to do an entity lookup which has no entity to resolve to" is not enough. I am pretty sure that the migration lookup (not an entity lookup) has to fail, and then there has to be an exception when trying to create the stub.

The bigger problem is that I want to know how to test and see what difference the change makes in the context of an actual migration. The update to the unit test only confirms that the re-thrown exception has the expected changes. It does not show me what actually gets logged in the message table.

Good STR would include at least one migration YAML file, and probably two or three. The migration(s) could use the embedded_data source plugin. The migration(s) that trigger(s) the new code should contain rows that cover all possibilities:

• The lookup succeeds.
• The lookup fails, but the stub gets creates.
• The lookup fails, and the stub throws an exception.

#9 Thanks @benjifisher, I have updated the IS accordingly.

@baikho:

Thanks for the additions to the issue summary. That should make testing a lot easier. It may be a week or two before I have time to test, but maybe someone else will pick it up before I do.

Also beyond the error message issue, ...

That looks like it could be a separate issue. Maybe the SkipRow exception should be re-thrown as a SkipProcess exception. Or maybe we can stop using exceptions after 📌 Allow process plugins to flag a row to be skipped Needs work is fixed.

#13

Maybe the SkipRow exception should be re-thrown as a SkipProcess exception. Or maybe we can stop using exceptions after 📌 Allow process plugins to flag a row to be skipped Needs work : Allow process plugins to flag a row to be skipped is fixed.

Yes, or something along these lines seems to work too:

+ $migrate_executable->saveMessage($new_message, MigrationInterface::MESSAGE_INFORMATIONAL);
+ $this->stopPipeline();
+ return NULL;
- throw new MigrateSkipRowException($new_message, 0);