Track current state of migrations

1. +++ b/core/modules/migrate/src/Entity/Migration.php
   @@ -372,6 +372,24 @@ protected function getEntityManager() {
   +  public function setMigrationStatus($status) {
   +    \Drupal::state()->set('migrate.' . $this->id() . '.status', $status);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getMigrationStatus() {
   +    $status = \Drupal::state()->get('migrate.' . $this->id() . '.status');
   +    if (is_null($status)) {
   +      $status = MigrationInterface::STATUS_IDLE;
   +    }
   +    return $status;
   

   State is just a wrapper around the key value store. I think we decided against it so we could a few things:

   * Allows us to retrieve or delete multiple entries at the same time.
   * Not have to build the unique ids for each storage like you have here.
   * Not have to check is_null on that result since keyValue->get() allows you to specify a default.

2. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -237,6 +237,15 @@ protected function getSource() {
   +    if ($this->migration->getMigrationStatus() != MigrationInterface::STATUS_IDLE) {
   

   Should we have a strict check here since STATUS_IDLE is 0 it seems easy to have false positives.

3. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -261,11 +271,17 @@ public function import() {
   +      if ($this->migration->getMigrationStatus() != MigrationInterface::STATUS_IMPORTING) {
   

   Same here.

4. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -237,6 +237,15 @@ protected function getSource() {
   +      $this->message->display(
   +        $this->t('Migration @id is busy with another operation', array(
   

   These should be on the same line, just the array values returned to a newline.

Well using the KeyValue store we also use a unique Id, apart from it's only unique within the migration_result kv store.

I'll take a look at that test later today.

Not pretty but something like the attached should do it.

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -238,9 +238,8 @@
+    if ($this->migration->getMigrationStatus() !== MigrationInterface::STATUS_IDLE) {
+      $this->message->display($this->t('Migration @id is busy with another operation', array(
           '@id' => $this->migration->id(),

Would it be worth outputting the migration status it is busy with?

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -238,9 +239,11 @@
+          '@status' => Migration::getTranslatedStatus($status),

I'm not entirely sure about translations but I don't quite understand why this couldn't be something like $this->migration->getStatusLabel().

Then getStatusLabel() could simply be return $this->t($this->getMigrationStatus());

I think i'm missing why it needs to be a static method and available at any time?

1. +++ b/core/modules/migrate/src/Entity/Migration.php
   @@ -15,6 +15,7 @@
   +use Drupal\Core\StringTranslation\TranslationWrapper;
   

   No longer needed.

2. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -237,6 +238,16 @@ protected function getSource() {
   +          '@status' => $this->t($this->migration->getStatusLabel()),
   

   @status is already used within a $this->t() so I don't know we need to run the status through t() on it's own.

   Sorry, I may have implied that in my last comment.

While it is correct that nothing in this code translates the status label and so getStatusLabel should be $this->t('Stopping') but why do we have this function on the entity???? I'd expect the entity to have a statusLabel field and this is the getter for it. Weird! Why is this not on MigrateExecutable?

+++ b/core/modules/migrate/src/Entity/Migration.php
@@ -214,6 +214,26 @@ class Migration extends ConfigEntityBase implements MigrationInterface, Requirem
+  public function getStatusLabel() {

@@ -372,6 +392,22 @@ protected function getEntityManager() {
+  public function setMigrationStatus($status) {

I think the naming of getStatusLabel() and getMigrationStatus()? should be consistent.

@chx, are you suggesting the getStatusLabel method be on the executable?

1. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -468,33 +488,15 @@ public function saveQueuedMessages() {
   -    /*
   -     * @TODO uncomment this
   -    if ($this->getStatus() == MigrationInterface::STATUS_STOPPING) {
   -      return MigrationBase::RESULT_STOPPED;
   -    }
   -    */
   -    // If feedback is requested, produce a progress message at the proper time
   -    /*
   -     * @TODO uncomment this
   -    if (isset($this->feedback)) {
   -      if (($this->feedback_unit == 'seconds' && time() - $this->lastfeedback >= $this->feedback) ||
   -          ($this->feedback_unit == 'items' && $this->processed_since_feedback >= $this->feedback)) {
   -        $this->progressMessage(MigrationInterface::RESULT_INCOMPLETE);
   -      }
   -    }
   -    */
   

   So we don't need this anymore? Is there an issue to bring it back somewhere?

2. +++ b/core/modules/migrate/src/Tests/MigrateStatusTest.php
   @@ -0,0 +1,62 @@
   +    $migration = entity_create('migration', array());
   +    $migration->set('id', 'migration_status_test');
   +    $migration->set('migration_groups', array('Testing'));
   +    $migration->set('source', array('plugin' => 'empty'));
   +    $migration->set('destination', array(
   +      'plugin' => 'config',
   +      'config_name' => 'migrate_test.settings',
   +    ));
   

   Not to worried, but did you know you can pass all these values to the second param of entity_create()

I think this is ready other than my question in point 1. Nice work.

I don't think it's preferred I just wondered if you was aware. Everything else looks good.

1. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -468,33 +488,15 @@ public function saveQueuedMessages() {
      /**
   -   * Checks for exceptional conditions, and display feedback.
   -   *
   -   * Standard top-of-loop stuff, common between rollback and import.
   +   * Checks for exceeded resource thresholds.
       */
   -  protected function checkStatus() {
   +  protected function checkThresholds() {
   

   This should detail its return value.

2. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -468,33 +488,15 @@ public function saveQueuedMessages() {
        return MigrationInterface::RESULT_COMPLETED;
   

   Does returning this from a method called checkThresholds() make sense?

+++ b/core/modules/migrate/src/Entity/Migration.php
   /**
+   * Labels corresponding to each defined status.
+   *
+   * @var array
+   */
+  protected $statusLabels = [
+    self::STATUS_IDLE => 'Idle',
+    self::STATUS_IMPORTING => 'Importing',
+    self::STATUS_ROLLING_BACK => 'Rolling back',
+    self::STATUS_STOPPING => 'Stopping',
+    self::STATUS_DISABLED => 'Disabled',
+  ];

Ideally these should be translated. Perhaps getStatusLabel() should do that?

+++ b/core/modules/migrate/src/Entity/Migration.php
+  public function setStatus($status) {
+    $migrate_status_store = \Drupal::keyValue('migrate_status');
+    $migrate_status_store->set($this->id(), $status);
+  }

Nit: can this be one line? \Drupal::keyValue()->set()

+++ b/core/modules/migrate/src/Entity/Migration.php
+  /**
+   * {@inheritdoc}
+   */
+  public function getStatus() {
+    $migrate_status_store = \Drupal::keyValue('migrate_status');
+    return $migrate_status_store->get($this->id(), static::STATUS_IDLE);
+  }

Ditto.

+++ b/core/modules/migrate/src/Entity/Migration.php
+  /**
+   * {@inheritdoc}
+   */
+  public function getStatusLabel() {
+    if (isset($this->statusLabels[$this->getStatus()])) {
+      return $this->statusLabels[$this->getStatus()];
+    }
+    else {
+      return '';
+    }
+  }

Micro-optimization -- let's call getStatus() once here to save the overhead.

+++ b/core/modules/migrate/src/Entity/Migration.php
     $migrate_result_store = \Drupal::keyValue('migrate_result');
     $migrate_result_store->set($this->id(), $result);

Can this also be converted to one line?

+++ b/core/modules/migrate/src/Tests/MigrateStatusTest.php
+    $status_list = array(
+      MigrationInterface::STATUS_IDLE,
+      MigrationInterface::STATUS_IMPORTING,
+      MigrationInterface::STATUS_ROLLING_BACK,
+      MigrationInterface::STATUS_STOPPING,
+      MigrationInterface::STATUS_DISABLED,
+    );
+    foreach ($status_list as $status) {
+      $migration->setStatus($status);
+      $this->assertIdentical($migration->getStatus(), $status);
+    }

Why bother testing every individual status? Surely it's enough to just test one and be done with it?

+++ b/core/modules/migrate/tests/src/Unit/MigrateTestCase.php
+    $migration->expects($this->any())
+      ->method('getStatus')
+      ->will($this->returnCallback(function() {
+        return $this->migrationStatus;
+      }));

For readability, can we use $migration->method('getStatus')->willReturnCallback(function() { ... }) instead?

+++ b/core/modules/migrate/tests/src/Unit/MigrateTestCase.php
   /**
+   * @var \Drupal\migrate\Entity\MigrationInterface::STATUS_*
+   */
+  protected $migrationStatus = MigrationInterface::STATUS_IDLE;

It's not clear how this is to be used in the context of the test. Can you add a bit of documentation here explaining it?

Beyond these minor things, this seems good to go.

Works for me. Thank you for accommodating my OCD-ness.

Committed and pushed to 8.0.x. Thanks!