I have been playing around with the migration system quite a bit and ran into a brick wall when it came to importing content when using Migration process which references a migration that has the destination no_stub option. In particular the Process Migration class (Drupal\migrate\Plugin\migrate\process\Migration) transform method uses cached versions of migrations that have already loaded their destination. This presents a problem because the Stub process uses the Entity Migration method getDestinationPlugin() and passes true in an effort to allow a MigrateSkipRowException. However, that exception will never occur since a cached migration entity is being used that has already loaded the destination without passing true to getDestinationPlugin().

Being relatively unfamiliar with Drupal 8 I was able to implement the below hack from what I learned through several rounds of xdebug sessions. I'm not sure it is the best solution but my migrations with no_stub set to true now seem to be respected with a MigrateSkipRowException being thrown properly.

The code in question is around line 106 of Drupal\migrate\Plugin\migrate\process\Migration. it is the area responsible for resolving the migration to be used to process the stub. Instead of using the existing migration instance I merely load in a new instance. This seems to work since the destination is unresolved and the following call to getDestinationPlugin() with true passed properly triggers the conditional to check for no_stub.

      if ($self) {
        $migration = $this->migrationStorage->load($this->migration->id());
      }
      elseif (isset($this->configuration['stub_id'])) {
        $migration = $this->migrationStorage->load($this->configuration['stub_id']);
      }
      else {
        $migration = $this->migrationStorage->load(reset($migrations)->id());
      }
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

oddz created an issue. See original summary.

oddz’s picture

Issue summary: View changes
oddz’s picture

Issue summary: View changes
oddz’s picture

Issue summary: View changes
oddz’s picture

Issue summary: View changes
mikeryan’s picture

mikeryan’s picture

Status: Active » Needs review
FileSize
9.47 KB

Fail patch demonstrating the problem.

mikeryan’s picture

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.

mikeryan’s picture

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.

The last submitted patch, 7: process_migration-2555127-7-FAIL.patch, failed testing.

mikeryan’s picture

Issue tags: +Migrate critical
FileSize
12.22 KB

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

Status: Needs review » Needs work

The last submitted patch, 11: process_migration-2555127-11.patch, failed testing.

mikeryan’s picture

Status: Needs work » Needs review
FileSize
12.18 KB

Status: Needs review » Needs work

The last submitted patch, 13: process_migration-2555127-13.patch, failed testing.

mikeryan’s picture

Status: Needs work » Needs review
FileSize
13.32 KB
867 bytes

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

mikeryan’s picture

All righty - ready for manual review.

quietone’s picture

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

+1 RTBC

benjy’s picture

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?

mikeryan’s picture

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.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

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.

The last submitted patch, 7: process_migration-2555127-7-FAIL.patch, failed testing.

mikeryan’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
13.65 KB
880 bytes

Comment added, thanks!

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks.

dmoore’s picture

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.

chx’s picture

Status: Reviewed & tested by the community » Needs work

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;
-      }

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

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

mikeryan’s picture

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.

mikeryan’s picture

Yep, that menu links patch fixed this.

dcam’s picture

Issue tags: +Needs reroll

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

mikeryan’s picture

Status: Needs work » Closed (outdated)

Sorry @dcam, neglected to change the status.

kostyashupenko’s picture

Status: Closed (outdated) » Needs review
Issue tags: -Needs reroll
FileSize
13.41 KB

reroll of #22

Status: Needs review » Needs work

The last submitted patch, 32: process_migration-2555127-31.patch, failed testing.

mikeryan’s picture

Status: Needs work » Closed (outdated)

Sorry, the problem is already fixed.

xjm’s picture

Status: Closed (outdated) » Closed (duplicate)