While we're working on fixing up logging for process plugins, I'd like to see us improve upon the work from #2655154: Optionally log messages for skip_on_empty and skip_row_if_not_set.

That issue added the ability to put a log message into your migration yml when you invoke skip_on_empty or skip_if_not_set. However, the message must be entirely hard-coded directly in the migration configuration. Your message can't use any context from your migration row at all. It'd be a lot more valuable to be able to log messages that are specific.

For example, let's say your migration has some logic to drop various taxonomy terms that are considered invalid in the D8 version of your site. If you're in a d6_term_node migration, and you want to skip because the term is gone, instead of this:

  field_department:
  -
    plugin: migration_lookup:
    migration: d6_taxonomy_term
    source: tid
  -
    plugin: skip_on_empty
    method: row
    message: 'Skipped missing department term'

It'd be nicer to be able do something like this:

  field_department:
  -
    plugin: migration_lookup:
    migration: d6_taxonomy_term
    source: tid
  -
    plugin: skip_on_empty
    method: row
    message: 'Skipped missing department term ($tid)'

A) Yes, I know, technically this specific case could be sort of figured out since the message table has the incomprehensible source_ids_hash column, and if you manually JOIN against the map table, you can eventually figure out what tid you skipped. But that's a lot of hoop jumping. And I'm *sure* we can come up with other examples where the context you want to log doesn't happen to be one of the source IDs.

B) Yes, $tid is totally invalid above. That's meant to be shorthand for a better solution. ;) Things I tried initially with this migration before looking at the code and realizing the message had to be hard-coded directly in the configuration included:

  skip_message:
    plugin: concat
    source:
      - 'Skipped missing department term: '
      - tid
  field_department:
  -
    plugin: migration_lookup:
    migration: d6_taxonomy_term
    source: tid
  -
    plugin: skip_on_empty
    method: row
    message: '@skip_message'

Etc. ;)

Anyway, the specifics of exactly the best way to get this working are TBD. I'm just opening the issue for discussion on the basic concept before I get too lost in the details.

Regardless, this should probably be blocked on #2959125: Not able to add MigrateSkipProcessException message, so starting life here postponed.

Thoughts?
Thanks!
-Derek

CommentFileSizeAuthor
#18 interdiff_16-18.txt940 bytesravi.shankar
#18 2960204-18.patch3.65 KBravi.shankar
#16 interdiff-2960204-15_16.txt679 bytesanchal_gupta
#16 2960204-16.patch4.57 KBanchal_gupta
#15 2960204-15.patch3.65 KBshubham chandra
#13 2960204-13.patch3.65 KBdanflanagan8

Issue fork drupal-2960204

Command icon 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:

About issue forks

Comments

dww created an issue. See original summary.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should 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.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.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.

quietone’s picture

quietone commented
Status: Postponed » Active

This was postponed on #2959125: Not able to add MigrateSkipProcessException message, which has been closed, so this in no longer postponed.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should 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.

quietone’s picture

quietone commented

In #2976098: MigrateExecutable should add details for the migration & destination property to exceptions that cause a row failure some context, the migration_id and the destination property, is being added to all MigrateSkipRowException and MigrateException. That will help.

However, that will not add the source value of the previous process on the pipeline as is suggested in the IS. Not sure how to get that value. Ideas anyone?

dww’s picture

dww
we/he/they
commented

Thanks for the update! Yeah, I haven't had to do any migrations in a while, so I've stopped paying too close attention. ;) Still not sure how to actually do this properly.

josephdpurcell’s picture

josephdpurcell commented

I'm using the migrate_tools MigrateExecutable since it has batching and counts progress. I modified it to support capturing exceptions, see https://gitlab.com/drupalspoons/migrate_tools/-/issues/107.

I know this isn't directly related since this ticket is about capturing context, but the ticket on migrate_tools references a few various tickets that might be useful to anyone who wanders here.

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

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

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

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should 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.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

This would be super useful. I have a migration that uses entity_lookup on an incoming field to look up a taxonomy term, and would like to know if the term doesn't exist (instead of creating it with entity_generate). I can use skip_on_empty to detect this case but would love to include the field value in the message, the ID alone means a bunch of manual investigation to see what the missing field value is.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Version: 9.4.x-dev » 10.0.x-dev
Status: Active » Needs review
StatusFileSize
new 2960204-13.patch3.65 KB

This one caught my eye the other day and then I got an idea for a simple way to implement this. Obviously naming stuff is hard, but I added a new configuration option called message_context that can accept one or more source/destination properties that are then used to fill placeholders in the message by leveraging sprintf.

As an example of what this looks like, for this source data:

foo: 123
bar: abc

and this process pipeline:

skip:
  plugin: skip_on_empty
  source: baz
  method: row
  message: 'Baz was empty while foo was %s and bar was %s'
  message_context:
    - foo
    - bar

the message is:

Baz was empty while foo was 123 and bar was abc

Here's a patch with test coverage included.

Shubham Chandra made their first commit to this issue’s fork.

shubham chandra’s picture

shubham chandra commented
StatusFileSize
new 2960204-15.patch3.65 KB

Reroll the patch #13 with Drupal 10.0.x

anchal_gupta’s picture

anchal_gupta commented
StatusFileSize
new 2960204-16.patch4.57 KB
new interdiff-2960204-15_16.txt679 bytes

I have fix cs error. Please review it

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

PHPStan reports:


  Line   core/tests/Drupal/Tests/Core/Theme/ThemeHookTest.php                 
 ------ --------------------------------------------------------------------- 
  18     Call to static method create() on an unknown class                   
         Drupal\Core\Theme\ThemeHook.
ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review
StatusFileSize
new 2960204-18.patch3.65 KB
new interdiff_16-18.txt940 bytes

Looks like this core/tests/Drupal/Tests/Core/Theme/ThemeHookTest.php extra file got added in patch #16.

So I've removed this file and created new patch.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

Let's take a step back here. This never needed a reroll. The patch in #18 is identical to the patch in #13. No diff whatsoever. The patch in #13 applies and runs on 10.0.x.

Let's please not pollute the issue queues and waste testing resources with unnecessary rerolls.

I'm going to hide all the patches prior to #18, but for any reviewers out there, #13 is the most recent comment that is of any value.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Related issues: +#2714529: Add source and destination IDs to the data returned by getMessageIterator()

As a little more context here, this issue predates a closely related issue: #2714529: Add source and destination IDs to the data returned by getMessageIterator()

These days (since July, 2019) the source ids and destination ids are all included in the migrate message tables. It's not simply the hash and there are no sql queries required to make sense of message anymore.

So I'm not sure that this feature would provide a ton of value. The implementation here doesn't add a ton of complexity and it's kind of neat. I could go either way.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Status: Needs review » Closed (won't fix)

Per @danflanagan8 's request.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

Ha, ha, thanks, @mikelutz

We discussed in the migrate meeting today (#3312722: [meeting] Migrate Meeting 2022-09-29 2100Z). The feeling was this would add complexity without adding much utility, especially given the details in #19. Less is more here.

I am going to steal this idea for the skip_on_condition process plugin in Migrate Conditions where the value will be considerably higher. I just made an issue: #3312746: Allow source/destination properties to appear in message for skip_on_condition

Thanks!