Add a rollback functionality to migrate

I used https://www.drupal.org/project/backup_migrate in D7 for rollbacks :) May be enough to create a backend auto interface if that module is installed :)

OK, picking up some of dawehner's work, this patch works to rollback entities (testing with the beer example in migrate_plus). Should we fill in rollback operations for all destinations in this patch or (my preference) get the basic infrastructure and entity rollback in here, and add some novice tasks to fill in the rest?

I'll note that I've changed rollbackMultiple() to rollback() - batch deletion is of limited value given entities in particular don't have bulk deletion in core.

I've not reviewed this yet but i'd agree with just having support for the generic entity destination and then creating follow-ups for the other destinations.

How do I even test this?

Where do I contribute the "drush migrate-rollback" command? Or am I thinking about it all wrong.

I have written a non-Drupal migrate module for D8 and am able to execute it using "drush migrate-manifest ..."

That said, I presume those wishing to execute a rollback for a d6_node migration are wondering similar things?

@krlucas: The Migrate Plus project is where the full drush commands for migration are being done. #2403411: Implement migrate-rollback command for D8 is the issue for implementing drush migrate-rollback once the API in this issue has been committed to core.

Time to reroll!

A reroll of #5.

This is critical for developing migrations.

With event goodness - still needs tests though. Works with latest migrate_plus patch at #2403411: Implement migrate-rollback command for D8.

I see several minor issues...

1. +++ b/core/modules/migrate/src/Event/MigrateEvents.php
   @@ -108,4 +110,64 @@
   +   * This event allows modules to perform an action whenever a migration rollback
   

   Nit: more than 80 characters. =P

2. +++ b/core/modules/migrate/src/Event/MigrateRowDeleteEvent.php
   @@ -0,0 +1,65 @@
   +   * @param array|bool $destination_id_values
   +   *   Values represent the destination ID.
   

   Why can this be an array or boolean? More explanation would be good here. The use of the word "values" in the parameter name implies an array.

3. +++ b/core/modules/migrate/src/Event/MigrateRowDeleteEvent.php
   @@ -0,0 +1,65 @@
   +   *   The migration entity being rolled back.
   

   Nit, but I think it's clearer to say "the migration being rolled back", dropping the word "entity".

4. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -310,6 +303,49 @@ public function import() {
   +    // Knock off rollback operation if the requirements haven't been met.
   +    // @todo: We should have a reverse requirements check here - a migration
   +    // shouldn't be rolled back until the migrations dependent on it have been
   +    // rolled back.
   +    /* try {
   +         $this->migration->checkRequirements();
   +       }
   +       catch (RequirementsException $e) {
   +         $this->message->display(
   +         $this->t('Migration @id did not meet the requirements. @message @requirements', array(
   +           '@id' => $this->migration->id(),
   +           '@message' => $e->getMessage(),
   +           '@requirements' => $e->getRequirementsString(),
   +         )), 'error');
   +         return MigrationInterface::RESULT_FAILED;
   +       }*/
   

   If this is going to be completely commented out until we properly implement the reverse requirement check, IMO this should not be here now.

5. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -310,6 +303,49 @@ public function import() {
   +    $return = MigrationInterface::RESULT_COMPLETED;
   

   $return is never changed. Why don't we just return MigrationInterface::RESULT_COMPLETED directly?

6. +++ b/core/modules/migrate/src/Plugin/MigrateDestinationInterface.php
   @@ -72,11 +72,11 @@ public function fields(MigrationInterface $migration = NULL);
   +   *   The key of the destination object to delete.
   

   "Key of the destination object"? Not sure I understand this. Can it be better-explained in the doc comment?

7. +++ b/core/modules/migrate/src/Plugin/MigrateIdMapInterface.php
   @@ -196,7 +196,7 @@ public function getRowsNeedingUpdate($count);
   @@ -210,6 +210,14 @@ public function lookupSourceID(array $destination_id_values);
   

   Nit: ID is capitalized here, but it's Id everywhere else?

8. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Config.php
   @@ -86,12 +86,12 @@ public function import(Row $row, array $old_destination_id_values = array()) {
   +   * @param array $destination_key
   ...
   +  public function rollback(array $destination_keys) {
   

   The parameter name in the doc comment does not match the actual parameter name :)

As it happens, I'm revisiting this today...

Updated patch to feature parity with import (e.g., managing idle/rolling-back status). Had hoped to do tests today, but didn't get to them...

Now with tests! Ready for human review once the bots have their say...

I think this is looking pretty good, mainly a few style issues and some questions.

1. +++ b/core/modules/migrate/src/Event/MigrateEvents.php
   @@ -108,4 +110,64 @@
   +   *
   +   * @Event
   +   *
   +   * @see \Drupal\migrate\Event\MigrateRollbackEvent
   +   *
   +   * @var string
   

   The spacing and order of these seems weird. Is this inline with what core does? Ignore me if it is.

2. +++ b/core/modules/migrate/src/Event/MigrateEvents.php
   @@ -108,4 +110,64 @@
   +   * @see \Drupal\migrate\Event\MigrateImportEvent
   ...
   +  const POST_ROLLBACK = 'migrate.post_rollback';
   

   Incorrect event class.

3. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -88,15 +90,6 @@ class MigrateExecutable implements MigrateExecutableInterface {
   -  protected $rollbackBatchSize = 50;
   

   Is it all or nothing in core?

4. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -334,6 +327,62 @@ public function import() {
   +      $this->message->display(
   +        $this->t('Migration @id is busy with another operation: @status',
   +        [
   

   I think this could all be on one line :)

5. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -334,6 +327,62 @@ public function import() {
   +    $this->getEventDispatcher()->dispatch(MigrateEvents::PRE_ROLLBACK, new MigrateRollbackEvent($this->migration));
   ...
   +        $this->getEventDispatcher()
   +             ->dispatch(MigrateEvents::PRE_ROW_DELETE, new MigrateRowDeleteEvent($this->migration, $destination_key));
   ...
   +        $this->getEventDispatcher()
   +             ->dispatch(MigrateEvents::POST_ROW_DELETE, new MigrateRowDeleteEvent($this->migration, $destination_key));
   

   Inconsistent formatting.

6. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -334,6 +327,62 @@ public function import() {
   +    $id_map = $this->migration->getIdMap();
   ...
   +        $id_map->delete(unserialize($serialized_key));
   

   I'm not a big fan of the serialized array so we can use it as a key. Maybe not here but it would be nice if could get the keys another way.

7. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -334,6 +327,62 @@ public function import() {
   +      // Check for memory exhaustion.
   +      if (($return = $this->checkStatus()) != MigrationInterface::RESULT_COMPLETED) {
   +        break;
   +      }
   +
   +      // If anyone has requested we stop, return the requested result.
   +      if ($this->migration->getStatus() == MigrationInterface::STATUS_STOPPING) {
   +        $return = $this->migration->getMigrationResult();
   +        break;
   +      }
   

   This is something we do in import as well, wonder if it's worth its own method.

8. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -334,6 +327,62 @@ public function import() {
   +    if ($return == MigrationInterface::RESULT_COMPLETED) {
   

   $this->migration->isComplete()

The spacing and order of these seems weird. Is this inline with what core does? Ignore me if it is.

Ignoring ;-). See ConfigEvents for an example (the one I originally copy-and-pasted from).

Incorrect event class.

Fixed.

Is it all or nothing in core?

It's one-at-a-time in core. $rollbackBatchSize was a relic of D7 migrate, used to determine how big a batch to pass to deleteMultiple(), but D8 entities only have delete() so no batching.

I think this could all be on one line :)

Sigh, ok. https://www.drupal.org/coding-standards#linelength obviously means nothing in D8...

Inconsistent formatting.

Fixed.

I'm not a big fan of the serialized array so we can use it as a key. Maybe not here but it would be nice if could get the keys another way.

Definitely out-of-scope here.

This is something we do in import as well, wonder if it's worth its own method.

When/if #2545632: [PP1] Move memory reclamation out of migrate executable is done, the memory handling should use interruptMigration() and the checkStatus() part here will go away.

$this->migration->isComplete()

Nope. For one thing, $return hasn't yet been passed to setMigrationResult(). For another, basing isComplete() on the result/status is wrong: #2554003: isComplete() should not rely on RESULT_COMPLETED

Thanks!

Hrrrm, retest did not reset the status...

Sigh, ok. https://www.drupal.org/coding-standards#linelength obviously means nothing in D8...

How I usually format long lines with an array is like below, passes phpcs.

  $this->message->display($this->t('Migration @id is busy with another operation: @status', [
    '@id' => $this->migration->id(), '@status' => $this->t($this->migration->getStatusLabel()),
  ]), 'error');

If you set the result a few lines earlier then isComplete() would work? Thanks for pointing me to the other issue, i'll leave some thoughts over there as well.

How I usually format long lines with an array is like below, passes phpcs.

Should I reroll?

If you set the result a few lines earlier then isComplete() would work? Thanks for pointing me to the other issue, i'll leave some thoughts over there as well.

isComplete() is still wrong. Wrong, wrong, wrong!

I've commented on the other issue about the result / isComplete DX.

It may be "wrong" from a technical sense but I think it's more poor naming, isComplete doesn't check RESULT_COMPLETE, now that's wrong!

Commented over there, I'm fine with renaming isComplete().

But, can I get an RTBC on this issue? What isComplete() (under any name) is meant to do is irrelevant to the RESULT_COMPLETED check here.

But, can I get an RTBC on this issue?

Sure!

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -88,15 +90,6 @@ class MigrateExecutable implements MigrateExecutableInterface {
-  protected $rollbackBatchSize = 50;

Asked about this. This is a holdover from D7, where we had the ability to delete multiple, and batch them. This was removed in D8.

Walked through this with @mikeryan and @phenaproxima, all looks good to me. The API's clear, well-documented (we added a few more comments to MigrateExecutable::rollback() as we went through), and useful functionality to have in core.

Committed and pushed to 8.0.x. Thanks! :D

Do we know which part of this issue broke that? We definitely need a follow-up to fix that, keeping Ids is pretty essential to the upgrade path.

I discussed some with dawehner in #drupal-contribute, and asked for a distinct issue with steps to reproduce. He said it broke for him after RC...3? I can't see how the rollback patch would be involved, it didn't touch getEntity() (the stub patch would be more likely). IDs are preserved for me in a straightforward D6->D8 upgrade. I should have asked more about what "custom update code" meant... Not clear to me if this was reimporting already-migrated content, or running a migration intended to update existing content (not from migration).