Remove incremental and rollback options from the UI (and add them back when they are more stable)

Hm, rollback was considered a blocker for adding the UI, with the reasoning that site owners needed a way to recover from botched or incomplete migrations (i.e. an unrecoverable state).

I could see the case that the rollback itself isn't safe either, but if we removed it from the UI I think we would in turn need to do a lot more to make it clear that the D8 site you're migrating into is considered 100% disposable. I guess let's get more feedback.

It was always my understanding that the upgrade process would have to be into a fresh D8 install. I believe if we provide rollback in the UI, then obviously non-developers will be using it and we could end up with some really edge case bugs that are side-effects of rolling back and migrating many times.

Rollback is a feature best exposed in a more developer orientated tool like the contrib UI and drush runners.

I agree with removing rollback support from the UI for now.

We can open an issue with patch -p1 -R to add it back, but seems a lot easier to add signposts for people that they should use a disposable install (possibly a tour?) than trying to support rollback, especially while migrations themselves aren't 100% yet.

We can test how much content/users/roles, etc. there is on the site before the migration and warn people about it in this UI module if we think this is targeted only at empty sites, right? I don't have a strong opinion on the removal either way.

I'm not strongly opinionated on this. It seemed like a good bang-for-the-buck feature at the time - although the primary upgrade scenario is always starting entirely from scratch, for little effort we made it easier to turn things around a bit quicker than a site-install. If the consensus is to enforce rebuild-from-scratch, that's fine.

So currently, when a migration has already been done, the user has a choice to either perform an incremental migration or roll it back.

Should we disallow either for now, or should we just always perform an incremental migration? I was going to suggest the latter, but I think the incremental option is actually broken as well because both locally and on simplytest.me I get this when attempting to rerun the migration:

Removing the option entirely would actually be an internal API change (which is okay too because the module is alpha-experimental).

Attached would just disallow running a migration if the migration had already been performed. It also removes the untested code paths so that they can be re-added one they are stable and tested.

And how that would look in the UI:

The msg in #13 states 'back up your data'. What data is that referring to?

but I think the incremental option is actually broken as well because both locally and on simplytest.me I get this when attempting to rerun the migration:

I think this is because on the testbot it has multiple database engines available but you only have 1. There is a fix for that in the UI improvements issue and the plugins issue.

I think this is because on the testbot it has multiple database engines available but you only have 1. There is a fix for that in the UI improvements issue and the plugins issue.

I do actually have multiple database engines, so there is some other bug affecting my environment as well as simplytest.me at least on 8.1.x.

The msg in #13 states 'back up your data'. What data is that referring to?

Well, we always say this for destructive operations. ;) In this case, presumably anything in the D8 site that is not from the migration.

We probably need to re-word that, since people aren't going to be able to restore their backup into a migrated site, they just shouldn't run a migration on a site with content at all.

Regarding #21, the first step of the form currently says:

Back up the database for this site. Upgrade will change the database for this site.

Even though the intro says:

Upgrade a Drupal site by importing it into a clean and empty new install of Drupal 8. You will lose any existing configuration once you import your site into it. See the upgrading handbook for more detailed information.

So it's not introduced here at least.

OTOH https://www.drupal.org/node/2350603 does say in big letters "Do not configure the Drupal 8 site" so maybe we can just link the migrate handbook instead.

My previous patch no longer applied and also had left some dead code paths. After going over this again, I'd recommend we refactor so that a single batch run() method does not handle both incremental migrations and rollbacks, and probably also that the rollback form builder has separate handling. The long conditionals that assume in some places "what is not rollback is import" but in other places "what is not import is rollback" without actually checking the submitted option are problematic to maintain.

Attached leaves in some unused control structures with some @todos instead (they need followups added). No interdiff because the patch was completely recreated. Obviously the if (TRUE) is also not really shippable but it should make it clearer in the followup where to cut on the dotted lines.

Sorry, #25 was only half the patch. Here is the full one.

Just wanted to throw this out there, I apologize if this is off-topic of leads to derailment:
If the only supported usage is a one-time operation directly after a fresh install why not make this an installation profile?

Hmm... yeah I thought about that as well. Still would be nice, and - I think - doable to select the modules as part of a profile, but that should not hold this up. Will play around with it and open a follow-up if there's anything there. Thanks for the feedback, though.

if (TRUE) { I think we should just re-indent the code - yes this patch is going to get messy but the result will be far easier to maintain.

The original patch did reindent. As I said, I obviously don't consider if (TRUE) shippable. It was just to show the issue with the entangled logic.

Added followup issues:

• #2687843: Add back incremental migrations through the UI
• #2687849: Add back rollbacks on migrate_drupal_ui (postponed on the first)
• #2687851: Refactor run() method on Migrate UI batch and remove the $operation parameter

Attached adds a @todo for the redundant control structures in the batch runner (ifs where no else is possible, same as in the form builder). I filed a followup rather than doing it in this issue because I want to make sure we get these options out of the UI ASAP and not block it on the correct way to refactor these controllers.

It's a pity we are removing these. I don't care too much about rollback but incremental is very useful. I needed to keep running migrations repeatedly when testing this out today and i can't imagine reinstalling the site every time I want to try again.

I see why it might be better to just rebuild the thing from scratch later but can't we keep it and improve it here itself, considering that the entire module is considered experimental? There are two warnings when enabling the module from the UI. Isn't that enough to tell people that things might not work as they expect?

As for the errors in migrate UI when we changed everything to plugins, I just found that issue today and posted a patch. I created #2691189: Rerunning a migration results in an error which I found was duplicate of #2687307: Fix Migrate UI form when incremental or rollback selected. Both patches fix it in a very similar way. Can we not just go ahead with that for now and fix the UI here?

If, ultimately, the migrations can be run repeatedly through some other means (like drush), it is okay. In any case, I don't mind the forms not working eventually considering that I know this is marked experimental.

To clarify, I agree that probably rollbacks don't make much sense and maybe that can be removed, but instead of removing ability to rerun, let that be the default option and provide another option to start over.

In general @hussainweb I totally agree. Incremental migrations are a required part of the functionality, which is why I marked #2687843: Add back incremental migrations through the UI as a Migrate critical.

I agree that probably rollbacks don't make much sense and maybe that can be removed, but instead of removing ability to rerun, let that be the default option and provide another option to start over.

That was what I tried at first, but unfortunately it was critically broken as described in #11 -- it did not work at all. And we need to finalize what will be in 8.1.0 now-ish. We can add this functionality back to the UI in 8.1.1 or such because it is an experimental module, but I'd rather ship with only actual support for things rather than pretending to support it and not supporting it. So this issue is essentially a hotfix for the beta, and once 8.1.0 is out we can add back support properly and with test coverage.

Incremental migration should still work with Drush, just not via the UI form.

+1 to #36, thanks for clarifying.

> Incremental migration should still work with Drush, just not via the UI form.
Thanks! I can live with that for now.

I was wondering if we should at least make it possible for the user to start over? In other words, a button on the overview form that clears the state 'migrate_drupal_ui.performed'. This lets you use the form at least to start over if you know what you are doing. This would not be incremental migration, just starting over. I think it could be as simple as removing the return $form; at line 706 (after applying the patch). I don't want to derail this issue and so if you think it's not worth it, it's fine.

Fixing a typo in the patch. I see links to followups are present in comments. Above notwithstanding, +1 to RTBC. I just fixed a typo, so I am assuming it is okay for me to RTBC.

Thanks @hussainweb! That typo fix looks good to me.

I was wondering if we should at least make it possible for the user to start over? In other words, a button on the overview form that clears the state 'migrate_drupal_ui.performed'. This lets you use the form at least to start over if you know what you are doing. This would not be incremental migration, just starting over. I think it could be as simple as removing the return $form; at line 706 (after applying the patch). I don't want to derail this issue and so if you think it's not worth it, it's fine.

I think though starting over on top of a previous migration actually is incremental migration? As far as I can tell from the UI module code anyway; incremental migration is the exact same code path as a fresh migration in the batch. If I'm mistaken though we can look into it in a followup or in #2687843: Add back incremental migrations through the UI.

@xjm: I think you're right. Let's leave it for the followup then. If somebody really wanted to do this, all they'd have to do would be to reset the state (which is possible with drush).

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -1019,98 +961,89 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
+    // Sort the table by source module names and within that destination
+    // module names.

Nit. Could use a comma after 'within that'.

Couldn't find anything else. I'll give this another once over a bit later on, and commit if that's still the case.

I think #41 is just reindented; might want to check it locally with a git diff -w.

I'd add to #36 that we're still finding quite nasty bugs like #2682705: Migrate process plugin does not save stubs to the idmap, leads to duplicates and broken references / #2690001: Migrate: Don't deduplicate forum vocabulary. Originally #2682705: Migrate process plugin does not save stubs to the idmap, leads to duplicates and broken references looked like an issue with incremental migrations, but it's was an issue with any migration that uses stubs.

I'm hopefully that the migrate UI will result in lots more 6.x migrations being tested on a wider variety of sites, which should in turn lead to more bug reports. But when looking at those, it's good to have the least possible variables we can for the moment. So removing this then putting it back in to 8.1.1 or similar seems very sensible.

Committed/pushed to 8.1.x and 8.2.x, thanks!

We should call this out specifically in the experimental modules section of the release notes.