Problem/Motivation

We introduced a UI to perform Drupal site upgrades in #2281691: User interface for migration-based upgrades - this added the option to rollback. Which given the use-case probably does not make much sense - given that the UI is designed to be used on a fresh install and it is completely untested #2678994: Write tests for rollback.

Proposed resolution

Remove the rollback through the UI option prior to 8.1.0. We can always add it back if we find a use-case or need to. But we can add it back with tests.

Remaining tasks

Agree that this approach makes sense.

User interface changes

Remove the rollback option from the migrate UI.

Before

After

API changes

Depends on the solution.

Data model changes

None

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

Comments

alexpott created an issue. See original summary.

alexpott’s picture

I discussed this with @benjy in IRC and he said:

i think rollback as a feature makes sense in core, but i'm not sure about exposing it via the upgrade ui, not at this stage anyway

alexpott’s picture

Title: Write tests for rollback » Remove rollback option from the UI
alexpott’s picture

xjm’s picture

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.

alexpott’s picture

@xjm good point - but also the migrate UI is doing ID replacements so migrating to an existing site would be very very very unwise. In fact thinking about we should have some warnings about that in the UI.

benjy’s picture

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.

catch’s picture

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.

Gábor Hojtsy’s picture

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.

mikeryan’s picture

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.

xjm’s picture

Issue summary: View changes
FileSize
36.13 KB
11.77 KB

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).

xjm’s picture

Status: Active » Needs review
FileSize
17.34 KB

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.

xjm’s picture

Issue summary: View changes
FileSize
23.87 KB

And how that would look in the UI:

xjm’s picture

Issue tags: +beta target
xjm’s picture

Issue tags: +Needs change record
quietone’s picture

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

benjy’s picture

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.

alexpott’s picture

So testing incremental on 8.1.x it works for me however - it is completely broken after the migrate as plugins patch :(

Personally I think we should expand the scope this issue to remove the untested parts of the Migrate UI and then we can file issues to add each feature back with test coverage.

xjm’s picture

Title: Remove rollback option from the UI » Remove incremental and rollback options from the UI (and add them back when they are more stable)
xjm’s picture

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.

catch’s picture

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.

alexpott’s picture

Created #2687307: Fix Migrate UI form when incremental or rollback selected to fix the incremental / rollback UI - well at least how the plugin patch broke it.

xjm’s picture

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.

xjm’s picture

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.

xjm’s picture

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.

xjm’s picture

FileSize
11.23 KB

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

tstoeckler’s picture

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?

alexpott’s picture

@tstoeckler because you have to choose which to enable modules to get all the migrations.

tstoeckler’s picture

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.

alexpott’s picture

+++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
@@ -1019,14 +990,8 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
+    // @todo Add back support for rollbacks.
+    if (TRUE) {

@@ -1125,45 +1090,8 @@ public function buildConfirmForm(array $form, FormStateInterface $form_state) {
+    // @todo Add back support for rollbacks.
+    if (TRUE) {

I think we should just completely disentangle the forms in the future so rather than just do 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.

Here's a patch that does that and also removes a code path in ::validateCredentialForm() that this patch makes unreachable as it would only get executed for incremental migrations. Also attached a patch that is simpler to review generated with git diff -w to exclude whitespace differences.

xjm’s picture

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.

alexpott’s picture

@xjm sorry - my bad - I should have read

Obviously the if (TRUE) is also not really shippable

from #25. I was just concentrating on removing the obscure code path from ::validateCredentialForm() and removed the if (TRUE) in the name of consistency in removing dead code paths.

xjm’s picture

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.

hussainweb’s picture

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.

xjm’s picture

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.

bojanz’s picture

+1 to #36, thanks for clarifying.

hussainweb’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.4 KB
21.69 KB

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

xjm’s picture

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.

hussainweb’s picture

@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).

catch’s picture

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

xjm’s picture

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

  • catch committed 688e42f on 8.2.x
    Issue #2683421 by xjm, alexpott, hussainweb: Remove incremental and...
catch’s picture

Status: Reviewed & tested by the community » Fixed

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!

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: +8.1.0 release notes

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

webchick’s picture