Log message if static_map plugin skips the row

This is nice, but it needs tests.

Add a test.

It is make it fail?

@dravenk, thanks for the patch. The new test code should be in a new test method, each method tests one and only thing about the process plugin. I take it from your question "It is make it fail?" that this wasn't tested locally. I could be wrong of course but if you haven't there are instructions in the handbook for Running PHPUnit tests

Thank you for your guidance.

We've got tests. The IS seems accurate (with a few tweaks now). LGTM.

Crediting the reviewers for getting tests and the issue summary in order. This is just a task and not a feature.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
   @@ -140,7 +140,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   -        throw new MigrateSkipRowException();
   +        throw new MigrateSkipRowException('Skipping, no mapped value found.');
   

   I think this exception could be even more helpful and detail which value is not in the map.

2. +++ b/core/modules/migrate/tests/src/Unit/process/StaticMapTest.php
   @@ -87,5 +87,13 @@ public function testMapWithInvalidSourceAndBypass() {
      }
   -
   +  /**
   +   * Tests when the source is invalid and bypass is empty.
   +   */
   +  public function testMapWithInvalidSourceAndEmptyBypass() {
   +    $configuration['map']['foo']['bar'] = 'baz';
   +    $this->plugin = new StaticMap($configuration, 'map', []);
   +    $this->setExpectedException(MigrateSkipRowException::class, 'Skipping, no mapped value found.');
   +    $this->plugin->transform(['bar'], $this->migrateExecutable, $this->row, 'destinationproperty');
   +  }
    }
   

   The test method should be surrounded by new lines - like all the others.

Thanks for your careful instruction.

@dravenk thanks for the patch - what about #12.1?

Also... nitpicky...

+++ b/core/modules/migrate/tests/src/Unit/process/StaticMapTest.php
@@ -88,4 +88,13 @@ public function testMapWithInvalidSourceAndBypass() {
+  }
 }

There needs to be a new line here too. But not doing that introduces a PHPCS. You can see this by running composer run-script phpcs core/modules/migrate/tests/src/Unit/process/StaticMapTest.php

You'll see:

> phpcs --standard=core/phpcs.xml.dist --runtime-set installed_paths $($COMPOSER_BINARY config vendor-dir)/drupal/coder/coder_sniffer -- 'core/modules/migrate/tests/src/Unit/process/StaticMapTest.php'

FILE: ...ev/core/modules/migrate/tests/src/Unit/process/StaticMapTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 100 | ERROR | [x] The closing brace for the class must have an empty
     |       |     line before it
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 373ms; Memory: 6Mb

I think exceptions should throw information, otherwise developers might have to spend a lot of time trying to figure out what's wrong with the program.
This command line tool is very useful, thank you. : )

@dravenk I agree exceptions should be useful. Wouldn't the most useful exception be:
throw new MigrateSkipRowException(sprintf('Skipping, no mapping found for value: %s.', print_r($value, TRUE)));

+++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -140,7 +140,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+        throw new MigrateSkipRowException(sprintf('Skipping, no mapping found for value: %s.', print_r($value, TRUE)));

Can we include the $destination_property in the exception while we are at it?

Trying to address #18

+++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -140,7 +140,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+        throw new MigrateSkipRowException(sprintf('Skipping, no mapping found for value: %s. and the destination property is: %s', print_r($value, TRUE), $destination_property));

I prefer:

Skipped (foo) for field_bar because no static mapping was encountered.

Cool, lets roll that out

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
   @@ -140,7 +140,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +        throw new MigrateSkipRowException(sprintf('Skipped (%s) for "%s" because no static mapping was encountered.', print_r($value, TRUE), $destination_property));
   

   If we go with parens for the first case (which I like), then we should probably go w/ parens for the second replacement. Or not use them at all. But using quotes for one and parens for the other seems odd. The reason some grouping is helpful, especially for the value is because of spaces. Which isn't so important for the destination property... since it cannot have spaces. But the default value could be rather long and include spaces.

2. +++ b/core/modules/migrate/tests/src/Unit/process/StaticMapTest.php
   @@ -88,4 +88,12 @@ public function testMapWithInvalidSourceAndBypass() {
   +   * Tests when the source is invalid and there is no default value.
   

   This comment is wrong. It tests when there is no mapping and no default value.

+++ b/core/modules/migrate/tests/src/Unit/process/StaticMapTest.php
@@ -88,4 +88,12 @@ public function testMapWithInvalidSourceAndBypass() {
+    $this->setExpectedException(MigrateSkipRowException::class, sprintf('Skipped (%s) for (destinationproperty) because no static mapping was encountered and no default value was provided.', print_r(['bar'], TRUE)));

I feel so sad to kick back on such a small thing. If we do sprintf, let's do that for all replacements. Better off just not using sprintf though and putting the values inline in the test.

For the other things you found in #29, let's open those up as individual follow-ups. This is pretty focused already and would derail something that is so close to RTBC. Individual issues will give us the chance to discuss each of these issues on case by case basis.

@rakesh.gectcr Let me know if you are working on this and I'll step back, I couldn't find you on IRC to ask.

Convert $value to a human-readable string.

@jo No problem thatnks for the patch.

I was trying to do individual follow ups according to the comments #30 and #29.

I have added the follow ups for the MigrateSkipRowException(); in following issues

1. https://www.drupal.org/project/drupal/issues/2959087
2. https://www.drupal.org/project/drupal/issues/2959106
3. https://www.drupal.org/project/drupal/issues/2959097

Then I found that we are not able to log the message on MigrateSkipProcessException
So I have created a follow for that in https://www.drupal.org/project/drupal/issues/2959125
and

1. https://www.drupal.org/project/drupal/issues/2959143#comment-12560638
2. https://www.drupal.org/project/drupal/issues/2959151#comment-12560637

I see now why sprintf was used in the tests. Its really hard to make an assertion on:

'Skipped (Array
(
    [0] => bar
)
) for (destinationproperty) because no static mapping was encountered and no default value was provided.'

Here's the closest to the spirit of what I had intended back in #30, given those limitations. This does everything in a sprintf. Consistency.

Add to meta

I feel this is a pretty novice issue to review. Tagging.

+++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -140,7 +140,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+        throw new MigrateSkipRowException(sprintf('Skipped (%s) for (%s) because no static mapping was encountered and no default value was provided.', print_r($value, TRUE), $destination_property));

Instead of parenthesis around the variables in the string it should be single quotes, parenthesis are not used in any other thrown message in the process plugins in migrate. Sometimes double quotes are used but the single quotes appear to be the most common. And let's keep it simple and change 'encountered' to 'found'.
Actually, looking at the existing MigrateSkipRowException messages they do not include the phrase 'skipped because' or similar, they just state the reason, so this could be even simpler. Perhaps, "No static mapping found for 'foo' and no default value provided for destination 'bar'.

Updated patch based on comments in #41.

This looks great! Just one more thing.

+++ b/core/modules/migrate/tests/src/Unit/process/StaticMapTest.php
@@ -88,4 +88,14 @@ public function testMapWithInvalidSourceAndBypass() {
+   * Tests when there is no mapping and there is no default value.

Let's change this to be in accord with the existing summary lines. Change to 'Tests when the source is invalid and there is no default value.' It makes it much easier to see where this test fits in amongst the others.

Updated the patch based on comments in #44.

I hope this is what you meant.

Assign to review

Thanks everyone! This look good and a new error message is always a welcome addition.

This is an example of the new message.

No static mapping found for 'Array
(
    [0] => bar
    [1] => foo
)
' and no default value provided for destination 'destinationproperty'.

updating credit.

Committed and pushed 522631b064 to 8.6.x and 95dc04ae48 to 8.5.x. Thanks!

diff --git a/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
index 1f3bfcc777..c809ff3b81 100644
--- a/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
+++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -3,6 +3,7 @@
 namespace Drupal\migrate\Plugin\migrate\process;
 
 use Drupal\Component\Utility\NestedArray;
+use Drupal\Component\Utility\Variable;
 use Drupal\migrate\ProcessPluginBase;
 use Drupal\migrate\MigrateException;
 use Drupal\migrate\MigrateExecutableInterface;
@@ -140,7 +141,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
         return $this->configuration['default_value'];
       }
       if (empty($this->configuration['bypass'])) {
-        throw new MigrateSkipRowException(sprintf("No static mapping found for '%s' and no default value provided for destination '%s'.", print_r($value, TRUE), $destination_property));
+        throw new MigrateSkipRowException(sprintf("No static mapping found for '%s' and no default value provided for destination '%s'.", Variable::export($value), $destination_property));
       }
       else {
         return $value;
diff --git a/core/modules/migrate/tests/src/Unit/process/StaticMapTest.php b/core/modules/migrate/tests/src/Unit/process/StaticMapTest.php
index c38a4f172c..1a9a61acd6 100644
--- a/core/modules/migrate/tests/src/Unit/process/StaticMapTest.php
+++ b/core/modules/migrate/tests/src/Unit/process/StaticMapTest.php
@@ -2,6 +2,7 @@
 
 namespace Drupal\Tests\migrate\Unit\process;
 
+use Drupal\Component\Utility\Variable;
 use Drupal\migrate\MigrateException;
 use Drupal\migrate\MigrateSkipRowException;
 use Drupal\migrate\Plugin\migrate\process\StaticMap;
@@ -50,7 +51,7 @@ public function testMapwithEmptySource() {
    * Tests when the source is invalid.
    */
   public function testMapwithInvalidSource() {
-    $this->setExpectedException(MigrateSkipRowException::class);
+    $this->setExpectedException(MigrateSkipRowException::class, sprintf("No static mapping found for '%s' and no default value provided for destination '%s'.", Variable::export(['bar']), 'destinationproperty'));
     $this->plugin->transform(['bar'], $this->migrateExecutable, $this->row, 'destinationproperty');
   }
 
@@ -88,14 +89,4 @@ public function testMapWithInvalidSourceAndBypass() {
     $this->plugin->transform(['bar'], $this->migrateExecutable, $this->row, 'destinationproperty');
   }
 
-  /**
-   * Tests when the source is invalid and there is no default value.
-   */
-  public function testMissingMapAndNoDefaultValue() {
-    $value = ['bar'];
-    $destination_property = 'destinationproperty';
-    $this->setExpectedException(MigrateSkipRowException::class, sprintf("No static mapping found for '%s' and no default value provided for destination '%s'.", print_r($value, TRUE), $destination_property));
-    $this->plugin->transform($value, $this->migrateExecutable, $this->row, $destination_property);
-  }
-
 }

The test is unnecessary - we can adjust testMapwithInvalidSource to have the exception message. Also we should use Variable::export() in preference to print_r() - it's output is a bit more condensed and easier to read and has to Drupalisms.