Log message if skip_on_empty plugin skips on empty

+++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -112,7 +112,7 @@ public function row($value, MigrateExecutableInterface $migrate_executable, Row
+      throw new MigrateSkipProcessException("Skipped because of an empty value.");

+++ b/core/modules/migrate/tests/src/Unit/process/SkipOnEmptyTest.php
@@ -19,7 +19,7 @@ class SkipOnEmptyTest extends MigrateProcessTestCase {
+    $this->setExpectedException(MigrateSkipProcessException::class, "Skipped because of an empty value.");

Can we print contextual info in the exception?

Added contextual information in the exception.

@Jo Fitzgerald, thanks for picking up these message improvement issues.

I think the row and process method should implement the same logic for a log message. With this patch the process method has a set exception message but the row method displays no message unless one is defined in the plugin configuration. How about we change both so that if there is a message in the configuration, that is displayed, otherwise use a default message.

The default message I currently like is 'The process was skipped because the source property foo was empty.' because the tenses are all the same, there is no use of a future perfect tense and it uses 'empty' to be differentiate that the message is not from the SkipRowIfNotSet plugin.

Addressed the comments in #7 and updated the tests accordingly.

Aww looks like I made a massive, incorrect assumption about the 'source' property. I'm not sure how else to obtain it. Any thoughts, @quietone?

Right then, let's refer to the destination property, that will be easier to locate in the migration anyway. This needed a reroll so there is no interdiff.

Skipping message for row passed the tests. So believe the problem is something else.
Also, I have updated the message for the process (from 'row' -> 'process').
Please check the screenshot for what I'm getting as messages array in MigrateLanguageContentSettingsTest.

Restore the new log message to the row method. And in the MigrateLanguageContentSettingsTest where it checks that there are not any messages from the migrating, explicitly remove any from the skip_on_empty plugin and then assert the messages array is empty. And having to make that changes makes we wonder about always outputting a message on a skip_on_empty, after all it is used a lot. And there is nothing here to say, hey I don't want message on this instance of skip_on_empty. What do you think?

In general, I think a "verbose" mode and a "quiet" mode would make the most sense -- in verbose we would log all this, skip on empty, mapping default/skip and so forth. That's for development. And when running live, then we shouldn't "overlog". My 2c.

Agree w/ #22.

I'm not a huge fan of doubling down on our use of exceptions as signals here, but I also am not sure I see the point of this. A row skip allows a message to be passed that is used as the exception message. That exception is caught in the executable and used as the row log message. This makes sense as we get one message per row that we can log and skipping the row is definitely worthy of being that message.

Skipping a process is different. The process exception gets thrown, caught in the executable which throws it away and moves on to the next process. There's no point in adding a message because the message isn't used or logged or even seen because the exception is only used as an internal signal. It can't bubble up to be used as the message log because many processes could be skipped on a single row.

In #3004927: Create Migration Lookup and Stub services I'm introducing a migrate logger channel service. I propose that once that is in we start injecting logging into the rest of the migration system that way.

I'm setting to NW at least because the message in the exception isn't used anywhere currently, so I don't think we are really doing anything here.

#22. Added a 'log' configuration property, although it is just a toggle, not 'verbose' and 'quiet'. Now, messages are only logged when 'log' is not empty. That changes current behavior so I guess we need a BC layer.
#35 This removes messages on a skip process.

+++ b/core/modules/migrate/src/Plugin/migrate/process/SkipOnEmpty.php
@@ -24,6 +24,7 @@
+ * - log: (optional): Set to TRUE to log messages. Defaults to FALSE.

@@ -84,7 +85,10 @@
-      $message = !empty($this->configuration['message']) ? $this->configuration['message'] : sprintf("The row was skipped because the source property for destination '%s' was empty.", $destination_property);
+      $message = '';
+      if (!empty($this->configuration['log'])) {
+        $message = !empty($this->configuration['message']) ? $this->configuration['message'] : sprintf("The row was skipped because the source property for destination '%s' was empty.", $destination_property);
+      }

Yeah, this changes the behavior. I suppose you could pass through $message like normal if it exists, but only pass through the new message if the configuration message doesn't exist and the log configuration is set to true, but this whole thing really feels like a closed works as designed to me. If you want to log a message for your skip row on empty, then we have a means to do that in your migration already.

Reading through this again I am inclined to agree with mikelutz that this is a won't fix. As he points out, there is already a mechanism to log a message when skipping a row.

I do like the suggestion in #22 top have a verbose mode when running migrations. But that is certainly for another issue.