Issue #2554003 by mikeryan, benjy, phenaproxima: isComplete() should not rely on RESULT_COMPLETED

core/modules/file/src/Tests/Migrate/d6/MigrateUploadTest.php

@@ -8,6 +8,7 @@
 namespace Drupal\file\Tests\Migrate\d6;
 
 use Drupal\file\Entity\File;
+use Drupal\migrate\Entity\Migration;
 use Drupal\migrate_drupal\Tests\d6\MigrateDrupal6TestBase;
 use Drupal\node\Entity\Node;
 
@@ -52,7 +53,11 @@ class MigrateUploadTest extends MigrateDrupal6TestBase {
     $this->prepareMigrations($id_mappings);
 
     $this->migrateContent();
-    $this->executeMigration('d6_upload');
+    // Since we are only testing a subset of the file migration, do not check
+    // that the full file migration has been run.
+    $migration = Migration::load('d6_upload');
+    $migration->set('requirements', []);
+    $this->executeMigration($migration);
   }
 
   /**

core/modules/migrate/src/Entity/Migration.php

@@ -387,7 +387,7 @@ class Migration extends ConfigEntityBase implements MigrationInterface, Requirem
     $missing_migrations = array_diff($this->requirements, array_keys($required_migrations));
     // Check if the dependencies are in good shape.
     foreach ($required_migrations as $migration_id => $required_migration) {
-      if (!$required_migration->isComplete()) {
+      if (!$required_migration->allRowsProcessed()) {
         $missing_migrations[] = $migration_id;
       }
     }
@@ -439,15 +439,15 @@ class Migration extends ConfigEntityBase implements MigrationInterface, Requirem
   /**
    * {@inheritdoc}
    */
-  public function setMigrationResult($result) {
+  public function getInterruptionResult() {
-    \Drupal::keyValue('migrate_result')->set($this->id(), $result);
+    return \Drupal::keyValue('migrate_interruption_result')->get($this->id(), static::RESULT_INCOMPLETE);
   }
 
   /**
    * {@inheritdoc}
    */
-  public function getMigrationResult() {
+  public function clearInterruptionResult() {
-    return \Drupal::keyValue('migrate_result')->get($this->id(), static::RESULT_INCOMPLETE);
+    \Drupal::keyValue('migrate_interruption_result')->delete($this->id());
   }
 
   /**
@@ -455,14 +455,24 @@ class Migration extends ConfigEntityBase implements MigrationInterface, Requirem
    */
   public function interruptMigration($result) {
     $this->setStatus(MigrationInterface::STATUS_STOPPING);
-    $this->setMigrationResult($result);
+    \Drupal::keyValue('migrate_interruption_result')->set($this->id(), $result);
   }
 
   /**
    * {@inheritdoc}
    */
-  public function isComplete() {
+  public function allRowsProcessed() {
-    return $this->getMigrationResult() === static::RESULT_COMPLETED;
+    $source_count = $this->getSourcePlugin()->count();
+    // If the source is uncountable, we have no way of knowing if it's
+    // complete, so stipulate that it is.
+    if ($source_count < 0) {
+      return TRUE;
+    }
+    $processed_count = $this->getIdMap()->processedCount();
+    // We don't use == because in some circumstances (like unresolved stubs
+    // being created), the processed count may be higher than the available
+    // source rows.
+    return $source_count <= $processed_count;
   }
 
   /**

core/modules/migrate/src/Entity/MigrationInterface.php

@@ -159,12 +159,12 @@ interface MigrationInterface extends ConfigEntityInterface {
   public function saveHighWater($high_water);
 
   /**
-   * Check if this migration is complete.
+   * Check if all source rows from this migration have been processed.
    *
    * @return bool
    *   TRUE if this migration is complete otherwise FALSE.
    */
-  public function isComplete();
+  public function allRowsProcessed();
 
   /**
    * Set the current migration status.
@@ -191,20 +191,17 @@ interface MigrationInterface extends ConfigEntityInterface {
   public function getStatusLabel();
 
   /**
-   * Set the migration result.
+   * Get the result to return upon interruption.
-   *
-   * @param int $result
-   *   One of the RESULT_* constants.
-   */
-  public function setMigrationResult($result);
-
-  /**
-   * Get the current migration result.
    *
    * @return int
-   *   The current migration result. Defaults to RESULT_INCOMPLETE.
+   *   The current interruption result. Defaults to RESULT_INCOMPLETE.
    */
-  public function getMigrationResult();
+  public function getInterruptionResult();
+
+  /**
+   * Clears the result to return upon interruption.
+   */
+  public function clearInterruptionResult();
 
   /**
    * Signal that the migration should be interrupted with the specified result

core/modules/migrate/src/MigrateExecutable.php

@@ -283,7 +283,8 @@ class MigrateExecutable implements MigrateExecutableInterface {
 
       // If anyone has requested we stop, return the requested result.
       if ($this->migration->getStatus() == MigrationInterface::STATUS_STOPPING) {
-        $return = $this->migration->getMigrationResult();
+        $return = $this->migration->getInterruptionResult();
+        $this->migration->clearInterruptionResult();
         break;
       }
 
@@ -299,7 +300,6 @@ class MigrateExecutable implements MigrateExecutableInterface {
       }
     }
 
-    $this->migration->setMigrationResult($return);
     $this->getEventDispatcher()->dispatch(MigrateEvents::POST_IMPORT, new MigrateImportEvent($this->migration));
     $this->migration->setStatus(MigrationInterface::STATUS_IDLE);
     return $return;
@@ -346,7 +346,8 @@ class MigrateExecutable implements MigrateExecutableInterface {
 
       // If anyone has requested we stop, return the requested result.
       if ($this->migration->getStatus() == MigrationInterface::STATUS_STOPPING) {
-        $return = $this->migration->getMigrationResult();
+        $return = $this->migration->getInterruptionResult();
+        $this->migration->clearInterruptionResult();
         break;
       }
     }
@@ -356,7 +357,6 @@ class MigrateExecutable implements MigrateExecutableInterface {
     }
 
     // Notify modules that rollback attempt was complete.
-    $this->migration->setMigrationResult($return);
     $this->getEventDispatcher()->dispatch(MigrateEvents::POST_ROLLBACK, new MigrateRollbackEvent($this->migration));
     $this->migration->setStatus(MigrationInterface::STATUS_IDLE);
 

core/modules/migrate/src/Tests/MigrateTestBase.php

@@ -153,9 +153,6 @@ abstract class MigrateTestBase extends KernelTestBase implements MigrateMessageI
     foreach ($id_mappings as $migration_id => $data) {
       // Use loadMultiple() here in order to load all variants.
       foreach (Migration::loadMultiple([$migration_id]) as $migration) {
-        // Mark the dependent migrations as complete.
-        $migration->setMigrationResult(MigrationInterface::RESULT_COMPLETED);
-
         $id_map = $migration->getIdMap();
         $id_map->setMessage($this);
         $source_ids = $migration->getSourcePlugin()->getIds();

core/modules/migrate/tests/src/Unit/MigrationTest.php

@@ -96,13 +96,13 @@ class MigrationTest extends UnitTestCase {
     $migration_d = $this->getMock('Drupal\migrate\Entity\MigrationInterface');
 
     $migration_b->expects($this->once())
-      ->method('isComplete')
+      ->method('allRowsProcessed')
       ->willReturn(TRUE);
     $migration_c->expects($this->once())
-      ->method('isComplete')
+      ->method('allRowsProcessed')
       ->willReturn(FALSE);
     $migration_d->expects($this->once())
-      ->method('isComplete')
+      ->method('allRowsProcessed')
       ->willReturn(TRUE);
 
     $migration_storage = $this->getMock('Drupal\Core\Entity\EntityStorageInterface');