Optionally log messages for skip_on_empty and skip_row_if_not_set

The migration message table is the logical place for row-specific messages like this. Should be fairly simple - where MigrateSkipRowException is caught, do a $this->idMap->saveMessage(). And of course anything that throws MigrateSkipRowException should be giving it a message...

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -224,6 +224,7 @@ public function import() {
+        $this->saveMessage($e->getMessage(), MigrateIdMapInterface::STATUS_IGNORED);

Let's skip this when the message is empty, no point in cluttering the message table with empty messages.

A test would be good.

@generalredneck: I'm very sorry I didn't get back to this issue more promptly, or immediately recall it when getting involved with reviewing #2818871: Allow logging skip row exception messages - that issue covers much of your work here (with identical code - you deserve an issue credit on that). For what it's worth, on today's migrate maintainer call we discussed doing a better job of keeping up with the "Needs review" issues for the migration system so patches don't languish - we formally parceled out the oldest (least-recently-updated) in the queue, which is how I came back to this one. We should do better going forward.

Having an option to log the skip_on_empty process plugin is still a good idea, though - you'll need to reroll due to the redundant changes in the other issue.

+++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -24,7 +24,13 @@ class SkipOnEmpty extends ProcessPluginBase {
+      if (isset($this->configuration['quiet']) && $this->configuration['quiet']) {

I think a "log_message" configuration option (with the opposite sense) would be more clear, with the default in its absence being not to log (maintaining the current behavior and not surprising people with messages they weren't expecting).

Weird, I don't see why this patch would introduce requirements failures - retesting.

+++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -24,7 +26,13 @@ class SkipOnEmpty extends ProcessPluginBase {
+      throw new MigrateSkipRowException($message, !empty($message));

Here's the problem - remove the second argument, we should *always* record the skip in the map table.

I removed second argument ( !empty($message) )

Main patch looks good, just a couple of test tweaks...

1. +++ b/core/modules/migrate/tests/src/Unit/process/SkipOnEmptyTest.php
   @@ -51,4 +51,37 @@ public function testRowBypassesOnNonEmpty() {
   +      (new SkipOnEmpty($configuration, 'skip_on_empty', []))
   +      ->transform('', $this->migrateExecutable, $this->row, 'destinationproperty');
   ...
   +      (new SkipOnEmpty($configuration, 'skip_on_empty', []))
   +      ->transform('', $this->migrateExecutable, $this->row, 'destinationproperty');
   

   The second line should be indented. Although, actually, I think it would be cleaner to assign to SkipOnEmpty plugin to a variable and call transform on the variable.

2. +++ b/core/modules/migrate/tests/src/Unit/process/SkipOnEmptyTest.php
   @@ -51,4 +51,37 @@ public function testRowBypassesOnNonEmpty() {
   +    catch(\Drupal\migrate\MigrateSkipRowException $e) {
   ...
   +    catch(\Drupal\migrate\MigrateSkipRowException $e) {
   

   Please use a 'use' statement for the exception and reference it as simply MigrateSkipRowException.

+1 for this feature, but:

1. We want flexible messages, instead of only logging a static message. For example a comment migration wants to say: Missed parent node and other migration something different, depending on its business logic. These messages are very valuable for content managers and operators.
2. Not only skip_on_empty but also skip_row_if_not_set should benefit from this.

Redefining the IS according to these considerations.

Oh, but this is a great idea. The patch looks good. My concerns are nitpicks, code standards, and documentation-related.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
   @@ -21,6 +21,9 @@
   + *   for this row. Logging a message has effect only for 'row' method. If is
   + *   missed, nothing is logged in the message table.
   

   The last sentence seems a bit unclear. And can the second-to-last sentence read "Messages are only logged for the 'row' skip level"?

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
   @@ -30,9 +33,11 @@
   + * If field_name is empty, skips the entire row and log 'Field field_name is
   

   s/log/logs

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipRowIfNotSet.php
   @@ -16,6 +16,8 @@
   + *   for this row. If is missed, nothing is logged in the message table.
   

   The last sentence is phrased strangely. Maybe "is missed" should be "if not set"?

4. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipRowIfNotSet.php
   @@ -26,10 +28,11 @@
   + * skipped and the "Missed the 'data' key" will be registered.
   

   This should end with "and the message "Missed the 'data' key" will be logged".

5. +++ b/core/modules/migrate/tests/src/Unit/process/SkipOnEmptyTest.php
   @@ -51,4 +52,35 @@ public function testRowBypassesOnNonEmpty() {
   +   * @expectedException \Drupal\migrate\MigrateSkipRowException
   +   * @expectedExceptionMessage
   +   */
   +  public function testRowSkipWithoutMessage() {
   

   I have recently been informed that we need to use $this->setExpectedException() in the test method, rather than the annotation.

6. +++ b/core/modules/migrate/tests/src/Unit/process/SkipOnEmptyTest.php
   @@ -51,4 +52,35 @@ public function testRowBypassesOnNonEmpty() {
   +   * @expectedException \Drupal\migrate\MigrateSkipRowException
   +   * @expectedExceptionMessage The value is empty
   +   */
   +  public function testRowSkipWithMessage() {
   

   Same here.

7. +++ b/core/modules/migrate/tests/src/Unit/process/SkipRowIfNotSetTest.php
   @@ -0,0 +1,46 @@
   +   * @expectedException \Drupal\migrate\MigrateSkipRowException
   +   * @expectedExceptionMessage
   +   */
   +  public function testRowSkipWithoutMessage() {
   

   And here.

8. +++ b/core/modules/migrate/tests/src/Unit/process/SkipRowIfNotSetTest.php
   @@ -0,0 +1,46 @@
   +   * @expectedException \Drupal\migrate\MigrateSkipRowException
   +   * @expectedExceptionMessage The 'some_key' key is not set
   +   */
   +  public function testRowSkipWithMessage() {
   

   And here.

@phenaproxima

I have recently been informed that we need to use $this->setExpectedException() in the test method, rather than the annotation.

Why and where can I found docs about that?

There is an issue regarding this https://www.drupal.org/node/2822837 , but it has been postponed. So should we use method $this->setExpectedException() ?

Indeed, a Coder policy is prepared in #2859286: Forbid @expectedException @expectedExceptionMessage.

1. +++ b/core/modules/migrate/tests/src/Unit/process/SkipOnEmptyTest.php
   @@ -51,4 +52,35 @@ public function testRowBypassesOnNonEmpty() {
   +   * @expectedException \Drupal\migrate\MigrateSkipRowException
   +   * @expectedExceptionMessage
   ...
   +   * @expectedException \Drupal\migrate\MigrateSkipRowException
   +   * @expectedExceptionMessage The value is empty
   
   +++ b/core/modules/migrate/tests/src/Unit/process/SkipRowIfNotSetTest.php
   @@ -0,0 +1,46 @@
   +   * @expectedExceptionMessage
   ...
   +    $this->setExpectedException(\Drupal\migrate\MigrateSkipRowException);
   ...
   +   * @expectedExceptionMessage The 'some_key' key is not set
   ...
   +    $this->setExpectedException(\Drupal\migrate\MigrateSkipRowException);
   

   All these annotations should be removed and $this->setException() should be before the point where the exception is expected. Also you'll need to pass the expected message. Use the MigrateSkipRowException::class form.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
   @@ -30,9 +33,11 @@
   + * If field_name is empty, skips the entire row and logs 'Field field_name is
   + * missed' in the message table.
   

   ... and logs the message 'Field...

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/SkipRowIfNotSet.php
   @@ -16,6 +16,8 @@
   + *   for this row. If is missed, nothing is logged in the message table.
   

   Replace "If is missed... " with "If not set, nothing is logged in the message table."

4. +++ b/core/modules/migrate/tests/src/Unit/process/SkipOnEmptyTest.php
   @@ -13,12 +14,12 @@ class SkipOnEmptyTest extends MigrateProcessTestCase {
   +    $this->setExpectedException(\Drupal\migrate\MigrateSkipProcessException);
   
   @@ -33,12 +34,12 @@ public function testProcessBypassesOnNonEmpty() {
   -   * @expectedException \Drupal\migrate\MigrateSkipRowException
   ...
   +    $this->setExpectedException(\Drupal\migrate\MigrateSkipRowException);
   

   Unrelated changes. Revert them.

Fixed #34.

Methinks it's ready. Thanks!

Re-rolled.

It's only a reroll, so back to RTBC.

As this is a non breaking API addition to and experimental module backported to 8.3.x.

Committed and pushed e53654a to 8.4.x and bf80ef2 to 8.3.x. Thanks!

I discussed with @mikeryan today and he convinced me that this will be a useful change. We also discussed the possible of following up with a verbose switch to report all skipped rows.

$message = !empty($this->configuration['message']) ? $this->configuration['message'] : '';
+      throw new MigrateSkipRowException($message);

This one never works , throws exception but doesn't log anything.

drush mmsg %migration_id% should give you log results. If it doesn't, please open a new issue to investigate the regression.

Dear people who added this cool feature:

a) Thanks!
b) RFC: #2960204: Make it possible for optional log messages from *skip* process plugins to contain context

Cheers,
-Derek