Process Migration Always Ignores no_stub Due to Pre-Existing Cached Entity Migrations

#2575173: d6_user picture migration generates messages for empty pictures is blocked on this.

Fail patch demonstrating the problem.

A straightforward solution - but I think throwing MigrateSkipRowException is an excessive response in general, and in particular is the wrong response in the #2575173: d6_user picture migration generates messages for empty pictures case. Maybe MigrateSkipProcessException? Or, maybe the solution (as oddz was pursuing) is to respond purely in the migration process plugin.

Just a note to myself for tomorrow, since I don't think I'll finish this tonight - testing the user picture scenario, MigrateSkipProcessException is not the answer because that means the process pipeline ends up with the original incoming source value, whereas in this case we want it to be NULL (so in this scenario the user picture remains unset, and in cases where you do want to abandon the containing entity you can put skip_row_if_empty in your process pipeline).

So, it should be up to the Migration process plugin to recognize the no_stub case and return NULL.

Yes, it does make more sense for the destination to apply its own no_stub configuration, rather than the Migration entity's getDestinationPlugin()...

So, apparently the no_stub schema was defined in the wrong place all along...

All righty - ready for manual review.

Ran the tests, checked the code re standards and it all looks good to me.

+1 RTBC

This looks good to me.

+++ b/core/modules/migrate/src/Entity/Migration.php
@@ -319,11 +318,8 @@ protected function getProcessNormalized(array $process) {
-      if ($stub_being_requested && !empty($this->destination['no_stub'])) {
-        throw new MigrateSkipRowException;
-      }

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -88,6 +88,9 @@ public static function create(ContainerInterface $container, array $configuratio
+    if ($row->isStub() && !empty($this->configuration['no_stub'])) {
+      return [];
+    }

By returning an empty array in import rather than skipping from the process plugin, the executable logs "'New object was not saved, no error provided'" rather than MigrateIdMapInterface::STATUS_IGNORED ?

Am I missing something extra here?

By returning an empty array in import rather than skipping from the process plugin, the executable logs "'New object was not saved, no error provided'" rather than MigrateIdMapInterface::STATUS_IGNORED ?

The empty return when a stub is requested and stubbing is not supported only happens when the import is invoked from the migration plugin, not when run under the (direct) control of the executable. The empty array is returned by the migration plugin, and it's up to subsequent process plugins (if any) to decide what to do when no stub was created - typically I would expect you would use skip_row_if_empty if the field was necessary, default_value if there is a reasonable default you can give, or simply drop through to leave the field empty as in #2575173: d6_user picture migration generates messages for empty pictures.

only happens when the import is invoked from the migration plugin, not when run under the (direct) control of the executable

Ahh, thats the key bit of information. Maybe we should add that in a comment, RTBC otherwise for me.

Comment added, thanks!

Thanks.

I have applied patch process_migration-2555127-21.patch, and can confirm all the File stub creation errors when migrating users have now disappeared, and the user records are being created correctly with an empty user picture.

Great job.

Repeating the same check in every import method is a waste of effort. The menu patch at #2589237: Menu links parent migration is broken fixes the same problem with a few lines of code:

   public function getDestinationPlugin($stub_being_requested = FALSE) {
+    if ($stub_being_requested && !empty($this->destination['no_stub'])) {
+      throw new MigrateSkipRowException;
+    }
     if (!isset($this->destinationPlugin)) {
-      if ($stub_being_requested && !empty($this->destination['no_stub'])) {
-        throw new MigrateSkipRowException;
-      }

Novice task to reroll, and try chx's suggestion.

I'll retest the user picture fix against current core and see if the change in #2589237: Menu links parent migration is broken is enough.

Yep, that menu links patch fixed this.

Tagging for the reroll in advance of Friday's mentored sprints.

Sorry @dcam, neglected to change the status.

reroll of #22

Sorry, the problem is already fixed.