Add back rollbacks on migrate_drupal_ui

This is postponed on #2687843: Add back incremental migrations through the UI, which has differing opinions on whether it should be implemented or not. In light of that on going discussion, and if rollback is still wanted in the UI, here is a patch for the rollback action. This includes a new confirmation message for both the upgrade and rollback actions.

Also, the proposed resolution includes 'Consider refactoring the form and batch process so the logic for it is not entangled with the initial migration and the rollback option.' Note that there are two existing issues pertaining to that #2687851: Refactor run() method on Migrate UI batch and remove the $operation parameter and #2679929: Reduce method and control structure length/complexity in Migrate UI.

Updated the test to use the new text.

Had a chat with alexpott on IRC. The short version is that we agree that rollback is useful. It provides an 'undo' function which is something many expect to have in software. And that means they can then 'undo' instead of re-install to fix simple things like installing or un-installing a module.

What do you think? Time to review this?

Here is the full conversation:
quietone: re the UI and rollback - I really don't know. This is one tricky area
quietone: when talking to other migrate maintainers it seems as though the migrate UI in core is viewed as a one chance casino
alexpott, I was wondering if some would prefer a full rollback and try again, say after install some modules, instead of resinstalling and trying again
s/install/installing/
quietone: that is a possible use-case I guess - but i'd prefer people to re-install. Cleaner. But OTOH this is incremental-lite :D
quietone: that use-case is probably the best use-case for an "incremental" ish thing in the UI. - Something that only allows you to run new migrations that are available.
quietone: But the idea of only have a "do it" and "undo it" button in the migrate UI might be a really good idea.
alexpott, do it / undo it. Ah, that 'feels' right.
quietone: so the workflow is install core & a few modules... run migrate - think nope - rollback - install or uninstall modules - migrate - thing yay!
quietone: feel free to copy this IRC chat into the issue
alexpott, yes, I think that fits for those who will be using the UI.

@xjm, as alexpott said "But the idea of only have a "do it" and "undo it" button in the migrate UI might be a really good idea." I think that is the reason, it gives those working just from the UI a way to 'Undo'. And it means they don't have to re-enter the database credentials and file path information.

Patch from #5 won't apply.

Checking patch core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php...
Hunk #2 succeeded at 745 (offset 24 lines).
Hunk #3 succeeded at 807 (offset 24 lines).
Hunk #4 succeeded at 928 (offset 24 lines).
Hunk #5 succeeded at 1012 (offset 24 lines).
Hunk #6 succeeded at 1026 (offset 24 lines).
Hunk #7 succeeded at 1131 (offset 24 lines).
Hunk #8 succeeded at 1208 (offset 24 lines).
Hunk #9 succeeded at 1227 (offset 24 lines).
Checking patch core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php...
Hunk #1 succeeded at 61 (offset 2 lines).
Hunk #2 succeeded at 127 (offset 1 line).
error: while searching for:
              $context['sandbox']['num_processed'], 'Upgraded @migration (processed 1 item total)', 'Upgraded @migration (processed @num_processed items total)',
              ['@migration' => $migration_name, '@num_processed' => $context['sandbox']['num_processed']]);
          }
          $context['sandbox']['messages'][] = $message;
          static::logger()->notice($message);
          $context['sandbox']['num_processed'] = 0;

error: patch failed: core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php:141
error: core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php: patch does not apply
Checking patch core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php...

Rerolled against 8.2.x branch.

I needed to reroll this, patch attached.

With this patch, I was not able to run an import in the first place - clicking Continue on the overview form simply reloaded it without proceeding to the credentials form (not clear on why that is).

1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -21,6 +21,16 @@ class MigrateUpgradeForm extends ConfirmFormBase {
      /**
   +   * If a migration has previously run, perform an incremental migration.
   +   */
   +  const MIGRATE_UPGRADE_INCREMENTAL = 1;
   

   Incremental upgrades are not being added, at least in this patch, so no point in adding this constant.

2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -735,10 +745,19 @@ public function buildOverviewForm(array $form, FormStateInterface $form_state) {
   +      $form['upgrade_option'] = array(
   +        '#type' => 'radios',
   +        '#title' => $this->t('You can rollback the upgrade.'),
   +        '#default_value' => static::MIGRATE_UPGRADE_ROLLBACK,
   +        '#options' => [
   +          static::MIGRATE_UPGRADE_ROLLBACK => $this->t('<strong>Rollback</strong>: Remove content and configuration entities (such as fields and node types). Default values of other configuration will not be reverted (such as site name).'),
   +        ],
   +      );
   

   There's not much point to a radio select with only one option. This would probably be better as just a simple value with a description making it clear that continuing will perform a rollback.

working on it.

Had mentioned about this issue in the weekly migration call, according to the priority this will be taking care of end of July 2017.
However worked on roll back against 8.4.x, which is attached.
To do,

1. [#12] has to addressed
2. /core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php No more existing, So that IMHO the following code has to moved to /core/modules/migrate_drupal_ui/src/Batch/MigrateUpgradeImportBatch.php
   

   /**
   diff --git a/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php b/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php
   index 2a271fb..e96d1b2 100644
   --- a/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php
   +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php
   @@ -61,7 +61,7 @@ class MigrateUpgradeRunBatch {
       * @param int[] $initial_ids
       *   The full set of migration IDs to import.
       * @param string $operation
   -   *   The operation to perform. Only 'import' is currently supported.
   +   *   The operation to perform, 'import' or 'rollback'.
       * @param array $config
       *   An array of additional configuration from the form.
       * @param array $context
   @@ -127,6 +127,9 @@ public static function run($initial_ids, $operation, $config, &$context) {
            if ($operation == 'import') {
              $migration_status = $executable->import();
            }
   +        else {
   +          $migration_status = $executable->rollback();
   +        }
          }
          catch (\Exception $e) {
            static::logger()->error($e->getMessage());
   @@ -142,6 +145,11 @@ public static function run($initial_ids, $operation, $config, &$context) {
                  $context['sandbox']['num_processed'], 'Upgraded @migration (processed 1 item total)', 'Upgraded @migration (processed @count items total)',
                  ['@migration' => $migration_name]);
              }
   +          else {
   +            $message = static::getTranslation()->formatPlural(
   +              $context['sandbox']['num_processed'], 'Rolled back @migration (processed 1 item total)', 'Rolled back @migration (processed @num_processed items total)',
   +              ['@migration' => $migration_name, '@num_processed' => $context['sandbox']['num_processed']]);
   +          }
              $context['sandbox']['messages'][] = (string) $message;
              static::logger()->notice($message);
              $context['sandbox']['num_processed'] = 0;
   @@ -210,6 +218,13 @@ public static function run($initial_ids, $operation, $config, &$context) {
                '@max' => $context['sandbox']['max'],
              ]) . "<br />\n" . $context['message'];
            }
   +        else {
   +          $context['message'] = t('Currently rolling back @migration (@current of @max total tasks)', [
   +              '@migration' => $migration_name,
   +              '@current' => $context['sandbox']['current'],
   +              '@max' => $context['sandbox']['max'],
   +            ]) . "<br />\n" . $context['message'];
   +        }
          }
        }
        else {
   @@ -252,6 +267,9 @@ protected static function displayResults($results) {
          if ($results['operation'] == 'import') {
            drupal_set_message(new PluralTranslatableMarkup($successes, 'Completed 1 upgrade task successfully', 'Completed @count upgrade tasks successfully'));
          }
   +      else {
   +        drupal_set_message(static::getTranslation()->formatPlural($successes, 'Completed 1 rollback task successfully', 'Completed @count rollback tasks successfully'));
   +      }
        }
    
        // If we had failures, log them and show the migration failed.
   @@ -260,6 +278,10 @@ protected static function displayResults($results) {
            drupal_set_message(new PluralTranslatableMarkup($failures, '1 upgrade failed', '@count upgrades failed'));
            drupal_set_message(new TranslatableMarkup('Upgrade process not completed'), 'error');
          }
   +      else {
   +        drupal_set_message(static::getTranslation()->formatPlural($failures, '1 rollback failed', '@count rollbacks failed'));
   +        drupal_set_message(t('Rollback process not completed'), 'error');
   +      }
        }
        else {
          if ($results['operation'] == 'import') {
   @@ -267,6 +289,9 @@ protected static function displayResults($results) {
            // but we didn't have failures so this is fine.
            drupal_set_message(new TranslatableMarkup('Congratulations, you upgraded Drupal!'));
          }
   +      else {
   +        drupal_set_message(t('Rollback of the upgrade is complete - you may now start the upgrade process from scratch.'));
   +      }
        }
    

Moving to 8.4.x and reducing to Normal - this is not a requirement for the migration system being declared "stable", which is our goal for 8.4.0, this is new functionality we would target for post-stability.

Updated the IS to include adding documentation to the handbook. Aside from being a nice idea in general, this is a to do item on #2561249: [Meta] Reorganization of (some) d.o. migration documentation, see #11.

Rerolling patch.

Fixing consts misplacing.

Added ./core/modules/migrate_drupal_ui/src/Batch/MigrateUpgradeRollbackBatch.php + code standard fixes.

This could use some documenting. I'm also going to pend it on #2918761: Break up MigrateUpgradeForm into smaller forms.

No longer postponed, and will need a reroll.

It was reverted, back to postponed.

1. I created a documentation follow-up and updated the issue summary accordingly, see #2947498: Document Migrate Drupal UI rollbacks.

2. Regarding the UI for this. The original screenshot what this used to look like back in the days said 'Rerun'. I propose we use the term 'Incremental upgrade' consistently.

Cheers,
Markus

32.2 I agree and have updated the IS. We can add a UX review when this is ready to be sure, but it is certainly better than 'rerun'.

#2918761: Break up MigrateUpgradeForm into smaller forms has been committed.

Assiging for reroll

Setting to NR for testbot

This is on the roadmap for 8.6, so let's bump priority.

Nice to see progress here. Tagging for needing a test.

Took a brief look at the patch #28 and it looks like a base class for the Batch would be useful.

Un assigining myself, for time being some one wants to pik it up..

Let's bring this up-to-date.

This far from complete. I've added a test, adding a rollback at the end of the existing full migration test. There is a new method, getEntityCountsRollback() to get the entity counts for the post rollback assertions but they are all wrong, it is just a copy/paste from getEntityCounts() because I haven't looked into the counts yet but I wanted to get the test built. Running the tests to see how bad this is.

There is no interdiff because I applied the two patches in #38 and started from there.

This addressed coding standard errors, a adjusted the assertion in FormStepTest for the new text on the Incremental form, and fixes and the problem where I hastily put getEntityCountsRollback in the wrong class.

Still very much a WIP and no need for review yet.

Edit: my fingers really like to name the interdiff wrong these days!

Only a bit of cleanup. Fixed the remaining coding standard errors and changed drupal_set_message to \Drupal::messenger().

Rerolling the patch.

This may fix two tests, the NoMultilingual ones.

I seem to be at an impasse and don't see how to fix this. Can anyone provide some direction.

Un-assigning myself since I can't see the problems here.

Retesting

This is failing on the page reload after the rollback finishes.

1) Drupal\Tests\migrate_drupal_ui\Functional\d6\MigrateUpgrade6Test::testMigrateUpgradeExecute
Exception: Error: Call to a member function url() on null
rdf_comment_storage_load()() (Line: 244)

Maybe this this related one or more of these #2978320: [backport] rdf comment storage load should not load NULL comments, #2614720: Fatal errors while loading/building orphaned comments or #2906470: Orphan comments and entries in comment_entity_statistics after comment field instance has been deleted ?

Retesting

Just a reroll

I seem to have this functionality already? Or is that because it is currently in migrate_tools or migrate_plus already (or for now)?

I haven't used migrate_tools in a long time but the core UI does not offer any type of rollback.

Another reroll

Add getEntityCountsRollback() to several tests, remove unnecessary validation from the IncrementalForm and get the migrations from the store for use in the rollback.

The current Upgrade6Test and Upgrade7Test logout the admin user and when I try to log back in as the root user it fails because the password has changed. So, instead of having to keep track of the password from the test fixture this patch makes new tests Rollback6Test and Rollback7Test. This patch also includes the latest patch from #3123095: Rollback of complete node migration fails so that the node complete migration actually does rollback the nodes.

While I am still not fond of testing the entity counts it is the best we have so entity counts are tested post rollback. The counts in the patch are not correct, and I expect failures.

Add the source_base_path for Rollback7Test and some cleanup.

Update entity counts.

Add related issue to comment, #38 over there.

The patch in needed a reroll and I am curious to see if adding #2614720: Fatal errors while loading/building orphaned comments will fix the error.

I'll make a proper rerolled patch after this test.

It appears that the #2614720: Fatal errors while loading/building orphaned comments does improve the outcome here but I can't be sure. I'll have to test locally.

Here is a reroll of the patch in #77

I am working on testcases

the fixing test case, let's wait for the boat.

@shaktik, thanks for the patch.

There's a typo.

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -306,7 +306,7 @@ public function rollback() {
+      if (issset($destination_key)) {

s/issset/isset/

sorry for typo error, fixed typo error.

@quietone, Thank you for quick check.

I am trying to fix but not completed still getting some issue in this method testMigrateUpgradeExecute, here is the link for the testing patch I tried https://www.drupal.org/project/drupal/issues/3164847#comment-13792232

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -306,7 +306,7 @@ public function rollback() {
       $destination_key = $id_map->currentDestination();
-      if ($destination_key) {
+      if (isset($destination_key)) {

MigrateIdMapInterface::currentDestination can return an array, including an empty array. So, instead of isset try !empty.

Added a patch as mentioned by #94.

@Vidushi Mehta, thank you for the patch. Can you provide an interdiff?

@quietone,

Thanks for the suggestion I also tried with !empty, but no luck.

Haha Bot's happy. @Vidushi Mehta thanks for the patch. but why you skipped all new files in the previous patch any Specific reason?

Thanks,
Shakti

@shaktik, thanks for pointing that out the lost files.

So, this needs another reroll, from #91, that adds the fix suggested in #94.

I also noticed that Rolback7Test is a copy/paste of Rollback6Test and that needs to be fixed.

1. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/Rollback6Test.php
   @@ -0,0 +1,102 @@
   +namespace Drupal\Tests\migrate_drupal_ui\Functional\d6;
   ...
   + * Tests Drupal 6 rollback using the migrate UI.
   ...
   +class Rollback6Test extends Upgrade6Test {
   

   s/6/7

2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/Rollback6Test.php
   @@ -0,0 +1,102 @@
   +  protected function assertFollowUpMigrationResults() {
   

   Replace this with the method of the same name from Upgrade7Test

Rerolled the patch and added the new files which was missed by me to add @shaktik sorry for that. I've added point 2 which was mentioned by @quietone but still point 1 is not covered as I didn't understand this point/change properly. @quietone & @shaktik it would be really helpful if you guys help me in this or @shaktik if you can cover this point.

@Vidushi Mehta, thanks. Sorry about the confusion. The string 's/6/7', means change 6 to 7 but that is moot because I made a mistake. I was looking at the wrong file so my #98.1 and #98.2 are wrong.

I have made a new patch to correct my mistake. Note this also includes a change in \Drupal\migrate\MigrateExecutable::rollback to check that $map_row is an array.

Rollback7Test is failing because of recent changes to the functional tests. This patch makes those changes, I hope (it is late).

The Rollback6Test is failing, https://www.drupal.org/pift-ci-job/1796503, on an rdf issue. For that the patch from #2978320: [backport] rdf comment storage load should not load NULL comments should be added. I am not doing that here because I want to see if there any other errors related to the changes in the functional tests. There shouldn't be any.

@quietone no issues reagrding your commnet on #101. But after all that changes still the Rollback6Test is failing.

Yes, the tests are failing. In #103 I said that the patch from #2978320: [backport] rdf comment storage load should not load NULL comments is needed. That still needs to be done. But even with that I expect it will fail the same was as the Drupal 7 test.

So, look at the test results for the Drupal 7 test and check out what it is failing on. That will give us an indication of the problem.

Are you setup to run these functional tests locally?

Retesting latest patch because #2978320: [backport] rdf comment storage load should not load NULL comments was committed.

Let's add a method for testing the rollback results. Modified drupalci.yml to just run the migrate_drupal_ui tests.

That is not what I meant to do. This should be better, though still a work in progress.

Better, now add a string to confirm the rollback and update entity counts.

Change some more entity_counts

And cleanup and fix more entity counts

Missed another entity count

Now restore drupalci.yml

Update the IS

+++ b/core/modules/migrate_drupal_ui/src/Form/IncrementalForm.php
@@ -78,16 +96,20 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+        static::MIGRATE_UPGRADE_INCREMENTAL => $this->t('<strong>Rerun</strong>: Import additional configuration and content that was not available when running the upgrade previously.'),

According the IS this should be 'Incremental upgrade' instead of 'Rerun'. Once that change is made a new screenshot needs to be added to the IS to replace the existing one of this page.

Making that change is suitable for a novice task, so tagging.

Made the changes requested in #121. Please review

@ayushmishra206, thanks. That looks correct. Can you also make a screenshot and update the Issue Summary?

Fixing test.

@vsujeetkumar, thanks for fixing the test.

Setting to NW for a screenshot of the changed page. That screenshot should be in the IS.

Added screenshots.

Probably could use a usability review of the screens in the IS. Tagging.

Did a self review. Found some items to cleanup and removed use of deprecated code.

Needs a reroll which I will finish tonight.

Rerolled and removed third parameter from a call to updateEntity that looks left over from an earlier design of this.

There are no changes that warrant creating new screen shots.

re-roll the patch.

@kishor_kolekar, can you add the interdiff? There are instructions for creating an interdiff in the handbook.

Creating the diff is suitable for a novice, adding tag.

Thanks, @quietone for your feedback...

Usability review

We discussed this issue at #3182480: Drupal Usability Meeting 2020-11-20.

The interface text looks fine. The only thing we did not like was handling the documentation page in a separate issue. Instead of adding the documentation page in #2947498: Document Migrate Drupal UI rollbacks, can we add it as part of this issue?

If you think that the documentation deserves a new page, then we can add the page now, but do not add it to the menu of the Upgrading Drupal guide. In practice, no one will find the page until we add it to the menu, but to be safe we can add a note at the top that it depends on this issue being fixed.

If you think the new documentation should be part of an existing page, then we could add a stub section now, saying that rollbacks will be possible once this issue is fixed. We could add a draft of the final text as a comment to this issue.

Either way, the patch here should be updated to add a link to the new documentation. I am setting the status to NW for that.

Probably this issue should also have a change record. I am adding the tag for that.

Last point: the new option says,

Remove content and configuration entities (such as fields and node types). Default values of other configuration will not be reverted (such as site name).

The new documentation should expand on that, with examples. I read that sentence two or three times before I realized that the key word is "entities".

Change record added.

Closed #2947498: Document Migrate Drupal UI rollbacks based on the usability review.

What is still to do is to add the documentation and update the patch. Leaving at NW

I think the documentation should be on the existing page, Upgrade using web browser, in a new section 'Starting the upgrade'. The new section would be for discussing the Overview Form, which has a version for when no migration has run and for when a migration has run.

Updated Upgrade using web browser, with a new section, 'Start', that will be explaining the Overview and Incremental forms.

I have removed the word 'entities' from the text for two reasons, one is the comment in #139 and the other is a memory of reading an issue that I can no longer find about not using the entity is user facing text. I reckon for many folk 'entity' has no meaning but the words content and configuration do and are sufficient here.

And a new screen shot

That was silly. I changed the text but not the tests.

I have removed the word 'entities' from the text ...

Good call. I think the reference you are looking for is Interface text > Word Choice. But the text now reads,

Remove content and configuration (such as fields and node types). Default values of other configuration will not be reverted (such as site name).

What is the difference between "configuration", as in "content and configuration", and "other configuration"? Is this variant accurate?

Remove content and configuration, such as fields and node types. Default values of simple configuration, such as site name, will not be reverted.

I prefer commas instead of parentheses, but I do not insist on it.

We will need to update the screenshot in the CR to show the updated text.

I am assigning the issue to myself for a code review. I plan to finish today.

1. This new test class has one test method, which seems to be a duplicate of MigrateNodeCompleteTest::testRollbackNodeComplete().

    +++ b/core/modules/node/tests/src/Kernel/Migrate/d7/RollbackNodeCompleteTest.php
    @@ -0,0 +1,44 @@

2. This change looks like an unrelated bug fix. If so, then it is out of scope for this issue. Should we create a follow-up issue, and add test coverage for whatever bug this is fixing?

    +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -313,14 +313,16 @@ public function rollback() {
    ...
           $destination_key = $id_map->currentDestination();
    -      if ($destination_key) {
    +      if (!empty($destination_key)) {
             $map_row = $id_map->getRowByDestination($destination_key);
    -        if ($map_row['rollback_action'] == MigrateIdMapInterface::ROLLBACK_DELETE) {
    -          $this->getEventDispatcher()
    -            ->dispatch(new MigrateRowDeleteEvent($this->migration, $destination_key), MigrateEvents::PRE_ROW_DELETE);
    -          $destination->rollback($destination_key);
    -          $this->getEventDispatcher()
    -            ->dispatch(new MigrateRowDeleteEvent($this->migration, $destination_key), MigrateEvents::POST_ROW_DELETE);
    +        if (is_array($map_row)) {
    +          if ($map_row['rollback_action'] == MigrateIdMapInterface::ROLLBACK_DELETE) {
    +            $this->getEventDispatcher()
    +              ->dispatch(new MigrateRowDeleteEvent($this->migration, $destination_key), MigrateEvents::PRE_ROW_DELETE);
    +            $destination->rollback($destination_key);
    +            $this->getEventDispatcher()
    +              ->dispatch(new MigrateRowDeleteEvent($this->migration, $destination_key), MigrateEvents::POST_ROW_DELETE);
    +          }
             }

3. Same here:

    +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
    @@ -276,10 +276,13 @@ public function rollback(array $destination_identifier) {
    ...
    -      $config = $this->storage->load($entity_id)->getConfigDependencyName();
    -      $config_override = $this->languageManager->getLanguageConfigOverride($language, $config);
    -      // Rollback the translation.
    -      $config_override->delete();
    +      // Ensure the entity exists before attempting to delete.
    +      if (!is_null($this->storage->load($entity_id))) {
    +        $config = $this->storage->load($entity_id)->getConfigDependencyName();
    +        $config_override = $this->languageManager->getLanguageConfigOverride($language, $config);
    +        // Rollback the translation.
    +        $config_override->delete();
    +      }
         }

4. Can we use StringTranslationTrait and then use $this->t() and whatever the plural version is called?

    +++ b/core/modules/migrate_drupal_ui/src/Batch/MigrateUpgradeRollbackBatch.php
    @@ -0,0 +1,211 @@
    ...
    +use Drupal\Core\StringTranslation\PluralTranslatableMarkup;
    +use Drupal\Core\StringTranslation\TranslatableMarkup;

5. Maybe MESSAGE_COUNT or MAX_MESSAGE_COUNT instead of MESSAGE_LENGTH:

    ...
    +  /**
    +   * Maximum number of previous messages to display.
    +   */
    +  const MESSAGE_LENGTH = 20;

6. Do we have to allow for the possibility that ’$initial_ids == []`?

    +  public static function run(array $initial_ids, array $config, array &$context) {
    +    if (!isset($context['sandbox']['migration_ids'])) {
    ...
    +      // migration_ids will be the list of IDs remaining to run.
    +      $context['sandbox']['migration_ids'] = $initial_ids;
    ...
    +    }
    ...
    +    $migration_id = reset($context['sandbox']['migration_ids']);

7. I have seen this code before! Should we postpone this issue on #2925899: MigrateUpgradeImportBatch does not use source_private_file_path & source_base_path correctly, making it impossible to have public & private files in separate locations? Better yet, can we avoid duplicating code between the two Batch classes?

    +    // @todo Find a way to avoid this in https://www.drupal.org/node/2804611.
    +    if ($definition['destination']['plugin'] === 'entity:file') {
    +      // Make sure we have a single trailing slash.
    +      if ($definition['source']['plugin'] === 'd7_file_private') {
    +        $configuration['source']['constants']['source_base_path'] = rtrim($config['source_private_file_path'], '/') . '/';
    +      }
    +      $configuration['source']['constants']['source_base_path'] = rtrim($config['source_base_path'], '/') . '/';
    +    }

   In fact, now that I know where to look, I really do not like how much code is duplicated between the two Batch classes. I think some refactoring is in scope for this issue. Also, the last lines quoted above are missing another recent change, #3151363: Double // in file paths.

8. Do we want to make rollback the default option, not incremental migration? (Is this discussed in earlier comments?)

9. Break before ->createInstances():

    +++ b/core/modules/migrate_drupal_ui/src/Form/IncrementalForm.php
    @@ -95,15 +120,54 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    ...
    +  protected function doRollback() {
    +    $config['source_base_path'] = $this->store->get('source_base_path');
    +
    +    $migrations = $this->migrationPluginManager->createInstances(array_keys($this->store->get('migrations')));

10. I think we will have to update the two functional tests once #3175953: Cleanup migrate drupal functional tests is fixed.

11. Is this method declared somewhere else?

     +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/Rollback6Test.php
     @@ -0,0 +1,82 @@
     ...
     +  /**
     +   * {@inheritdoc}
     +   */
     +  protected function getEntityCountsRollback() {

@benjifisher, thanks for the review.

145. Text changed, no more parentheses.

146.
1. Looks like a left over file. It is not needed. The rollback test is now part of MigrateNodeCompleteTest.
2. todo
3. todo
4. Don't think this can be changed because this is a static method.
5. I'd rather not do that here. Could be a followup.
6. I don't know, I just copied from MigrateUpgradeImportBatch.
7. Added a base class with a method for this bit of code.
8. There has been no discussion of what the default should be. Probably should not change it though, so it is 'Incremental'.
9. Fixed.
10. Yes, that is very likely.
11. There is a method called getEntityCountsRollback in both Rollback6Test and Rollback7Test. Even though those tests are so similar i decided not to make yet another base class.

One new screenshot added. showing the default of Incremental and the new text for rollback.

@quietone, thanks for the update. There is a lot here!

• #145: Shorter text is probably better, and we have a link to the more complete explanation.
• #146.1: Thanks
• #146.2, #1476.3: todo
• #146.4: Right, static functions always mess me up.
• #146.7, #146.8, #146.9: Thanks
• #146.11: Since there is no base class and no interface for these two classes, we should not use {@inheritdoc} for the getEntityCountsRollback() methods.

Adding the base class is a big help. I agree that we can postpone further work to a follow-up issue. There is still a lot of code that is duplicated in the run() method, and I would like to move that to the base class. It should handle all the sandbox management. The important thing for now is that we have isolated the code that we know is going to change from #3151363: Double // in file paths (already fixed) and #3175953: Cleanup migrate drupal functional tests (RTBC). We can handle #146.5, #146.6 in the follow-up issue.

Just one more request for the new base class:

    +++ b/core/modules/migrate_drupal_ui/src/Batch/MigrateUpgradeBatchBase.php
    @@ -0,0 +1,56 @@
    +<?php
    +
    +namespace Drupal\migrate_drupal_ui\Batch;
    +
    +/**
    + * Base class for migrations.
    + */
    +class MigrateUpgradeBatchBase {

Make the class comment a little more specific: something like "Base class for migration batch operations."

@benjifisher, thanks.

Nice, only comment changes!

146.11. Yes, of course. Just got confused, I am so used to seeing inheritdoc on the getEntityCount* methods I didn't even notice.
148. Thanks. I didn't like that class comment either.

Added followup. #3191590: [PP-1] Refactor Migrate Drupal UI batch classes.

And added another screenshot, so all the screenshots are up to date now.

Still NW for #146.2, #146.3. Those look like unrelated bug fixes, but maybe there is a reason to include them here.

Thanks for creating the follow-up issue.

-   * {@inheritdoc}
+   * Gets expected number of entities per entity after rollback.

That should be "... per entity type ...".

146.2 The change in MigrateExecutable was added in #88 and is not needed.

146.3 The change in EntityConfigBase was added in #73 and tests fail if it is not included. I am pretty sure it has to do with dependencies. For d6 it fails when rolling back d6_profile_field_option_translation but I am just not seeing what dependency is incorrect. However, even with that I do think that testing the entity exists before deleting it is a good practice and this change should stay.

This patch removes the change to MigrateExecutable and updates the comments as per #150.

1. Thanks for reverting the change pointed out in #146.2 (MigrateExecutable). I am updating the Proposed resolution to mention the change pointed out in #146.3:

    +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
    @@ -276,10 +276,13 @@ public function rollback(array $destination_identifier) {
    ...
    +      // Ensure the entity exists before attempting to delete.
    +      if (!is_null($this->storage->load($entity_id))) {
    +        $config = $this->storage->load($entity_id)->getConfigDependencyName();

   Can we make it simply if ($this->storage->load($entity_id))? If so, then break the long line while you are at it.

   Either way, this change looks completely safe. I guess the test failure is shown in Upgrade7Test in the tests for #71. If we want to add an issue to investigate the dependency problem, that is a good place to start. Even if we fix that, it is easy for someone working on a site upgrade to delete individual content items, then do a rollback. We do not want a PHP error when that happens, so a check for NULL here is a good idea.

2. The last things we need to do are the documentation updates. Is the CR up to date (including screenshots)? I am updating the issue summary since we are doing #2947498: Document Migrate Drupal UI rollbacks as part of this issue. Can we add some proposed text for Upgrade using web browser to a comment or the issue summary?