Problem/Motivation

There are quite a few issues for improving or adding message to the SkipRowExceptions in some process plugins. So I asked myself if the other process plugins could use similar improvement, thus this meta to see what needs to be done.

There are suggestions to add the destination property (#7) and the migration name (#8) and in one of the child issues heddn suggested 'as much context as possible'.

There is this to add file and line number #2976098: MigrateExecutable should add details for the migration & destination property to exceptions that cause a row failure.

Using that as a guide each process plugin should have the destination property and the migration name as a minimum, and then more added as needed for that particular process plugin. When in doubt, refer to the coding standards for Php Exceptions.

The child issues for each process plugin can probably be novice issues as well.

These plugin throw exceptions

These plugins do not throw exceptions

  • Callback
  • DefaultValue
  • DedupeBase
  • DedupeEntity
  • EntityExists
  • FieldFormatterSettingsDefaults
  • Get
  • Log
  • MachineName
  • MakeUniqueBase
  • MakeUniqueEntityField
  • Route
  • SubProcess

These plugins throw exceptions and have messages that don't seem to need improvement or has been fixed. Make a new issue if you disagree. And the final message will be improved when #2959444: [Meta] Improve exception messages in process plugins is fixed.

Proposed resolution

Add as much context as sensible to each process plugin.

Remaining tasks

Review a process plugin and if the exception message needs improvement then make an issue. If not sure ask here.

Original work
Process plugins improved in the first round, before the move to adding destination property, migration name and anything deemed necessary.

Comments

quietone created an issue. See original summary.

dww’s picture

dww
we/he/they
commented
Issue summary: View changes
Issue tags: +DX (Developer Experience)

I love this issue and what it's trying to coordinate / accomplish. Logging useful messages during migrations would be a huge win for the DX of folks writing/running migrations.

Meanwhile, I added another child issue that's related to this: #2960204: Make it possible for optional log messages from *skip* process plugins to contain context. It's not exactly in scope for what this task/plan is about, but I figured it was close enough to justify putting it under this. If anyone strongly objects, feel free to clear its parent issue reference field and it can stand alone.

Thanks,
-Derek

quietone’s picture

quietone commented
Issue summary: View changes

Read through the migrate process plugins and updated the IS. Added in information for current issues. Looks like rakesh.gectcr did a great job identifying all the skips processes that need better exception messages.

TODO:
Check plugins in other modules.
Add an issue for SkipRowIfNotSet

quietone’s picture

quietone commented
Issue summary: View changes

Grepped for throws in all other process plugins and added them to the list.

quietone’s picture

quietone commented
Title: [Meta] Improve messages in process plugins » [Meta] Improve exception messages in process plugins

Changed title since this is just the exception messages.

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.

quietone’s picture

quietone commented
Issue summary: View changes

Adding issue for format date.

The suggestion in that issue is to add the destination property to the exception message. It might be a good idea to add the destination property to all the process plugin exception messages. Just a thought.

joachim’s picture

joachim commented

> It might be a good idea to add the destination property to all the process plugin exception messages. Just a thought.

Yes, we should definitely add the destination property to all process plugin exceptions.

I was actually thinking of filing an issue to generalize that, but I didn't know this one existed.

I would say also that the migration name needs to be given in the exception message, as when stubs are being created, you can be several migrations away from the one you are actually running.

quietone’s picture

quietone commented
Issue summary: View changes

Update the IS to move this add more context to the messages and clear the committed issues from the list as they need to be double checked to see if they need more detail added.

joachim’s picture

joachim commented

I was wondering if there was a way we could enhance the DX of this, either by providing a method in ProcessPluginBase, or by having the migrate system catch any process exceptions & rethrow them with the standard list of details.

quietone’s picture

quietone commented
Related issues: +#2996293: Improve exception logging, +#2969551: Migrate messages from caught exceptions need file and line details

Yes, let's find a better way to do this!

Add two issues about error messages to related issues.

jofitz’s picture

jofitz
he/him
commented
Issue summary: View changes

Add Explode.php child issue.

quietone’s picture

quietone commented
Issue summary: View changes

Add the coding standard for exceptions to the IS.

quietone’s picture

quietone commented

Adding in a comment from heddn in #2959143: Log message if migration_lookup skips on empty

From facing this personally, we should print as much context as possible, including source_ids, migrations and value. We don't have all that context though inside of skipOnEmpty, so we might need to pass it.

And Jo Fitzgerald pointed out later in that issues, where do we get the context from?

So far the suggestions solutions are from joachim,

providing a method in ProcessPluginBase, or by having the migrate system catch any process exceptions & rethrow them with the standard list of details.

Note that the two related issues offer patches for the improving the exception messages thrown during MigrateExecutable::import(). And with my sense of consistency I think it would be worth handling the process exception in import() as well.

Any thoughts?

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
Location Ghent 🇧🇪🇪🇺
commented

Shouldn't this be bumped to Major priority?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

(Also made this the parent for #2969231: errors in migration process configuration don't give a clear message and #2969551: Migrate messages from caught exceptions need file and line details.)

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.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Issue summary: View changes

I just added #3148959: Improve migrate messages from the extract plugin as a child issue, so I am adding it to the issue summary here.

I see that #2976098: MigrateExecutable should add details for the migration & destination property to exceptions that cause a row failure has been added as a child issue but not mentioned in the comments here. That issue will add the destination property (#7) and the migration name (#8) when the exception is caught. IMO that is a better solution than adding that information when the exception is thrown.

quietone’s picture

quietone commented
Issue summary: View changes
Related issues: +#3047328: Allow logging for non-strings values

There is now an issue for Flatten.

quietone’s picture

quietone commented
Issue summary: View changes

Went through the process plugins and briefly reviewed the error messages. To me, they look like they don't need improvement, especially with the migration_id and the destination property name being added in #2976098: MigrateExecutable should add details for the migration & destination property to exceptions that cause a row failure.

quietone’s picture

quietone commented
Issue summary: View changes

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

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.