Harden SubProcess process plugin

+++ b/core/modules/migrate/src/Plugin/migrate/process/SubProcess.php
@@ -207,6 +207,9 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+        if ($key === NULL) {

Probably could use the "why" as a comment here.

Adding tests.

+++ b/core/modules/migrate/src/Plugin/migrate/process/SubProcess.php
@@ -207,6 +207,9 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
         }
+        if ($key === NULL) {
+          continue;
+        }
         $return[$key] = $destination;

Still need the why this might happen comment.

FWIW, The two tests cases are that 1) the process creates an id that does not exist or 2) the process creates an id that is NULL.

How that relates to real filter data, I am not sure. Looking at d7_filter_format it seems that module/delta are NULL? So, bad data?

The patch in #8 resolved my problem with text filters. Here's a dump of the {filter} table, if it'd help anyone work out what's causing the problem.

@DamienMcKenna, thank you. I do find it helpful to have real data, not just the test fixture, to look at.

This adds a comment.

RTBC from me, though I think the last line should be "@see \Drupal\filter\Plugin\migrate\process\FilterID"?

Running this on another 8.9.2 codebase (yay upgrading D7 sites!) I now end up with this error:

Missing filter plugin: filter_null

Is this related?

Yes, it should be @see.

Re #18. It is not really related to this issue. The Known Issues page has a section on Text/Input formats which discusses filter_null. Hopefully, that will answer your query.

The latest patch looks good, thank you.

As for filter_null, I didn't realize it was going to throw an error, so I'll look into it; thank you.

patch 19 also applies cleanly to 8.8.x, 8.9.x and 9.1.x
although I haven't yet tested it, next time I get the plugin missing glitch I will test.

I compared the patch in #19 to the one in #12, which was previously RTBC. The later patch adds the comment

        // Do not save the result if the key is NULL. The configured process
        // pipeline used in transformKey() will return NULL if a
        // MigrateSkipProcessException is thrown.
        // @see \Drupal\filter\Plugin\migrate\process\FilterID

in the place indicated in #12.

I do not think it is worth setting the status back to NW, but

        if ($key === NULL) {
          continue;
        }
        $return[$key] = $destination;

at the end of the loop is a little more complicated than

        if ($key !== NULL) {
          $return[$key] = $destination;
        }

and, in the unit test, this looks redundant:

    $this->assertCount(0, $new_value);
    $this->assertArrayEquals([], $new_value);

Fixes for #22.

@quietone:

Thanks for indulging me! The patch in #23 addresses the two tiny issues I pointed out in #22.

Back to RTBC.

+++ b/core/modules/migrate/src/Plugin/migrate/process/SubProcess.php
@@ -207,7 +207,13 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
-        $return[$key] = $destination;
+        // Do not save the result if the key is NULL. The configured process
+        // pipeline used in transformKey() will return NULL if a
+        // MigrateSkipProcessException is thrown.
+        // @see \Drupal\filter\Plugin\migrate\process\FilterID
+        if ($key !== NULL) {
+          $return[$key] = $destination;
+        }

I've stared at this code many times trying to work out if this is correct. I've considered what happens if the sub process whats to set a null value and isn't the call above

        $migrate_executable->processRow($new_row, $this->configuration['process']);
        $destination = $new_row->getDestination();

Also generating a MigrateSkipProcessException? (I believe it is)

So maybe we should change \Drupal\migrate\MigrateExecutable::processRow() to have a mode when it does not catch exceptions and then we could handle the skip row here for better consistency.

However the current solution if okay. If the key transformation returns NULL then this means that it should be unset. I think this should be properly documented in the plugin documentation. In this part:

 * Normally the array returned from sub_process will have its original keys. If
 * you need to change the key, it is possible for the returned array to be keyed
 * by one of the transformed values in the sub-array. For the same source data
 * used in the previous example, the migration below would result to keys
 * 'filter_2' and 'filter_0'.

Something like, If the key is transformed to a NULL then the value will be omitted. Or something like that... I'm not too happy with the end of this but something like that.

Yes, it takes a bit to see what is happening here. I agree that more documentation is a good idea. However, I placed at the top of the doc block instead of in the example sp it is more visible.

Thanks @quietone that looks great. Should this issue be a bug fix?

Yes, I think so.

+++ b/core/modules/migrate/src/Plugin/migrate/process/SubProcess.php
@@ -16,7 +16,8 @@
  * Available configuration keys:
  *   - process: the plugin(s) that will process each element of the source.
  *   - key: runs the process pipeline for the key to determine a new dynamic
- *     name.
+ *     name. If the key is transformed to a NULL then the result of the
+ *     sub_process pipeline is ignored.

What does "transformed to" mean? Does this mean that if the "new dynamic name" is NULL, then the row (array) is skipped?

I think I prefer the suggestion in #25 to say something in the examples section instead of in the configuration section. Maybe both?

When the key is NULL, that row in the sub_process is silently ignored. I am cautious on using 'skip' as that could imply that a MigrateSkipProcessException or MigrateSkipRowException is throw, neither of which is true in this case.

Thanks, I think that is clearer.

Should this issue be a bug fix?

On one hand, it looks like a bug to me. In a recent project, I had a D7 site using the media_filter text filter. I found #3061571: If no Drupal 8 equivalent filter is found during migration, drop transformation-only filters, so that body fields show up and thought, "I am glad that I decided to start with Drupal 8.9.0-beta2 instead of 8.8.0". But then I ran the migrations, and I still got errors from that filter, and the format that used it was not migrated.

On the other hand, #3061571: If no Drupal 8 equivalent filter is found during migration, drop transformation-only filters, so that body fields show up is tagged as a Task, not a bug, so maybe this issue should be the same.

Either way, we should be targeting 9.1.x, not 9.0.x, right? I will ask the testbot to check.

@benjifisher I'd argue that this is a bug exposed by doing #3061571: If no Drupal 8 equivalent filter is found during migration, drop transformation-only filters, so that body fields show up in 8.9.x - hence we should fix all the way back to that release.

Committed and pushed 61c9564477 to 9.1.x and ed21c9b478 to 9.0.x and 83cc4d8e8c to 8.9.x. Thanks!

This fix (patch 30) also works well for D8.8.8 and we don't use migrate so I am willing to hold my nose on that error, we'll be using the patch until our distribution provider gets around to upgrading core to 8.9.x or 9.x

Should this work with 9.2.0 as I am getting the dreaded:

In DiscoveryTrait.php line 53:

[Drupal\Component\Plugin\Exception\PluginNotFoundException]
The "" plugin does not exist. Valid plugin IDs for Drupal\migrate\Plugin\MigratePluginManager are: blah blah blah

I am having this issue on 8.9.16 currently. I haven't migrated from D7, I've been on 8.9.x since the site was created.

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "entity:profile:customer" plugin does not exist. Valid plugin IDs for Drupal\Core\TypedData\TypedDataManager are: ....

(line 53 of /core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).