Following up on my comment in https://www.drupal.org/node/2559571#comment-10282791:

A word on removing the $this->message->display() (and not adding it to the new catch())... display() is meant for messages that go directly to the front-end (drush terminal or web UI page), and my feeling is that the detailed per-row messages should not be sent through display(). My experience is that in many cases, the volume there can be overwhelming and hide more immediately useful messages (like a migration being skipped if dependencies aren't met) - these detailed messages are best reviewed at leisure (yes, I need to implement message viewing in migrate_tools). If there's a strong feeling otherwise, then we should not be expected to always pair calls to saveMessage() and display(), but rather have saveMessage() itself call display().

Better yet, it would be best if the id map plugin had a boolean option whether to display logged messages... Hmmm...

So, I still think in the general migration development case we want the per-row messages to only go into the message tables. However, migrate_upgrade should probably display them as they come, and actually for development you might want to add a --debug option to your drush migrate-import command and see them come out. So, let's add the option.

My current thought is the best way to do this will be for the idmap saveMessage() to dispatch an event, which the MigrateMessageInterface implementor can catch and display if requested. Will try that out...

CommentFileSizeAuthor
#8 interdiff.txt8.51 KBmikeryan
#8 option_to_tee_idmap-2574973-8.patch8.26 KBmikeryan
#5 interdiff.txt499 bytesmikeryan
#5 option_to_tee_idmap-2574973-5.patch11.3 KBmikeryan
#4 option_to_tee_idmap-2574973-4.patch11.29 KBmikeryan
#2 option_to_tee_idmap-2574973-2.patch7.77 KBmikeryan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeryan created an issue. See original summary.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
option_to_tee_idmap-2574973-2.patch7.77 KB

Patch attached, see also #2575127: Support displaying all messages sent to message tables for a patch for migrate_upgrade to make use of this.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs review » Needs work

A discussion with @phenaproxima raises the point, this loses the context of the source ID corresponding to the message, we should make sure that gets put out through the message interface.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
option_to_tee_idmap-2574973-4.patch11.29 KB

Amazing sometimes how much effort it takes to produce a simple-looking test...

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
FileSize
option_to_tee_idmap-2574973-5.patch11.3 KB
interdiff.txt499 bytes

mikeryan-- for messing up a simple implode().

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented 
  1. +++ b/core/modules/migrate/src/Event/MigrateEvents.php
@@ -170,4 +171,19 @@
+   * @Event
+   *
+   * @see \Drupal\migrate\Event\MigratePostRowSaveEvent

    Why @see MigratePostRowSaveEvent? 

  2. +++ b/core/modules/migrate/src/Event/MigrateIdMapMessageEvent.php
@@ -0,0 +1,87 @@
+  /**
+   * Constructs a post-save event object.
+   *
+   * @param \Drupal\migrate\Entity\MigrationInterface $migration
+   *   Migration entity.
+   * @param \Drupal\migrate\Row $row
+   *   Row object.
+   * @param array|bool $destination_id_values
+   *   Values represent the destination ID.
+   */
+  public function __construct(array $source_id_values, $message, $level) {

    This doc comment ain't accurate :) 

  3. +++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -107,6 +108,21 @@ class MigrateExecutable implements MigrateExecutableInterface {
+  /**
+   * Indicates whether messages being logged to the idmap's message tables
+   * should also be logged to $this->message.
+   *
+   * @var bool
+   */
+  protected $logIdMapMessages = FALSE;

    I'm not sure why this is an option, because then we need to explicitly remember to do it. IMO we should *always* register the MigrateExecutable as a message event listener, and pipe only errors and warnings to $this->messages. 

  4. +++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -210,6 +232,11 @@ public function import() {
+    if ($this->logIdMapMessages) {
+      \Drupal::service('event_dispatcher')->addListener(MigrateEvents::IDMAP_MESSAGE,
+        [$this, 'onIdMapMessage']);
+    }

    Should use $this->eventDispatcher. 

  5. +++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -300,6 +327,11 @@ public function import() {
+    if ($this->logIdMapMessages) {
+      \Drupal::service('event_dispatcher')->removeListener(MigrateEvents::IDMAP_MESSAGE,
+        [$this, 'onIdMapMessage']);
+    }

    Ditto. 

  6. +++ b/core/modules/migrate/tests/src/Unit/TestMigrateExecutable.php
@@ -134,4 +134,13 @@ protected function formatSize($size) {
+  /**
+   * Return the migration's idmap plugin.
+   *
+   * @return \Drupal\migrate\Plugin\MigrateIdMapInterface
+   */
+  public function getIdMap() {
+    return $this->migration->getIdMap();
+  }
+

    Not sure what this is used for.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs review » Needs work

Upon discussion with phenaproxima, I'm removing the support for this from MigrateExecutable - it'll be up to the front end to implement the listener and decide when to handle the idmap messages.

Also, fixing copypasta in MigrateIdMapMessageEvent...

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs work » Needs review
FileSize
option_to_tee_idmap-2574973-8.patch8.26 KB
interdiff.txt8.51 KB

Here we're basically just implementing the event, it's up to the front ends to make use of it to choose whether or not to display idmap messages.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented 
+++ b/core/modules/migrate/src/Event/MigrateIdMapMessageEvent.php
@@ -0,0 +1,106 @@
+  /**
+   * Constructs a post-save event object.
+   *

This is still wrong, but can be fixed on commit. :)

Other than that, I think this approach is much cleaner and clearer. RTBC from me once DrupalCI passes it!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

MMM-hmm.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: option_to_tee_idmap-2574973-8.patch, failed testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs work » Reviewed & tested by the community

CI failure, not an actual test failure.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: option_to_tee_idmap-2574973-8.patch, failed testing.

The last submitted patch, 8: option_to_tee_idmap-2574973-8.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Reviewed & tested by the community

  • webchick committed 6a3f127 on 8.0.x 
    Issue #2574973 by mikeryan, phenaproxima: Option to tee idmap messages...
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Category: Feature request » Task
Status: Reviewed & tested by the community » Fixed

Oh, awesome. Really happy to see this ready.

I'm moving from feature request to task. My understanding is that this a requirement for the Migrate UI to show messages from within a migration.

Committed and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.