Migrate rollback does not rollback failed items

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.

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!

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.

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

Actually, bug fixes should go against 8.1.x.

We still need tests here.

How was this never set to major?

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?

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

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.

+++ 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

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.

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.

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.

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

Were you thinking about something like this?

Alrighty, thanks!

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.