Problem/Motivation

Rolling back a migration that had failed items doesn't actually revert the failed migrated items. I tried migrating a CSV into a node type with a missing title. Obviously this failed. But then rollback also failed to roll-back the failed items.

Truncating the migrate_map_foo table resolves things as a manual work around.

Proposed resolution

Looks like currentDestination() returns array(NULL) in that case. This needs to change.

Remaining tasks

User interface changes

API changes

Data model changes

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

heddn created an issue. See original summary.

heddn’s picture

Issue summary: View changes
mikeryan’s picture

Note: looks to me like the fault is in currentDestination(), returning array(NULL) for failed rows, which results in the attempt to delete entities with an id of NULL.

drclaw’s picture

Here's a patch to get the ball rolling on this one. I'm not 100% sure if this is the right approach but I figured I'd take a stab at it. Mostly I'm worried about:

  1. Adding a new method to \Drupal\migrate\Plugin\MigrateIdMapInterface will break any existing contrib implementations (may not be an issue while still in rc2...?) - this could be avoided if there is an existing way to get the current row source values; I just couldn't find one.
  2. We're of relying on implementations of currentDestination() to return NULL if all the destination keys are NULL (instead an array of destination_key => NULL like it was before) which might be an issue for any code using it and expecting an array. Although, the method doc does indicate that NULL is a possible return value...

Anyway, I didn't write any tests yet. I thought I'd get some feedback on the approach first.

Thanks!

mikeryan’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Yes, I think that's the right approach - only logical to have a currentSource() call.

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -341,6 +342,11 @@ public function rollback() {
         $id_map->deleteDestination($destination_key);
       }
+      else {
+        // No destination key, import probably failed, remove row
+        $source_key = $id_map->currentSource();
+        $id_map->delete($source_key);
+      }

Rather than separate delete calls, it would be simlper to just delete by the source key in all cases.

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

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review

Let's see how the last patch does with 8.2.x...

mikeryan’s picture

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

Actually, bug fixes should go against 8.1.x.

mikeryan’s picture

Status: Needs review » Needs work

We still need tests here.

mikeryan’s picture

Priority: Normal » Major

How was this never set to major?

Lendude’s picture

Ran into this, applied patch, "Rolled back 651 items - done", nice!

Bit of nitpicking while I'm here:

  1. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -341,6 +342,11 @@ public function rollback() {
    +      else {
    +        // No destination key, import probably failed, remove row
    

    This should be turned into an actual english sentence.

  2. +++ b/core/modules/migrate/src/Plugin/MigrateIdMapInterface.php
    @@ -224,6 +224,14 @@ public function lookupDestinationId(array $source_id_values);
    +  public function currentSource();
    

    This addition makes this an API change and so this should go to 8.2.x, or doesn't that count in experimental modules?

Lendude’s picture

Added a test and fixed nitpick. Leaving it at 8.1.x for now.

The last submitted patch, 12: migrate-rollback_failed_items-2579343-12-TEST_ONLY.patch, failed testing.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me!

I think this is OK for 8.1.x - the only BC break would be if someone out there were implementing their own id_map, which is an unlikely scenario.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/migrate/tests/src/Kernel/MigrateRollbackTest.php
@@ -128,6 +128,9 @@ public function testRollback() {
+    // Add a failed row to test if this can be rolled back without errors.
+    $this->mockFailure($term_migration, ['id' => '4', 'vocab' => '2', 'name' => 'FAIL']);

I really like this patch it adds a much needed method. However, this is the only test coverage added. It would be awesome to get test coverage of the new currentSource() method and the changes to currentDestination(). I would have expected additional coverage in \Drupal\Tests\migrate\Unit\MigrateSqlIdMapTest

alexpott’s picture

Issue tags: +Migrate critical

Also for me this is a migrate critical - failing to rollback failed items will break migrations and the work around of truncating the table is not good.

Lendude’s picture

Added test coverage for currentSource() and currentDestination(). Grouped the coverage together in one test method since they are basically returning two sides of the same data set. But if we want one test method per class method I can see the benefit in that too, *shrug*.

Also, I didn't find a better way to start it up then by calling $id_map->rewind();, which feels weird, but starting with next() gave a fatal. So if there is a better way to initialise all the needed variables, let me know.

mikeryan’s picture

Status: Needs review » Needs work

Much as I hate to set it back - the change in currentDestination() affects the case when a destid column is NULL, so that case needs to be tested.

Lendude’s picture

@mikeryan, no worries, more test coverage is always good.

Were you thinking about something like this?

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

Alrighty, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 51a76ef to 8.3.x and 2960609 to 8.2.x and 4408487 to 8.1.x. Thanks!

As a migrate critical this can go against all three branches because migrate is not yet out of alpha.

  • alexpott committed 51a76ef on 8.3.x
    Issue #2579343 by Lendude, drclaw, heddn, mikeryan: Migrate rollback...

  • alexpott committed 2960609 on 8.2.x
    Issue #2579343 by Lendude, drclaw, heddn, mikeryan: Migrate rollback...

  • alexpott committed 4408487 on 8.1.x
    Issue #2579343 by Lendude, drclaw, heddn, mikeryan: Migrate rollback...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.