Rollback should not delete uid 1

Um, yeah, not good... The stuff for handling uid 1 should be setting the rollback_action to to MigrateIdMapInterface::ROLLBACK_PRESERVE.

There is no means of restoring previous values (because they're not preserved anywhere), it's the same deal as simple configuration.

D'oh! I set the rollback flag to PRESERVE and it still got deleted, so the ability to preserve seems to be broken...

We need to fix #2599152: ROLLBACK_PRESERVE is not respected first.

OK, preservation in general is committed, now to fix uid 1. The test is more challenging that expected...

Definitely something we need to fix in RC.

My challenge was installing extra stuff in setUp() that the rollback touched, actual test code is straightforward. This fail patch demonstrates the issue, now on to fixing it. Which is going to be interesting, because the right fix is broader than the uid 1 case, we want to set ROLLBACK_PRESERVE generally when we're updating something that already existed pre-migration on the destination side, but need to be sure we don't set it when we're updating something we previously migrated...

The fix is *almost* very simple - I have straightforward code in the destination updateEntity() which identifies what rollback action to set. The problem, however, is what I want to set is $rollbackAction in the MigrateExecutable class, which is conveniently public, but the destination doesn't have access to that class (and I don't really want it to, my goal is *less* coupling: #2543568: Remove the md_entity destination plugin hack). Flirting with putting the rollbackAction value in the Migration class, where both the executable and the destination can access it. Or, maybe just put it in the destination class and the executable can query it from there? Hmm....

Here we go. Note:

1. The original fail patch proved nothing, because uid 1 did not exist in the test database. New test attached. Is calling user_install() in setUp() kosher?
2. The rollback action is only relevant for successfully imported items. so I removed it from saveIdMapping() calls on failures.
3. We should have more general testing than just the uid 1 case.

Actually, what was needed was for the preservation test to not be digging behind the scenes to set up the preservation scenario, but to let it happen naturally (by pre-creating an entity that would be migrated over and making sure it didn't get deleted on rollback).

Ready for manual review once the automated tests pass.

Oops, noticed the rollback action was not initialized for config entities...

Overall, this looks quite sensible to me.

1. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -65,13 +65,6 @@ class MigrateExecutable implements MigrateExecutableInterface {
   -   * The rollback action to be saved for the current row.
   -   *
   -   * @var int
   -   */
   -  public $rollbackAction;
   ...
   -        $this->migration->getIdMap()->saveIdMapping($row, array(), $e->getStatus(), $this->rollbackAction);
   +        $this->migration->getIdMap()->saveIdMapping($row, array(), $e->getStatus());
   ...
   

   Nice cleanup of a property that doesn't appear used anywhere in HEAD, but is it strictly related to this issue? Would it make sense to split it into a separate issue for a cleaner commit history?

2. +++ b/core/modules/migrate/src/Plugin/MigrateDestinationInterface.php
   @@ -80,7 +80,20 @@ public function import(Row $row, array $old_destination_id_values = array());
   +   * After import() is called, the appropriate rollback action for the last
   +   * imported item.
   

   Can be shortened to one line. For example: "The rollback action for the last imported item.".

3. +++ b/core/modules/migrate/src/Tests/MigrateRollbackTest.php
   @@ -111,15 +111,14 @@ public function testRollback() {
   +    // Pre-create a term, to make sure it isn't deleted on rollback.
   +    $preserved_term_id = 1;
   +    $new_term = Term::create(['tid' => $preserved_term_id, 'vid' => 1, 'name' => 'music']);
   +    $new_term->save();
   

   Lovely!

4. +++ b/core/modules/migrate/src/Tests/MigrateRollbackTest.php
   @@ -111,15 +111,14 @@ public function testRollback() {
   -    // Mark one row to be preserved on rollback.
   -    $preserved_term_id = 2;
   -    $map_row = $term_id_map->getRowBySource(['id' => $preserved_term_id]);
   -    $dummy_row = new Row(['id' => $preserved_term_id], $ids);
   -    $term_id_map->saveIdMapping($dummy_row, [$map_row['destid1']],
   -      $map_row['source_row_status'], MigrateIdMapInterface::ROLLBACK_PRESERVE);
   

   Are we sure we want to delete this test though? Are there any valid use cases for this kind of manual setting that still warrants testing?

5. +++ b/core/modules/user/src/Tests/Migrate/d6/MigrateUserTest.php
   @@ -29,6 +30,10 @@ protected function setUp() {
   +    $this->installEntitySchema('node');
   

   Why 'node'? Should it be 'user'? And if so, how are tests passing without the user entity schema installed?

6. +++ b/core/modules/user/src/Tests/Migrate/d6/MigrateUserTest.php
   @@ -117,6 +122,25 @@ public function testUser() {
   +        // assertNotNull()'s call to var_export() is unhappy with recursiveness
   +        // of the account object.
   +        $this->assertFalse(is_null($account));
   

   The var_export() is only called if you don't pass in an assertion message. So this can be something like $this->assertNotNull($account, 'User 1 was preserved after a migration rollback');.

Nice cleanup of a property that doesn't appear used anywhere in HEAD, but is it strictly related to this issue? Would it make sense to split it into a separate issue for a cleaner commit history?

Yes, it is strictly related - this property is being moved from the executable class (where it was never set) to the destination plugin which is the natural place to set it.

Can be shortened to one line. For example: "The rollback action for the last imported item.".

Done.

Are we sure we want to delete this test though? Are there any valid use cases for this kind of manual setting that still warrants testing?

Well, this segment of code was just added in #2599152: ROLLBACK_PRESERVE is not respected and needed to take a hacky approach to setting up the preservation scenario due to the bug in the current issue, so it's natural to replace it with a direct test of this issue. But, as you suggest, the saveIdMapping() could be used directly, so let's test that too.

Why 'node'? Should it be 'user'? And if so, how are tests passing without the user entity schema installed?

User schema is already installed in MigrateDrupalTestBase. This is here because deleting users invokes node_user_cancel(), which does an entityQuery() on the node table (in case they might have owned any nodes).

The var_export() is only called if you don't pass in an assertion message. So this can be something like $this->assertNotNull($account, 'User 1 was preserved after a migration rollback');.

Done.

Thanks for the changes and explanations. #18 looks great to me. I didn't manually test though, so not setting to RTBC. But if someone does confirm that this works as expected in manual testing, I think it can be RTBC'd.

Worked OK for me!

There was some concern expressed in IRC by chx that this might add some unnecessary coupling. However, I think the problem here is suitably bad enough that we should get this in now and then open a follow-up when/if a better approach is discovered.

Committed and pushed to 8.0.x. Thanks!

That was not what we agreed on: we agreed to immediately open a major followup because this is not the right fix. I even wanted to keep this issue open as the patch committed is an architecturally broken stopgap. https://www.drupal.org/node/2603120

Correct, but as noted in IRC, I have no idea what the actual problem is with this patch (and the new issue doesn't explain it, either) so didn't feel in a position to do so myself.

The problem is encapsulation violation. That's theory. In practice, "sourceid1" is an Sql idmap detail which will immediately break when you have a non sql idmap.