Problem/Motivation

The UI for upgrades to Drupal 8 from Drupal 6/7 is being developed in contrib, to be pulled into core when "ready" (the current hope is to get it in for RC1). This UI is under development as migrate_upgrade.

The detailed current design plan is in a Google doc - anyone can read and comment there. Broadly, the workflow should be as follows:

Initial upgrade

  1. Overview page (at http://example.com/upgrade), describing what is to happen and how to prepare for it.
  2. Credentials page - get the DB info for the source site, as well as the address from which to fetch the files.
  3. Report/confirmation page - After analyzing the source database, report on specifically what is going to happen - what source modules will be imported into what destination modules, and what source modules are not being upgraded.
  4. Batch - the migrations are run through the normal Batch API.
  5. Post-upgrade report - message summarizing successful/failed migrations, with link to watchdog listing of specific messages.

Subsequent upgrade

When returning to /upgrade, the following workflow will happen:

  1. Overview page - reports that an upgrade has already been performed, and offers the option of rolling back and starting over, or running an incremental upgrade (importing new content, and content that now has an upgrade path which previously didn't).
  2. If rolling back was chosen, imported configuration entities (e.g., fields, node types, etc.) and content are deleted and migration configuration removed (simple configuration is *not* restored to its original state), and we return to the original overview page.
  3. Otherwise, we skip the credentials page (we already have that information) and go directly to the report/confirmation page, proceeding normally from there.
Files: 
CommentFileSizeAuthor
#151 2281691.quick-fix-on-commit.txt684 bytesalexpott
#147 interdiff-hook-help.txt1.13 KBxjm
#147 migrate-2281691-147.patch78.26 KBxjm
#141 interdiff_strings.txt1.36 KBxjm
#141 migrate_ui_2281691-141.patch78.25 KBxjm
#137 interdiff.txt564 bytesGábor Hojtsy
#137 2281691-137.patch78.28 KBGábor Hojtsy
#136 interdiff.txt8.36 KBGábor Hojtsy
#136 2281691-136.patch78.28 KBGábor Hojtsy
#126 purkiss-post-d81-migrate-ui.png297.88 KBstevepurkiss
#124 corify_migrate_drupal_ui.txt2 KBalexpott
#123 121-123-interdiff.txt471 bytesalexpott
#123 2281691-123.patch77.59 KBalexpott
#121 2281691-121.patch77.13 KBalexpott
#121 corify_migrate_drupal_ui.txt1.47 KBalexpott
#115 2281691-migrate-115.patch64.61 KBtim.plunkett
#90 corify_0.txt1.43 KBquietone
#90 10-Log.png23.9 KBquietone
#90 09-UpgradeComplete.png9.28 KBquietone
#90 08-UpgradeRunning.png57.45 KBquietone
#90 07-BottomofConfirmPage.png12.56 KBquietone
#90 06-TopofConfirmPage.png18.54 KBquietone
#90 05-Credentials.png34.01 KBquietone
#90 04-UpgradeStart.png34.16 KBquietone
#90 03-InstalledMsg.png25.04 KBquietone
#90 02-VerifyInstall.png13.87 KBquietone
#90 01-Extend.png11.81 KBquietone
#86 corify.txt1.09 KBmikeryan
#84 meta_user_interface-2281691-84.patch62.64 KBmikeryan

Comments

mikeryan’s picture

mikeryan’s picture

Issue summary:View changes
mikeryan’s picture

Issue summary:View changes
mikeryan’s picture

Issue summary:View changes
mikeryan’s picture

Issue summary:View changes
mikeryan’s picture

Issue summary:View changes
mikeryan’s picture

Issue summary:View changes
mikeryan’s picture

Issue summary:View changes
mikeryan’s picture

Issue summary:View changes
mikeryan’s picture

For context, attached are screenshots of the interface as reviewed with Bojhan and webchick - all the current child issues are relative to this version of the UI.

mikeryan’s picture

Issue summary:View changes
mikeryan’s picture

Issue summary:View changes

That's it, all the known child issues at this time have been added.

mikeryan’s picture

So, out of all these child issues, the biggest one is #2282021: Consolidate upgrade process to one form/one click. I'm thinking of simplifying it even further, narrowing the scope of the migrate_upgrade functionality to just a one-click full configuration-and-content import (or maybe allowing a configuration import and subsequent content import). Please weigh in at #2282021: Consolidate upgrade process to one form/one click.

Thanks.

benjy’s picture

Would be nice to have an update on this issue of the current state of the UI, big outstanding issues etc.

mikeryan’s picture

Issue summary:View changes

Added context issue.

mikeryan’s picture

Sorry I missed that question - basically, the UI has been working to the extent the underlying framework has for several months, with occasional chasing-HEAD breakage. I haven't tried it for a few weeks so it might well be broken at the moment, but catching up with recent core changes should take care of that. The open issues aren't really major - either cleanup/improving reuse, or providing more/better feedback - the biggest thing I think is the context issue. Should the migration readiness assessment being discussed be integrated with this upgrade form, or separate? Should it be in contrib - in migrate_plus, or perhaps coder_upgrade?

benjy’s picture

Also, after talking with webchick I thought i'd do an initial review, i forked migrate_upgrade into a sandbox and i've done some general clean-up here: https://www.drupal.org/sandbox/benjy/2448261

@mikeryan, not sure if you want me to post it into one big patch in the migrate upgrade issue queue or if you want me to break it up. The diff is here: http://privatepaste.com/458f93e589

Also, I can confirm, all is working with HEAD as of the last day or two.

mikeryan’s picture

I would like to see it broken up, I see a few different things going on there it'd be easier to review separately:

  1. Database driver setup refactoring.
  2. Version detection changes.
  3. Batch results logging.
  4. Miscellaneous rearrangement and simple coding standard stuff (e.g., use of $this->t()).

On a quick once-over I have some comments/questions, but it'll be easier to discuss in more specific issues.

Thanks!

benjy’s picture

Sorry, I should have mentioned there is an overview on the sandbox overview page.

iantresman’s picture

Just to add my support for a user interface (UI). Most people running Drupal sites are not programmer/developers, and without a UI, will find migration very difficult, if not impossible.

I envisage an option to specify a Drupal 6/7 mySql hostname, username and password, and the migration system to investigate what may be migrated, followed by an option to select data to migrate.

I believe the node import module provides a UI to import/migrate from CSV files.

webchick’s picture

Priority:Normal» Major
Issue tags:+Migrate critical

Escalating to major, and also creating the "Migrate critical" tag since this is actually critical (but not critical enough to block D8's release).

#2030501: [meta] Ensure that Drupal 6 sites have a functional upgrade path to either Drupal 7 or 8 before Drupal 6 loses security support is a D8 critical tracking this.

mikeryan’s picture

An update - right now, the main thing I want to get into the UI under development as migrate_upgrade before submitting to core is #2463909: Migrations should support non-installed default configurations (templates) - been busy pursuing other basic core API issues, but I'll try to get my focus back on that one. Actually, the next thing I plan to do is #2504759: Implement drush migrate-upgrade command, which will make it easier to develop/test the template support.

webchick’s picture

Status:Active» Needs review
Issue tags:+Needs usability review, +needs accessibility review

So while there's no patch here yet, https://www.drupal.org/project/migrate_upgrade actually exists and is functional. (I tried it myself last week on both mine and Dries's blogs :)).

If we want to get this in before RC1 (which really only makes sense once the other migrate criticals are resolved), we need to do some reviews of it ASAP from a UX/a11y POV. Tagging.

webchick’s picture

Testing instructions + screenshot at https://www.drupal.org/node/2257723

mikeryan’s picture

Quick question - what should the module name be for the UI? The existing contrib migrate_upgrade module contains both UI and a drush command, and at least for the short term (until we follow through with submitting it to the drush project) continue to exist for the drush. Should the upgrade UI in core be migrate_upgrade_ui? Or perhaps migrate_drupal_ui?

andypost’s picture

is this UI depends on migrate_drupal or migrate module itself?

mikeryan’s picture

Yes - it's a UI to run the migrations defined in migrate_drupal.

Pending any objections, right now I'm doing it as migrate_drupal_ui...

mikeryan’s picture

mikeryan’s picture

Oh, on the migrate_drupal_ui name - I just want to point out a counter-argument to using that, which is that while migrate_drupal provides a general framework for Drupal-to-Drupal migrations, this UI is narrowly focused on the straight, complete upgrade use case - a general-purpose UI for doing custom Drupal-to-Drupal migrations will go into contrib (D8 version of migrate_d2d).

webchick’s picture

I would just stick it in the Migrate Drupal module directly, personally. That might not be popular. :)

benjy’s picture

I would just stick it in the Migrate Drupal module directly, personally. That might not be popular. :)

I don't mind that idea if we could wrap it in a permission.

Otherwise migrate_upgrade_ui is probably better for contrib and migrate_drupal_ui better for cores more specific use case.

mikeryan’s picture

Issue tags:+Needs tests
FileSize
23.54 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,595 pass(es). View

Here's a patch adding the upgrade UI to core as module migrate_drupal_ui. Obviously needs tests, UX work, etc., but it is functional. I've done some cleanup relative to the contrib migrate_upgrade version - the most visible change is providing more feedback while running the migrations.

Heading off the first question a code reviewer will have looking at this patch in isolation - what's with this MigrationCreationTrait and why isn't it simply implemented within MigrateUpgradeForm? The reason is the exact same functionality is needed by the migrate-upgrade drush command (currently in the contrib migrate_upgrade module, ultimately to be added to drush itself) - rather than having the drush command (or, for that matter, any alternate upgrade front ends people might want to build) reinvent the wheel, it should be reusable. We could make it a class instead - maybe even a service - feedback welcome.

andypost’s picture

  1. +++ b/core/modules/migrate_drupal_ui/migrate_drupal_ui.info.yml
    @@ -0,0 +1,10 @@
    +dependencies:
    +  - migrate
    +  - migrate_drupal
    
    +++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
    @@ -0,0 +1,24 @@
    +    return $this->redirect('dblog.overview');
    

    so it depends on migrate_drupal and dblog

  2. +++ b/core/modules/migrate_drupal_ui/migrate_drupal_ui.install
    @@ -0,0 +1,16 @@
    +function migrate_drupal_ui_install() {
    +  $url = Url::fromUri('base:upgrade')->toString();
    +  drupal_set_message(t("The Migrate Drupal UI module has been enabled. Proceed to the <a href='@url'>upgrade form</a>.", array('@url' => $url)));
    

    useless

  3. +++ b/core/modules/migrate_drupal_ui/migrate_drupal_ui.module
    @@ -0,0 +1,19 @@
    + * @file
    + * Alert administrators before starting the import process.
    

    module do different

  4. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,164 @@
    +/*
    +    $form['files'] = array(
    +      '#type' => 'details',
    

    tbd

mikeryan’s picture

+++ b/core/modules/migrate_drupal_ui/migrate_drupal_ui.info.yml
@@ -0,0 +1,10 @@
+package: Core

Just a note that per https://www.drupal.org/node/2313651#comment-10269591, on the next roll of this patch let's set the package to Experimental.

phenaproxima’s picture

I think the Migrate Upgrade module should be rolled straight into Migrate Drupal, not as a submodule. It has no other use beyond Migrate Drupal. It was always intended to work with Migrate Drupal. IMO there's no point hiding the UI while exposing Migrate Drupal's API (such as it is); hide it all or don't.

mikeryan’s picture

Well, the migrate_drupal framework will be useful in contrib (migrate_d2d) to support custom Drupal-to-Drupal migration paths. That being said, I'm imagining migrate_d2d may end up using the core upgrade form anyway (altering it to be the first step of a wizard instead of a standalone form), so having the upgrade UI automatically present when migrate_drupal is enabled isn't necessarily unnecessary...

webchick’s picture

Part #1/N of my review. :)

Ok testing this patch, along with #2532534: Migration Files for Drupal 7 Comments against the database from webchick.net. This is a fairly bare-bones D7 site with no additional contribs other than Omega for the theme and Mollom for the spam fighting. It has been upgraded since Drupal 5, I believe. It also has some cruft (one time I tested advpoll module out, installed wp once or twice), but since most real-world databases will also have cruft, I think that's ok. ;)


First, I got very confused because I couldn’t find the module to turn it on. ;) That’s because it doesn’t yet have the “Core (Experimental)” package like the other Migration modules do.

RECOMMENDATION Change package to “Core (Experimental)”

Once that’s done, it looks like this:

Migrate, Migrate Drupal, and Migrate Drupal UI modules all listed under Core (Experimental) package on modules page.

Turning on Migrate Drupal UI automatically enables the Migrate and Migrate Drupal modules. So far so good!

Upon enabling it you get the following message at the top of the modules page, directing you to the upgrade form at /upgrade.

The Migrate Drupal UI module has been enabled. Please proceed to the upgrade form.

The only problem with that is if you missed that message in that moment, or if someone else enabled this module before you got there, there’s no way to figure out what you’re supposed to do. There’s no help page (link just says “No help is available for module Migrate Drupal UI.” ), and there’s also no “configure” link on the module from the modules page to jump me right there.

COMMIT BLOCKER Need a hook_help(), like all Drupal modules. Part of the docs gate.
RECOMMENDATION Add a “configure” property to the .info file that points off to /upgrade.

Once you meander over to /upgrade, you see this:

Form with fields for source DB and files directory location.

First, it asks for your source DB credentials, then the location of your public files directory.

OBSERVATION Oh sweet, you can enter the location of a local file directory, too. That probably makes a lot more sense than http://, given that would both a) greatly slow down the migration with 100s or 10000s of HTTP requests and b) pummel your live site, if that’s the address you gave, which seems to be what it’s asking for.

RECOMMENDATION I think remove the default here, and switch the help text around to mention the local files directory option first.

OBSERVATION What about private files? Mike pointed out that for D6 there was no distinction between private/public files, so one file destination will get both. D7 though has separate directories, and for that there’s #2547125: D7 file migration should allow selecting public/private/all files. Tagged that one as a Migrate D7 critical.

RECOMMENDATION Add a version selector to the form (at the top, most likely): Drupal 6 or Drupal 7. If Drupal 7 is selected, toggle visibility of a “Private files” text field. (This selector could also be form_altered() by contrib to provide for example a Drupal 5 selection as well, or used as a “rip-cord” in the event that D7 is not ready when we want to support D6 “officially”; just hard-code the field to D6 and let contrib alter other options in.)

RECOMMENDATION Remove the word “public” from that field’s help text if D6 is selected, since it’ll actually import all files in that case.

RECOMMENDATION More minor, but “Source site files/path” sounds a bit overly technical. I would call this “Source Files” (for the details group, to match “Source Database”) and “Files directory” for the textbook label.

Going to run through an actual migration next.

yoroy’s picture

Initial thoughts based on webchicks screenshots above:

Since this is a set of modules for a very specific and one-time scenario, that message with the link to the upgrade form should probably be persistent. Similar to how it works when you're in maintenance mode.

I think we need more careful and consistent wording, it's a mix of 'upgrade' and 'migrate' at the moment.

hass’s picture

Add a version selector to the form (at the top, most likely): Drupal 6 or Drupal 7. If Drupal 7 is selected, toggle visibility of a “Private files” text field.

We may better connect to the source database and detect the source version automatically. May add one extra step or use ajax.

webchick’s picture

Here's part #2/N, aka, "What happens when things go wrong." :)


Ok, heeeere we go! Testing a D7 -> D8 migration now.

First, I fill in my values:

Database/file values

Incidentally, if you fill in total garbage here, the error messages are quite obtuse:

Raw SQL error

RECOMMENDATION It would be ideal if we could re-use the same error handling as the installer, which prints much user-friendlier messages.

OBSERVATION There doesn't seem to be any validation around the file path field? Or at least I wasn't able to trigger it despite typing in garbage values for both local filepath and http:// address (that could've been due to "migrate already happened" errors tho, see below... but in any case we should probably validate the form first before we proceed).

This instantiates a Batch API process, like so:

Batch API indicates various things are being imported.

So far, so good.

Then, you hit a problem, such as:

Lots of PHP problems

Example output:


Missing bundle entity, entity type node_type, entity id story. (/Users/webchick/Sites/8.x/core/lib/Drupal/Core/Entity/EntityType.php:793)
Missing bundle entity, entity type comment_type, entity id comment_node_advpoll_binary. (/Users/webchick/Sites/8.x/core/lib/Drupal/Core/Entity/EntityType.php:793)
Missing bundle entity, entity type comment_type, entity id comment_node_advpoll_ranking. (/Users/webchick/Sites/8.x/core/lib/Drupal/Core/Entity/EntityType.php:793)
Missing bundle entity, entity type comment_type, entity id comment_node_story. (/Users/webchick/Sites/8.x/core/lib/Drupal/Core/Entity/EntityType.php:793)
Missing bundle entity, entity type comment_type, entity id comment_node_page. (/Users/webchick/Sites/8.x/core/lib/Drupal/Core/Entity/EntityType.php:793)
Missing bundle entity, entity type node_type, entity id advpoll_binary. (/Users/webchick/Sites/8.x/core/lib/Drupal/Core/Entity/EntityType.php:793)
Missing bundle entity, entity type node_type, entity id advpoll_ranking. (/Users/webchick/Sites/8.x/core/lib/Drupal/Core/Entity/EntityType.php:793)
Missing bundle entity, entity type node_type, entity id story. (/Users/webchick/Sites/8.x/core/lib/Drupal/Core/Entity/EntityType.php:793)
Missing bundle entity, entity type node_type, entity id story. (/Users/webchick/Sites/8.x/core/lib/Drupal/Core/Entity/EntityType.php:793)

RECOMMENDATION One annoying thing off the bat is that I can see there are errors, but in almost all cases they disappear faster than I can copy/paste or screenshot them. (something about php_code, something about files, something about story). The output upon moving to each step gets overrridden. Ideally, the messages would just append together and I could save the entire thing as a log of what happened (this is what update.php used to do).

RECOMMENDATION These things are errors, so should be styled as errors, so they're not missed.

Once the migration is complete, you’re automatically redirected to your home page with a status message on how you did:

Indicates success/fail of migrations

RECOMMENDATION Here we’re using “Import” which is a third synonymous word with Migrate/Upgrade. Agreed with yoroy; let’s definitely get the terminology cleaned up.

The "Review detailed migration log" link takes me to the normal database log, with the messages filtered down to migrate_drupal_ui automatically (nice):

Database log showing migrate errors

COMMIT BLOCKER However, the big problem is that none of the PHP errors I saw from earlier are included here. :( Only the high-level info on what passed/failed. I tried resetting the db log filters, thinking these were possibly “php” errors and thus wouldn’t show up in the filtered view, but nothing there either. Migrate is somehow eating these PHP errors from the log..?

COMMIT BLOCKER When a migration fails, the message is “Import of Drupal 7 comments failed”. Well, thanks a lot. ;) How did it fail, and where do I start debugging?

OBSERVATION Some of these seem to report false positives. The row at the top of my report, “Imported Drupal 7 node title label” does not have a corresponding failure row, yet obviously did not work because my site still has no nodes. :)

So then you might think to yourself, "Ok, self. Let's apply some more migrate patches and try that migration again!" Oh, except you can't. You get this when you hit the "Perform upgrade" button again:

'Migration' entity with ID 'd7_comment' already exists

COMMIT BLOCKER We need a way to either re-do the migration or to clear it so you can start from scratch again.

RECOMMENDATION That error message is also super obtuse. Can we get that cleaned up to be more human-friendly? :) "Comments were already imported. Clear database and try again." or similar.

webchick’s picture

Two other notes to capture from IRC:

1) The migrate UI is currently associated with "administer site configuration" but on many sites that's given out kind of willy-nilly. We should most likely base it around "Administer software updates" instead. (Generally-speaking, only User 1 should ever be using this.)

2) While untested (I'll test this as soon as I can get something to actually migrate ;)), the likely thing that happens when using this UI is that any existing content/users/etc. you have are going to be overridden with those from the old site. For example, user 1 was admin, now it's "root." Node 1 was "test" now it's "Welcome to our site."

Given that, we probably need to do a couple of things:

- Big, fat, undeniable red warning on this page (maybe even make "Perform upgrade" red instead of "friendly blue") if site has existing content, and what the consequences of that will be.
- Possibly in the installer, make this an explicit choice: "are you installing D8 to build from, or are you installing D8 to migrate to?" (This would be a separate issue, since there's a lot to contend with there.)

webchick’s picture

mikeryan’s picture

Status:Needs review» Needs work

Anything marked "done" here is in the code for the next patch, more complex things will come.

so it depends on migrate_drupal and dblog

Good point - for the moment, I'm adding the explicit dependency to the .info.yml file. But, is requiring dblog a good idea?

RECOMMENDATION Change package to “Core (Experimental)”

Done.

RECOMMENDATION Need a hook_help(), like all Drupal modules. Part of the docs gate.

hook_help() was there, but not for the help.page.migrate_drupal_ui route. Added some initial help text there.

RECOMMENDATION Add a “configure” property to the .info file that points off to /upgrade.

Hmm, it is there and working for me (at least with the changes I've made so far, none of which seemed specifically applicable)?

RECOMMENDATION I think remove the default here, and switch the help text around to mention the local files directory option first.

Done.

RECOMMENDATION Add a version selector to the form (at the top, most likely): Drupal 6 or Drupal 7. If Drupal 7 is selected, toggle visibility of a “Private files” text field. (This selector could also be form_altered() by contrib to provide for example a Drupal 5 selection as well, or used as a “rip-cord” in the event that D7 is not ready when we want to support D6 “officially”; just hard-code the field to D6 and let contrib alter other options in.)

Will do. Note that actually handling private files for D7 depends on #2547125: D7 file migration should allow selecting public/private/all files.

RECOMMENDATION Remove the word “public” from that field’s help text if D6 is selected, since it’ll actually import all files in that case.

Done for the moment, D7 variant to be added as part of the above.

RECOMMENDATION More minor, but “Source site files/path” sounds a bit overly technical. I would call this “Source Files” (for the details group, to match “Source Database”) and “Files directory” for the textbook label.

Done.

Since this is a set of modules for a very specific and one-time scenario, that message with the link to the upgrade form should probably be persistent. Similar to how it works when you're in maintenance mode.

I'll look into how that's done. Related thought - should we require the site to be in maintenance mode before allowing the upgrade operation?

I think we need more careful and consistent wording, it's a mix of 'upgrade' and 'migrate' at the moment.

So, as discussed on IRC - this form is specifically for a direct, simple upgrade from an earlier version of Drupal to Drupal 8, which is a special case of the general concept of migration. Other scenarios (including customized Drupal-to-Drupal migrations) will be supported in contrib and stick to the "migrate" language. So, my original thought was to make sure all user-facing text here uses the "upgrade" language - but, it is built on the migrate and migrate_drupal modules (which support the wider "migrate" use case), and any errors that bubble up from those modules will be using the "migrate" language, which could be confusing. Once we've got the framework nailed down such errors should be rare, but they will happen.

We may better connect to the source database and detect the source version
automatically. May add one extra step or use ajax.

We're doing the automatic detection now. The issue there is that only D7 needs that private file directory specified, and it's not practical to enable this with ajax from the db credentials. We also really want to keep this to a single form if we can. So, an explicit version selection doesn't seem overly burdensome, all things considered - the detection code will still be there to sanity-check the selection.

And one other thing, upgraded array() instances to [].

Now, on to the newer comments...

webchick’s picture

Incidentally, I think we should push the "whether or not to do D6/D7 as an explicit selector and/or how to show the public/private text fields for D7" issue over to #2547125: D7 file migration should allow selecting public/private/all files. That's likely to need some discussion, so we shouldn't hold this UI up which I was able to just successfully use to migrate webchick.net's content over to D8 (with a few migrate bug fix patches). YEAH! :D

mikeryan’s picture

RECOMMENDATION It would be ideal if we could re-use the same error handling as the installer, which prints much user-friendlier messages.

Will look into how the installer nicifies the messages.

OBSERVATION There doesn't seem to be any validation around the file path field? Or at least I wasn't able to trigger it despite typing in garbage values for both local filepath and http:// address (that could've been due to "migrate already happened" errors tho, see below... but in any case we should probably validate the form first before we proceed).

So, file_exists() on a local filepath and... get_headers() on an HTTP URL? Or, anything with ://, no reason you couldn't put the files on ftp://...

RECOMMENDATION One annoying thing off the bat is that I can see there are errors, but in almost all cases they disappear faster than I can copy/paste or screenshot them. (something about php_code, something about files, something about story). The output upon moving to each step gets overrridden. Ideally, the messages would just append together and I could save the entire thing as a log of what happened (this is what update.php used to do).

The problem here in circumstances like this is the volume - the modern batch API puts the messages above the progress bar, which will get shoved below the fold in no time. I'm sure there's a way to move the progress bar above though, but I don't like the idea of unbounded messages being saved and displayed, it can get crazy if some module has an "Undefined index" notice triggered by every node you migrate (all too common in my D7 experience).

RECOMMENDATION These things are errors, so should be styled as errors, so they're not missed.

Will look at that, MigrateMessageCapture will need to get the message levels.

COMMIT BLOCKER However, the big problem is that none of the PHP errors I saw from earlier are included here. :( Only the high-level info on what passed/failed. I tried resetting the db log filters, thinking these were possibly “php” errors and thus wouldn’t show up in the filtered view, but nothing there either. Migrate is somehow eating these PHP errors from the log..?

They're probably in the infamous message tables, we need a way to expose them here in a structured way... or just pipe them to dblog.

COMMIT BLOCKER When a migration fails, the message is “Import of Drupal 7 comments failed”. Well, thanks a lot. ;) How did it fail, and where do I start debugging?

Good question. The reason it failed should have been emitted right before that message (almost certainly because a migration it was dependent on failed). Not sure why you didn't see "Migration d7_comment did not meet the requirements: [details here]" as the log message immediately before that one.

OBSERVATION Some of these seem to report false positives. The row at the top of my report, “Imported Drupal 7 node title label” does not have a corresponding failure row, yet obviously did not work because my site still has no nodes. :)

"Drupal 7 node title label" did most likely succeed - that's (if I'm not mistaken) is just about setting the title label on node types, it doesn't migrate the nodes themselves.

Anyway, clearly messaging in general needs attention - what should go on the batch page, what should go into the dblog, how to expose the further detail from migrate's message tables....

Bojhan’s picture

Mike one way to work on the messaging a bit more is to have them listed here, perhaps as a screenshot with the quoted text. Its quite hard to try and trigger all the situations in which messages could appear to help you in writing better messages.

I notice some of the spacing in webchicks screenshots are off, is this simply artefact of the screenshot or does it not vertically align?

mikeryan’s picture

Mike one way to work on the messaging a bit more is to have them listed here, perhaps as a screenshot with the quoted text. Its quite hard to try and trigger all the situations in which messages could appear to help you in writing better messages.

Some of the kinds of messages seen in Angie's examples are:

  • Simple progress messages. On the batch page as the migration is running you see things like "Imported Drupal 7 field configuration" and "Currently importing Drupal 7 filter format configuration". In the dblog (displayed at the end) you see "Importing Drupal 7 node title label" (this seems kind of pointless) and "Imported Drupal 7 node title label".
  • If a migration as a whole fails, something like "Import of Drupal 7 menu links failed" will appear as the process is running, and in dblog at the end.
  • The SQLSTATE "Unknown database" error is of course being thrown by the Database API, apparently install.php intercepts such errors and makes them look better.
  • "Missing bundle entity, entity type node_type, entity id story. (/Users/webchick/Sites/8.x/core/lib/Drupal/Core/Entity/EntityType.php:793)" is being thrown by the entity system. Since migration touches all sorts of APIs, pretty much any error that entity/configuration/field/etc. systems can generate can be exposed during migration.
  • Not seen here - if a migration depends on another migration which did not complete, there should be a message about that like
            $this->t('Migration @id did not meet the requirements. @message @requirements', array(
              '@id' => $this->migration->id(),
              '@message' => $e->getMessage(),
              '@requirements' => $e->getRequirementsString(),
            )), 'error');
    

    I don't know why that didn't show up on the batch page or in the log. I do know the message itself needs work.

I notice some of the spacing in webchicks screenshots are off, is this simply artefact of the screenshot or does it not vertically align?

Do you have a specific example? They look like I would expect.

Going back to bits I missed from Angie:

COMMIT BLOCKER We need a way to either re-do the migration or to clear it so you can start from scratch again.

RECOMMENDATION That error message is also super obtuse. Can we get that cleaned up to be more human-friendly? :) "Comments were already imported. Clear database and try again." or similar.

This form actually is performing two steps - it generates the appropriate migrations based on the source site and destination site configurations (e.g., if the book module isn't enabled on both sites, no book migration config entity will be created), and then it invokes the import operation on those generated migrations. So, we could theoretically offer to delete the existing migrations first, or to overwrite them, but what happens with any partial content/configuration already imported? Content could be rolled back, but no one's keeping track of the original configuration. At any rate, in what circumstances would the results of running it again differ? In this case, with the application of patches, but I don't think recovering from running with bleeding-edge patches is a use-case we need to cover. Indeed, I would argue that this is a backup-your-database-and-be-prepared-to-restore-it operation - it is a major version upgrade, after all.

The migrate UI is currently associated with "administer site configuration" but on many sites that's given out kind of willy-nilly. We should most likely base it around "Administer software updates" instead. (Generally-speaking, only User 1 should ever be using this.)

Done.

While untested (I'll test this as soon as I can get something to actually migrate ;)), the likely thing that happens when using this UI is that any existing content/users/etc. you have are going to be overridden with those from the old site. For example, user 1 was admin, now it's "root." Node 1 was "test" now it's "Welcome to our site."

Well, at the moment user 1 won't get touched, but we need to figure out how to handle it: #2560637: Improve handling of uid 1 during migration

Big, fat, undeniable red warning on this page (maybe even make "Perform upgrade" red instead of "friendly blue") if site has existing content, and what the consequences of that will be.

Backup all your shit! Now!

(dang, style="color: red" doesn't work)

Possibly in the installer, make this an explicit choice: "are you installing D8 to build from, or are you installing D8 to migrate to?" (This would be a separate issue, since there's a lot to contend with there.)

Yes, we discussed this in IRC - if the answer is "upgrading", we would install migrate_drupal_ui (or whatever it's called) and redirect straight to /upgrade at the end of the install process. Here's another consideration, however - if, say, that book module is enabled on the source site but not in D8, book hierarchies will not be migrated. It was an explicit decision early on that the upgrade process would not automatically enable modules in D8, but we do need a space somewhere for the site administrator to have the opportunity to do it. And they need info on what modules are at risk of not coming over, which we can't give them until they've submitted the credentials on the upgrade form...

mikeryan’s picture

So, along with the question of giving administrators information on what stuff won't migrate until the relevant D8 modules are installed, there's https://www.drupal.org/node/2562073#comment-10289183. With the PHP module out of core, we will be converting php_code filters to filter_null, meaning that content using a format with the php_code filter will not display. Big WTF for anyone using php_code who didn't carefully read the release notes.

I know we'd like to have the single form, single click, but I'm feeling more and more we need to have a confirmation page that does some analysis of the source and destination sites before executing the migration. This page might:

  1. List all source site modules that will be imported based on the current configuration.
  2. List any D8 modules not enabled for which there is source data to import (and allow them to be enabled directly from this page?).
  3. If the php_code filter is in use, warn that content using it will not display without either installing the contrib php module, or editing it after migration to set a different format and remove any PHP.

Thoughts?

webchick’s picture

Well, something like that sounds absolutely fantastic to me.

So page 1: Just your DB credentials / File locations.

page 2: "Howdy, partner. It looks like you're running Drupal 7, with 46 contributed modules. You should know the following things about your site:

- EVERYTHING YOU HOLD DEAR WILL BE OVERWRITTEN. MAKE SURE YOU HAVE BACKUPS.

- PHP Code blah blah
- Foo, Bar, Baz modules don't have migrations, their stuff won't be moved over.
- something else.

[Proceed!]

mikeryan’s picture

I think the best way to move forward on the various feedback/ideas is to work on them in the migrate_upgrade project, where they can be done in separate issues/patches, and the improvements made immediately available to people doing upgrades today. It should be fairly simple to script turning that module into a core patch (mv the files, do a few renames, rewrite some namespaces), so at appropriate checkpoints I can post an updated patch here.

webchick’s picture

Issue summary:View changes
Status:Needs work» Postponed

Yep, agreed.

mikeryan’s picture

OK, I've backported all the work I've done here (plus a little more message tweaking) to migrate_upgrade). Next steps:

  1. Create individual issues for outstanding topics above, parented to this issue.
  2. Write a script for regenerating this core patch from the current migrate_upgrade module.
  3. Use that script at appropriate checkpoints to update the patch in this issue.
mikeryan’s picture

Attached is a core patch generated from the current status of migrate_upgrade - following that project is really the best way to keep up with the evolving upgrade UI, but I can easily regenerate the core patch here periodically.

The most interesting issues being pursued at the moment are #2567119: [meta] What to do when upgrade has already been done? and #2562849: [meta] Confirmation page - in particular, see webchick's feedback at https://www.drupal.org/node/2565159#comment-10329499.

tl;dr, the original idea of a single page enter-credentials-and-boom! is off the table now. The steps involved now look like:

  • First attempt at upgrade:
    1. Enter db credentials/file location
    2. Review, showing what's being upgraded (and what's not being upgraded).
    3. Run the upgrade process and report on it. Currently the reporting is just dblog messages - perhaps it should be something more like the current migrate_tools UI (below)?
  • Subsequent returns to the upgrade form:
    1. Show status of migrations like the current migrate_tools UI?
    2. Recognizing that the upgrade has been previously performed, ask whether it should be rolled back and redone from scratch, or an incremental migration should be done.
      • If the former, roll back migrated content (simple configuration would not be restored), remove migration configuration, and regenerate the configuration and rerun from scratch like the first time.
      • If the latter, create applicable migrations that didn't already exist (don't touch existing migration configuration). Run the entire migration process - existing migrations will pick up any new content, new ones will get all content.

So, the general-purpose dashboard we currently have in the migrate_tools submodule of migrate_plus looks like this:

Of particular note is the Messages column, which links to a list of migration-specific messages (e.g., for a file migration that source file IDs 71 and 189 failed to copy over). For the upgrade case, we would have a simplified version of this - dropping the machine name, status, operations, and perhaps last imported columns...

I'm really a backend developer - this all will go much quicker with help from a front-end developer, and especially with designers actively participating in refining this UI.

(edit: moved stuff belonging here that was accidentally put into the issue summary)

mikeryan’s picture

Issue summary:View changes
webchick’s picture

First, copying over my comment from #2565159: Create basic confirmation page for better visibility...

It's important here to take a step back and look at the UI we're replacing. That UI is update.php. Update.php is not like a shining pinnacle of UX design, but it does accomplish the following things:

0) If something is up that's going to cause you problems down the road, it warns you about that right away and stops you from proceeding:

Error about not having access to run the script.

1) It first warns you about what the process you're about to engage in, and how to take proper precautions.

First screen explains the whys and wherefores and recommends backing up.

2) Next, it shows you what it's about to do and gives you a chance to review it before proceeding:

Shows two pending updates.

3) It performs operations in a batch...

Batch API shows 2 items processed

4) If there were problems, it tells you what happened, with something you can go and Google more to figure out...

Update #8005 failed, with error message.

5) You're able to review detailed logs to determine even more about what happened in order to help with said Googling...

In addition to update errors, shows some DrupalKernel errors which may be a factor.

6) Once you (think you) have fixed your problem you're free to attempt the update again. It will remember what things didn't work and allow you to re-try them:

Shows failed 8005 update again.

7) Repeat until you get a green board (see no more error messages) and then you know you are done:

No errors.

webchick’s picture

Next, let's show screenshots of the current UI as of right now, which I reviewed with @LewisNyman and @yoroy today:

The first screen prompts for your source database credentials, and then the location to your files, either from the public-facing website itself or from a local directory:

Two sections for source database / source files.

This screen already got reviewed in quite a bit of detail above, and I believe all feedback has now been addressed (e.g. we use "upgrade" consistently in the text, etc.). The one thing that Lewis pointed out is that the page title (not pictured), "Drupal Upgrade: Source site information" is non-standard from others, and should probably be shortened to just "Drupal Upgrade" (I think even I could make that patch, so NBD :D).

Pressing the button leads you to an overall summary of what's about to happen:

Source module / destination module / description columns

Two things came up in review:

1) That "Are you sure?" warning should be moved to a standard confirmation form pattern when clicking the "Perform upgrade?" button.

2) The blank cells here are "concerning" (via yoroy) and they also mean two different things: a) there's a bug, or b) it's intentional, e.g. Block content type / Block content body field configuration both relate to block_content migration so don't bother repeating the info. For this purpose, re-utilzing the "table grouping" we use on e.g. the Permissions page is probably the better pattern to employ here:

List of permissions grouped by module name

At the bottom of the form you'll see a variety of rows like this:

No upgrade detected

These need to be styled in error/warning (similar to Status report page) because THIS IS WHERE YOU WILL LOSE DATA. And right now they look like "FYI" kind of rows.

When you click the button you get a batch API screen:

Progress bar with messages

Both Lewis and yoroy were perplexed at why the messages are showing above vs. below the bar. If that's a quick fix, would be great cos atm the bar moves around which is unexpected. (It does make trying to take screen shots fun, though. ;))

Once the upgrade is complete, you'll see a message like this:

Green message saying 44 migrations completed.

One thing I noticed is that if PHP errors or something not specific to migrate were encountered, those will not be flagged here, giving the mistaken impression that everything's hunky-dory when it's not.

You can review the log for migrate-specific messages:

DBLog filtered down to migrate_upgrade errors

No comment, that all looks fine.

If you attempt to re-run the migration, this is what you see:

Migrate entity 'd7_block' already exists.

We need to fix that error message so that it's intelligible. Quick fix suggestion: "A site upgrade has already been attempted. Please restore from backup to try again." Longer fix suggestion: Let me re-run it as many times as I need to, like update.php.

webchick’s picture

Finally, let's talk about the real sticking point (at least for me), which is... how do I know when I am done?

The migration might've been successful, in that it shows no errors, but the key thing is that unless I'm sure that all of the modules on my old site have been accounted for, I'm not really done. I need to know what's missing so that I can go and download extra modules (e.g. php_code filter needs php module in contrib now).

And if there are missed things, unlike with update.php, I have no opportunity to re-run the upgrade again without starting over from scratch. That by itself is very similar to major version upgrades in update.php, so not a huge deal. However. The two of them together mean you can end up totally hosed. If you mistakenly believe you are done, because it shows you some lovely green status message at the end, and then you launch your new D8 site and people start adding comments, nodes, etc. to it, and then 3 weeks later you're like "oh crap I forgot about the Donkey module" your only recourse is to restore from a 3 week old backup (which means data loss) or start futzing around manually with the DB tables (which means hiring Mike Ryan ;)).

And this "when am I done?" question is really, really tricky to figure out programmatically. Because all Migrate Upgrade has to go on is the source database, aka the system table in D6/7. Here's an excerpt from mine: (viva PHPMyAdmin)

Excerpt from system table

The system table can tell you some useful things, like the machine names of all modules that were on your old site, whether they were enabled or not, and the schema version. Depending on what this table says about you, you might or might not be hosed when you click that "Perform upgrade" button:

  1. If a module was on the old site and has a destination migration on the new site, we don't need to care. Done.
  2. If a module's status is 0 (disabled) and its schema version is -1, it was never installed and we do not care about it at all (contact module). Done.
  3. If a module's status is 0 and its schema version is not -1, then you have a disabled module that may or may not define data and that is horrible. Recommend skipping these too and logging an error.
  4. If a module's status is 1 that means it was enabled in the old database and we need to care about it, or else we risk losing data.
  5. If an enabled module has a schema version > 0 (like 7009 in the case of comment module), that almost guarantees that we will lose data if we cannot find a corresponding destination module. Those should be flagged as a red error with blink tag and capital letters. ;)
  6. If an enabled module has a schema version of 0, we have no effing idea what that means. It could mean that the module defines no data and we can chirp along happily without a migration path (contextual module). Or it might mean that the module defines a table schema and/or variables and just has never needed a hook_update_N(). We have no idea, so we should flag these as warnings, at least.

That last point is particularly bad, because the only way to figure out if it's "NBD" or "OMG data loss" is to:

  • Go into the old module directory and look for a something.install file.
  • If present, check for hook_schema(). If it's there, you're definitely going to lose data.
  • Also grep anywhere in the module at all for a variable_get() call. If present, and the module is the one controlling that data, you're also going to lose data.
  • Probably other things I am not even thinking about.

And this is completely unreasonable to expect the target audience of this UI to do. (Heck, I'm not sure even I could do that as a Drupalist with 10 years of experience and with PHP/SQL chops.)

I don't know a solution to how to tell if you are "done" beyond something like logging all of the tables in the source database, logging all of the tables that migrations in the new site know about (this would have to be a YAML property so it could be checked prior to modules being enabled) and then showing this information in a Debug window or something.

But I definitely foresee lots of people really, really hosing themselves if we ship with a UI that doesn't know how to answer this question. :(

yoroy’s picture

Issue summary:View changes
FileSize
40.27 KB

This is my understanding of the upgrade process:

screenflow for performing an upgrade

  1. Database credentials, files path etc.
  2. A pre-upgrade report on upgrades paths found and *not* found
  3. A confirmation screen: are you sure you want to start the upgrade
  4. The upgrade-in-progress screen
  5. A report on what happened + option to start over again

Webchick and I discussed mostly what we should be doing at step 2, see #57 for the different scenarios.

webchick’s picture

Ok, after discussing in IRC with @mikeryan and @yoroy...

1) Forget about advanced debugging (e.g. trying to divine whether a missing destination is a problem or not from the system table, listing table names that are unmigrated, etc.) As enumerated in #57 this is a road to pain and tears and bees.
2) Similarly, forget about warning vs. error distinction for missing destinations. A destination for a source module is missing? It's an error.
3) Allow re-running the UI as many times as you wish.
4) Consult the map tables and only migrate stuff not already accounted for, displaying the list of remaining tasks (like update.php does). This way, if you see something you don't expect you have an opportunity to abort before your data is overridden by your old site's info.

This seems reasonable to me and gets rid of the requirement to know if you're done. I still worry people won't be vigilant enough about reviewing what's about to be migrated in subsequent migrations but same problem exists for CMI.

yoroy’s picture

FileSize
32.55 KB

Updated flow:

- Add an initial screen explaining "lets do this, here's how to prepare", similar to screenshot 1 in #55
- An extra confirmation page is maybe not so necessary after all. Especially if you can safely run the upgrade process multiple times.

For the pre-upgrade report, maybe what is now in the 3rd column (data to be upgraded) could be shown in the pattern used on the Extend page:

We'll have to think more about what the post upgrade screen could look like. Also, running multiple times means keeping track of what was already upgraded and what not. Should that be made visible in the UI?

mikeryan’s picture

All right, since I find trying to figure out what the current consensus in a long, complex meta issue like this the equivalent of understanding a PHP class given only a sequence of commit diffs;-), I've tried to pull together the last couple days' worth of discussion into a spec at https://docs.google.com/document/d/1Mf-IsowOnQNVuW7xYeNjvX2t2J-rBw3bff-y.... I'd like to nail that down so it contains only the consensus current design, particularly as a guide to Barcelona sprinters, but right now there are a lot of questions in there best discussed here in this issue.

Of course, discussing a dozen questions at once in a single issue is chaos... So first, right now, my immediate priority is to implement the incremental migration scenario ("Returning to the upgrade page", last major section of the doc) - simply performing an incremental migration if you go through the credentials page again would be simple enough, but we need some feedback to the administrator first, and it's not clear to me exactly what/how to provide that. My thought at the moment is that you would basically dive right into the pre-upgrade review screen, which would now be enhanced with the status of the previously-run upgrades (green for "nothing new here", yellow for "new content will be imported by this one", ? for "here's a new upgrade path you didn't have before"...). Thoughts?

Also, the last couple of days we haven't discussed the rollback option (throw it all out and start over)...

Thanks.

mikeryan’s picture

*crickets*

Anyway, I've tagged a bunch of migrate_upgrade issues for Barcelona. Until/unless I hear otherwise, I'm following what I've written up in the Google doc. The hope is that at Barcelona better front-end developers than I can dig into the the UI details, before then the issues I want to address are:

Bojhan’s picture

Status:Postponed» Active
Bojhan’s picture

I've read through the various issues, it looks solid to me. It seems to touch upon everything discussed here. We might need to discuss our ability to remove the extra confirmation screen, as less steps is probably better.

I am looking forward to having this in core, and iterating over it.

Can we update the issue summary, and get the understanding from the doc in place? It would be nice to be able to direct people to the correct followups

mikeryan’s picture

At Bojhan's suggestion, created a child issue summarizing UI issues tagged for Barcelona (since they're in the contrib migrate_upgrade module, they aren't visible to people searching the tags in the core queue): #2573403: [meta] Outstanding tasks for upgrade UI.

mikeryan’s picture

Issue summary:View changes

Updated issue summary.

mikeryan’s picture

Haven't refreshed the core patch lately, so here it is (along with the script I use to move the contrib migrate_upgrade module into core). Note that, besides the UI work that's been going on intermittently in contrib, with this version the module is being renamed Drupal Upgrade/drupal_upgrade. Or, should it be simply Upgrade?

catch’s picture

Status:Active» Needs work

Updating to show there's a patch.

Gábor Hojtsy’s picture

Here is a video of how this looks like live with a Drupal 7 migration. Since that is not complete yet, this also demonstrates error display:

https://drive.google.com/file/d/0B6xsrc5BVkagMjdrTHZ3R2o0YXM/view?usp=sh...

One can also rerun migrations either additionally to what was already run or after starting over. I did not put that into the video, however if you go to /upgrade, this is what you see.

Picking re-run goes directly back to the available/missing upgrades list. Picking rollback asks for confirmation:

Once confirmed, it rolls things back in a batch (similarly displayed as the migration itself with a rolling list of notes on what is going on). And then ends up on the start screen of the migration ui:

Gábor Hojtsy’s picture

Version:8.0.x-dev» 8.1.x-dev
Status:Needs work» Needs review
FileSize
62.9 KB

Rolled a new version of the patch with the script in #67, thanks. Moving to 8.1 for testing.

Status:Needs review» Needs work

The last submitted patch, 70: 2281691-70.patch, failed testing.

xjm’s picture

The hook_help() failure is just about having a standard format of help text on the help page; I'll submit a fix for that.

xjm’s picture

xjm’s picture

Status:Needs work» Needs review
FileSize
892 bytes
1.03 KB

It turns out an hour at the gym is not long enough for InstallUninstallTest to complete on my laptop, so here's a combined patch to check whether the failure is fixed.

xjm’s picture

FileSize
62.97 KB

Fail. Actual combined patch.

The last submitted patch, 74: migrate-2281691-74.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 75: migrate-2281691-76.patch, failed testing.

Gábor Hojtsy’s picture

Looks like the only other fail now is the missing composer.json entry.

xjm’s picture

Status:Needs work» Needs review
FileSize
63.43 KB
479 bytes

Here's a fix for that too. I guess we could either add this to the script or apply manually.

xjm’s picture

Started a first pass code review. Here are a few quick things I think we should probably fix prior to core inclusion. Probably we can create issues for these in the drupal_upgrade queue.

  1. +++ b/core/modules/drupal_upgrade/src/DrushLogMigrateMessage.php
    @@ -0,0 +1,28 @@
    +    drush_log($message, $type);
    

    So I'd expect to see some dependency for this injected here. Assuming this code only runs if Drush is available?

  2. +++ b/core/modules/drupal_upgrade/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1178 @@
    +   * @todo: Find a mechanism to derive this information from the migrations
    +   *   themselves.
    +   *
    +   * @var array
    +   */
    +  protected $moduleUpgradePaths = [
    

    I believe there is an existing issue for this; so I think it makes sense to go forward with this still outstanding. Probably should link that issue in the @todo. Would be good to add a little more documentation for this since it's such an important part of the functionality atm.

  3. +++ b/core/modules/drupal_upgrade/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1178 @@
    +      $desc = "The following items will not be upgraded. " .
    +        'For more information see <a href="https://www.drupal.org/upgrade/migrate"> Upgrading from Drupal 6 or 7 to Drupal 8</a>.';
    ...
    +        '#description' => $this->t($desc),
    ...
    +            '#plain_text' => t($destination_module),
    ...
    +        "</li><li>" . t($missing_count . ' missing upgrade paths') . "</li></ul>",
    
    +++ b/core/modules/drupal_upgrade/src/MigrateUpgradeRunBatch.php
    @@ -0,0 +1,360 @@
    +            $singular_message = 'Upgraded @migration (processed 1 item total)';
    +            $plural_message = 'Upgraded @migration (processed @num_processed items total)';
    ...
    +            $singular_message = 'Rolled back @migration (processed 1 item total)';
    +            $plural_message = 'Rolled back @migration (processed @num_processed items total)';
    ...
    +          $message = \Drupal::translation()->formatPlural(
    +            $context['sandbox']['num_processed'], $singular_message, $plural_message,
    +            ['@migration' => $migration_name, '@num_processed' => $context['sandbox']['num_processed']]);
    ...
    +            $singular_message = 'Continuing with @migration (processed 1 item)';
    +            $plural_message = 'Continuing with @migration (processed @num_processed items)';
    +          $context['sandbox']['messages'][] = \Drupal::translation()->formatPlural(
    +            static::$numProcessed, $singular_message, $plural_message,
    +            ['@migration' => $migration_name, '@num_processed' => static::$numProcessed]);
    ...
    +          $message = 'Currently upgrading @migration (@current of @max total tasks)';
    ...
    +          $message = 'Currently rolling back @migration (@current of @max total tasks)';
    ...
    +        $context['message'] = t($message,
    +          ['@migration' => $migration_name, '@current' => $context['sandbox']['current'],
    +           '@max' => $context['sandbox']['max']]) . "<br />\n" . $context['message'];
    

    These are an incorrect uses of t(); l.d.o cannot find the string this way to make it translatable, and it also is a potentially unsafe pattern for contrib to copy.

  4. +++ b/core/modules/drupal_upgrade/src/MigrateUpgradeRunBatch.php
    @@ -0,0 +1,360 @@
    +        drupal_set_message(t('Completed @count successfully.', ['@count' => $translation->formatPlural($successes, '1 upgrade task', '@count upgrade tasks')]));
    ...
    +        drupal_set_message(t('Completed @count successfully.', ['@count' => $translation->formatPlural($successes, '1 rollback task', '@count rollback tasks')]));
    ...
    +        drupal_set_message(t('@count failed', ['@count' => $translation->formatPlural($failures, '1 upgrade', '@count upgrades')]), 'error');
    +        drupal_set_message(t('Upgrade process not completed'), 'error');
    ...
    +        drupal_set_message(t('@count failed', ['@count' => $translation->formatPlural($failures, '1 rollback', '@count rollbacks')]), 'error');
    +        drupal_set_message(t('Rollback process not completed'), 'error');
    

    I think using formatPlural() within t() like this also can be problematic to translate (in some languages the translation of "failed" would be different depending on the number). So I think these also need to be just formatPlural().

xjm’s picture

@Dries, @webchick, @effulgentsia, @alexpott and I discussed this issue today and agreed that we should add this to core as an alpha-stability experimental module for 8.1.x. There are still other Migrate critical issues that need to be resolved before any of Migrate can enter a release candidate, but it's okay (and expected) to have these criticals outstanding as we enter an alpha phase. (For more information, see https://www.drupal.org/core/experimental.)

We have until the beta begins on March 2 to get this in, and I think we are on track! Thanks especially to @Gábor Hojtsy, @mikeryan, and @abhishek-anand who have helped get this issue on track for 8.1.x.

The next step is to agree what exactly needs to be done to get this into core as an alpha experimental module. Since this is the first such module for a minor version, we'll need to make some decisions as we go. Here are the areas we should discuss:

  1. Usability and accessibility gates: I believe the current user interface is of sufficient quality for an alpha experimental module, and we can iterate on it each month according to the experimental policy as we get feedback and testing. We should have final usability and accessibility reviews of the UI before it becomes a stable module, but we have until at least 8.2.x for that.
  2. Documentation gate: @Gábor Hojtsy has already done a lot of work on the documentation in the handbook, which is linked from the hook_help() in the required standard. In terms of the API documentation, there are a few minor missing minimum documentation requirements that I think we could work on in core during the beta and RC phases for 8.1.x itself.
  3. Testing gate: For me, this is the biggest area of concern right now. There is no test coverage for the UI as part of the patch. Are we willing to add an alpha module with no test coverage? Seems risky. I don't think we need to have full test coverage for adding an alpha experimental module, but I do think we should have some coverage of the most basic functionality. #2647470: Write tests is the issue for that and has a start at test coverage, so this should be fixed soon.
  4. Security: The review above highlights a few misuses of t() that are certainly not security issues themselves since the source strings are all known or from the info files of other modules, but we should not commit them to core since it is important for core to demonstrate the secure use of t(). Those things should be easy to fix. Outside of that, the functionality should only ever be provided to trusted users covered by the "administer software updates" permission as in the patch.
  5. Data integrity: #2570529: Delay actually saving migration entities until ready to run is an important outstanding issue for being able to recover from bad migrations. However, as @alexpott pointed out, Migration is always a safe operation, because no matter how wrong it goes you can always fall back to the source site. One caveat that @webchick raised though is that there is a big risk of not knowing when some data does not have a needed migration path, and prematurely switching to the new D8 site with apparent data loss. (This was also an issue with update.php, and Migrate is actually much safer because unlike update.php it does not destroy the original site and data.) Still, since the D7 migration path is not stable or complete, we should add warnings about the D7 migration path to the UI before we add this to core.
mikeryan’s picture

So I'd expect to see some dependency for this injected here. Assuming this code only runs if Drush is available?

DrushLogMigrateMessage.php should be excluded from the core patch by the corify script, I'm updating that. Note that the contrib migrate_upgrade module contains both the UI slated for core, and the migrate-upgrade drush command, which should be submitted to be included in Drush itself at some point.

I believe there is an existing issue for this; so I think it makes sense to go forward with this still outstanding. Probably should link that issue in the @todo. Would be good to add a little more documentation for this since it's such an important part of the functionality atm.

(referencing the $moduleUpgradePaths array) - I'll fix this up in the contrib module.

These are an incorrect uses of t(); l.d.o cannot find the string this way to make it translatable, and it also is a potentially unsafe pattern for contrib to copy.

For l.d.o, we need to have the literal strings directly in the t() and formatPlural() calls, correct?

I think using formatPlural() within t() like this also can be problematic to translate (in some languages the translation of "failed" would be different depending on the number). So I think these also need to be just formatPlural().

So, if I understand the requirement correctly, this will kind of explode the code a bit - if each t/formatPlural must be fully expanded, that seems to require a lot of redundancy in the code, right? But, if there's no way around it to support translation... I'll see if I can do this tonight.

@Dries, @webchick, @effulgentsia, @alexpott and I discussed this issue today and agreed that we should add this to core as an alpha-stability experimental module for 8.1.x...

This all looks good to me.

One other thing, just want to get the list of committers to migrate_upgrade in here:

User Last commit First commit Commits
mikeryan 2 months ago 1 year ago 51 commits
benjy 11 months ago 11 months ago 16 commits
Ryan Weal 1 year ago 1 year ago 4 commits
cilefen 1 year ago 1 year ago 4 commits
er.pushpinderrana 1 year ago 1 year ago 3 commits
Nixou 10 months ago 10 months ago 2 commits
anavarre 1 year ago 1 year ago 1 commit
Palashvijay4O 1 year ago 1 year ago 1 commit
hussainweb 1 year ago 1 year ago 1 commit
xjm 1 day ago 1 day ago 1 commit
quietone 4 months ago 4 months ago 1 commit
Luukyb 4 months ago 4 months ago 1 commit
aleksip 4 months ago 4 months ago 1 commit
krh121791 1 year ago 1 year ago 1 commit
mikeryan’s picture

Speaking of formatPlural(), I see inconsistent results with single counts like...

Upgraded Search configuration (processed 1 item total)
...
Upgraded File configuration (processed 1 items total)

Any ideas why that might be happening?

mikeryan’s picture

Updated corify (extra credit: have corify do the edit to composer.json) and the patch itself with the above changes plus the menu_links fix.

mikeryan’s picture

Speaking of the menu_link fix, please add Jo Fitzgerald to the credits list.

mikeryan’s picture

FileSize
1.09 KB

Here's the new corify (didn't notice that d.o had silently ignored my upload without the .txt extension).

xjm’s picture

@mikeryan, thanks for posting the credits! In order to ensure the d.o credit is saved, it'd be great if all those folks could comment on this issue (because of #2474609: Not possible to credit people who didn't comment in an issue). I tweeted about this here: https://twitter.com/xjmdrupal/status/698203369446494209

xjm’s picture

andypost’s picture

Issue summary:View changes
Issue tags:+Needs screenshots

suppose summary should be updated with current UI

quietone’s picture

Screenshots attached. Did I miss anything?

Also, corify was failing for me, so I've uploaded my version. The fix was to change "-i .bak" to "-i.bak". I also put in some echos because I like feedback from scripts.

Bojhan’s picture

@xjm Great, that we are putting this in. I am a bit puzzeled by "Usability and accessibility gates: I believe the current user interface is of sufficient quality for an alpha experimental module, and we can iterate on it each month according to the experimental policy as we get feedback and testing." - could you explain what heuristics are use to establish a UI is of "experimental quality" ?

xjm’s picture

Thanks @Bojhan. This is based on this new core policy:
https://www.drupal.org/core/experimental
(The TLDR is, we put modules that are not perfect in core as experimental to iterate on them more quickly, based on the proposals for less painful feature and UI development.)

xjm’s picture

This issue is currently a blocker for the UI test coverage, which in turn blocks putting the UI in:
#2675000: SQLBase::mapjoinable does not support SQLite part 2

yoroy’s picture

For the UI I can only agree with @bojhan in #91. I started taking notes based on the video @Gábor Hojtsy posted in #69, most of those were about the interface texts. The task flow is in sufficient shape so UI shouldn't be a blocker.

yoroy’s picture

Issue tags:-Needs screenshots
xjm’s picture

Thanks @yoroy! Do you think we can work on the UI texts in some followups based on your notes? I think as the module itself will be alpha it's okay to have string changes for it during the 8.1.x beta.

cilefen’s picture

Random comment... ;-)

Luukyb’s picture

Just random comment following the message from xjm, this is getting closer to core!!

yoroy’s picture

Ah sorry, that was what I meant to say: the string changes can be followups :)

xjm’s picture

Updating credit; thanks @quietone, @Luukyb, and @cilefen for working on the contrib module!

er.pushpinderrana’s picture

Thanks xjm for sending out the notification to me, very appreciated. Just putting random comment here but will not miss the opportunity to say thanks to everyone who are making this close to core.

It shows a team spirit and a great example of collaborative work :).

hussainweb’s picture

Thanks @xjm for pointing me here.

xjm’s picture

So here is our current status to the best of my knowledge:

  1. #2647470: Write tests is a blocker. It is currently failing on SQLite (despite #2675000: SQLBase::mapjoinable does not support SQLite part 2 being committed to fix that). :(
  2. We decided #2570529: Delay actually saving migration entities until ready to run does not need to be blocking, because it can be fixed during the beta and will also potentially be resolved by the Migrate plugin issue. It can be included in the patch also though if the contrib module maintainers choose.
  3. Once that is done, we should post a current core patch to this issue for review.
  4. #2675006: Write UnitTest for MigrationCreationTrait can be addressed in the core queue during the beta or after.
  5. @alexpott had some concerns about #2675066: Restrict migrate upgrade permission to user1 being outstanding, but I think this would also be okay to fix during the beta or after.
  6. It looks like most of the previous issues with translations were addressed. I found a couple other small issues with them when reading the codebase but nothing blocking, just bugs that might result in double-escaping or problematic pluralization for some languages. Will get these posted once the patch is available and we can fix them in followup issues if needed. All can be fixed during beta or after.
  7. @alexpott also pointed out that having one single form builder for multiple different forms is a bit weird. I think it would be safe to split it into separate controllers during the beta as well.
  8. There are miscellaneous nonstandard docs things that can probably be fixed before or after commit, but nothing that was a glaring omission that I found yet.
  9. Followups TBD from @yoroy about the interface texts, which can go in during the beta or after since it is an alpha module.

Updating credit, also. Thanks @hussainweb and @er.pushpinderrana!

aleksip’s picture

Random comment, thanks @xjm and everyone involved!

anavarre’s picture

Thanks for the ping, @xjm

xjm’s picture

Updating credit, thanks!

alexpott’s picture

I've been working on several issues to get migrate UI in core for example... #2678564: A missing schema to support migrate UI - source_base_path and database_state_key and #2647470: Write tests

alexpott’s picture

After reviewing a lot of the code in https://www.drupal.org/project/migrate_upgrade and thinking about how it is tied into the assumptions made in the core migrate_drupal module, I think we should not be adding yet another module to core. Rather this should become part of the migrate_drupal codebase. The migrate_upgrade codebase basically contains a form, a drush command and a trait for both the form and the drush command to use to migrate a Drupal site. Obviously we're not going to be putting the drush commands in core so we're adding a form and trait to migrate a Drupal site through the UI. That would seem to be a pretty good fit with the existing migrate_drupal module especially since this module is designed to be turned off once the migration is done.

yoroy’s picture

Ryan Weal’s picture

Making a comment... thanks for the ping @xjm. Cool to see this is getting merged in!

xjm’s picture

Updating credit.

I am also in favor of the proposal in #109; I think it would require a change to the corification script and maintainer signoff though?

mikeryan’s picture

Yes, corify would need to be updated.

The proposal makes perfect sense purely from the point of view of the full upgrade path. However, we are going to want to provide tools for custom Drupal-to-Drupal migrations in contrib, which will need migrate_drupal but not the upgrade UI... Perhaps they could disable the /upgrade path?

xjm’s picture

Hmm, that sounds questionable. Maybe best to proceed with it as two modules. After all, Views UI does nothing without Views and is also coupled to its assumptions, etc.

tim.plunkett’s picture

Just a note, if you place the module in /modules and not in /modules/contrib as I do, then the corify script will fail on the cd, and then happily rm -Rf .git in your core checkout. Ouch.

Here is a newly generated patch from the 8.x-1.x branch for review purposes.

tim.plunkett’s picture

  1. +++ b/core/modules/drupal_upgrade/src/Controller/MigrateController.php
    @@ -0,0 +1,24 @@
    +<?php
    +/**
    ...
    +  }
    +}
    

    (minor) Missing blank line

  2. +++ b/core/modules/drupal_upgrade/src/Controller/MigrateController.php
    @@ -0,0 +1,24 @@
    + * Contains Drupal\drupal_upgrade\Controller\MigrateController.
    

    Missing leading slash

  3. +++ b/core/modules/drupal_upgrade/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1196 @@
    +class MigrateUpgradeForm extends FormBase implements ConfirmFormInterface {
    

    Any reason this doesn't extend ConfirmFormBase directly?

    Though it seems some of these methods like getCancelUrl(), getDescription(), getConfirmText(), and getCancelText() are not used, but implemented. Only getFormName() and getQuestion() seem to be used.

  4. +++ b/core/modules/drupal_upgrade/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1196 @@
    +   * @todo: Hardcoding this information is not robust - the migrations
    +   * themselves should hold the necessary information.
    +   * @see https://www.drupal.org/node/2569805
    

    Yikes, that's a big array.

    Subsequent lines of an @todo are indented two extra spaces. If this link is the issue to fix the @todo, it should just be a part of the sentence, not a separate @see. If it is not the issue to fix it, create one and link it here in the sentence. Also no colon after @todo.

  5. +++ b/core/modules/drupal_upgrade/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1196 @@
    +        drupal_set_message($this->t('Unrecognized form step @step',
    +          ['@step' => $step]), 'error');
    

    No need to wrap this, please but it on one line.

  6. +++ b/core/modules/drupal_upgrade/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1196 @@
    +    if ($date_performed = \Drupal::state()->get('drupal_upgrade.performed')) {
    ...
    +          ['@date' => \Drupal::service('date.formatter')->format($date_performed)]),
    ...
    +      $query = \Drupal::entityQuery('migration', 'OR');
    ...
    +          if ($database = \Drupal::state()->get($source['database_state_key'])['database']) {
    ...
    +      $form_state->setErrorByName($database['driver'] . '][0', \Drupal::service('renderer')->renderPlain($error_message));
    ...
    +      $query = \Drupal::entityQuery('migration');
    ...
    +      $migrations = \Drupal::entityManager()
    +         ->getStorage('migration')
    ...
    +      \Drupal::state()->delete('drupal_upgrade.performed');
    ...
    +      \Drupal::state()->set('drupal_upgrade.performed', REQUEST_TIME);
    

    These should be injected. Also, entityManager is deprecated.

  7. +++ b/core/modules/drupal_upgrade/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1196 @@
    +    // Make sure the install API is available.
    +    include_once DRUPAL_ROOT . '/core/includes/install.inc';
    ...
    +    $drivers = drupal_get_database_types();
    ...
    +  protected function getDatabaseTypes() {
    

    This should use the protected method.

  8. +++ b/core/modules/drupal_upgrade/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1196 @@
    +    /*
    +    // @todo: Not yet implemented, depends on https://www.drupal.org/node/2547125.
    +    $form['files']['private_file_directory'] = [
    +    '#type' => 'textfield',
    +    '#title' => $this->t('Private file path'),
    +    '#description' => $this->t('To import private files from your current Drupal site, enter a local file directory containing your files (e.g. /var/private_files).'),
    +    ];
    +     */
    

    This code is incorrectly formatted, and we usually don't commit uncommented code.

  9. +++ b/core/modules/drupal_upgrade/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1196 @@
    +          if (substr($migration_tag, 0, 7) == 'Drupal ') {
    

    ===

  10. +++ b/core/modules/drupal_upgrade/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1196 @@
    +    try {
    +
    ...
    +    catch (\Exception $e) {
    

    This is a MASSIVE try block. Which calls are we catching exceptions for? Why can't they be individually wrapped with classed exceptions?
    Also there is a blank line.

  11. +++ b/core/modules/drupal_upgrade/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1196 @@
    +    $rollback = $form_state->getValue('upgrade_option') == static::MIGRATE_UPGRADE_ROLLBACK;
    +    if ($rollback) {
    

    One line, please

  12. +++ b/core/modules/drupal_upgrade/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1196 @@
    +    else {
    +      $table_data = [];
    +      $system_data = [];
    +      foreach ($form_state->get('migration') as $migration) {
    

    This is a very large else block. Can this function be rewritten to avoid that, or broken into smaller methods?

  13. +++ b/core/modules/drupal_upgrade/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1196 @@
    +    // By default, render the form using theme_confirm_form().
    +    if (!isset($form['#theme'])) {
    +      $form['#theme'] = 'confirm_form';
    +    }
    

    This is an odd way to do this, what other #theme would be here?

  14. +++ b/core/modules/drupal_upgrade/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1196 @@
    +      /** @var MigrationInterface[] $migrations */
    

    This needs to be a FQCN

  15. +++ b/core/modules/drupal_upgrade/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1196 @@
    +          'Drupal\drupal_upgrade\MigrateUpgradeRunBatch',
    

    You can use MigrateUpgradeRunBatch::class here, and anywhere else this refers to class names as strings

  16. +++ b/core/modules/drupal_upgrade/src/MigrateUpgradeRunBatch.php
    @@ -0,0 +1,358 @@
    +  public static function run($initial_ids, $operation, &$context) {
    

    This is another very large method that does a lot. Any way to break it up?

  17. +++ b/core/modules/drupal_upgrade/src/MigrateUpgradeRunBatch.php
    @@ -0,0 +1,358 @@
    +        \Drupal::service('event_dispatcher')->addListener(MigrateEvents::POST_ROW_SAVE,
    ...
    +        \Drupal::service('event_dispatcher')->addListener(MigrateEvents::MAP_SAVE,
    ...
    +        \Drupal::service('event_dispatcher')->addListener(MigrateEvents::IDMAP_MESSAGE,
    ...
    +        \Drupal::service('event_dispatcher')->addListener(MigrateEvents::POST_ROW_DELETE,
    ...
    +        \Drupal::service('event_dispatcher')->addListener(MigrateEvents::MAP_DELETE,
    

    Can you put the event dispatcher into a local variable?

  18. +++ b/core/modules/drupal_upgrade/src/MigrateUpgradeRunBatch.php
    @@ -0,0 +1,358 @@
    +          [get_class(), 'onPostRowSave']);
    

    Use static::class instead of get_class()

  19. +++ b/core/modules/drupal_upgrade/src/MigrateUpgradeRunBatch.php
    @@ -0,0 +1,358 @@
    +          $context['message'] = t('Currently upgrading @migration (@current of @max total tasks)',
    +              ['@migration' => $migration_name, '@current' => $context['sandbox']['current'],
    +                '@max' => $context['sandbox']['max']]) . "<br />\n" . $context['message'];
    ...
    +          $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'];
    

    These are oddly formatted. Also, doesn't appending the message to the end of a translated string mess this up?

  20. +++ b/core/modules/drupal_upgrade/src/MigrateUpgradeRunBatch.php
    @@ -0,0 +1,358 @@
    +    if ($event->getLevel() == MigrationInterface::MESSAGE_NOTICE ||
    +        $event->getLevel() == MigrationInterface::MESSAGE_INFORMATIONAL) {
    ...
    +    $message = t('Source ID @source_id: @message',
    +      ['@source_id' => $source_id_string, '@message' => $event->getMessage()]);
    

    One line each please

  21. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -0,0 +1,205 @@
    +trait MigrationCreationTrait {
    

    It's not ideal for a trait to call out to \Drupal::service().

    Ideally it should have abstract protected methods for each service, or even concrete protected methods wrapping the service. Maybe even while assigning it to a protected property.

  22. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -0,0 +1,205 @@
    +   * @param \Drupal\Core\Database\Database $database
    ...
    +  protected function getConnection(array $database) {
    ...
    +   * @param \Drupal\Core\Database\Database $database
    ...
    +  protected function getSystemData(array $database) {
    ...
    +   * @param \Drupal\Core\Database\Database $database
    ...
    +  protected function getMigrationTemplates(array $database, $source_base_path) {
    

    Docs clash. Is it an array or an object?

  23. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -0,0 +1,205 @@
    +    catch (\Exception $e) {
    ...
    +      throw new \Exception('Source database does not contain a recognizable Drupal version.');
    

    Is there a classed exception to catch or throw instead? Using \Exception is not as ideal

  24. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -0,0 +1,205 @@
    +   * @param string $migration_templates
    ...
    +  protected function getMigrations(array $migration_templates) {
    

    Array or string?

  25. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -0,0 +1,205 @@
    +        if ($migration->getSourcePlugin() instanceof RequirementsInterface) {
    +          $migration->getSourcePlugin()->checkRequirements();
    +        }
    +        if ($migration->getDestinationPlugin() instanceof RequirementsInterface) {
    +          $migration->getDestinationPlugin()->checkRequirements();
    

    These methods are likely idempotent, but for these instanceof checks to be recognized by an IDE you need to assign them to a local variable first.

  26. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -0,0 +1,205 @@
    +  protected function createMigrations(array $migration_templates) {
    

    Missing @retun

Many of the class/method docs don't start with a verb ending in "s", and the first line is often over 80 characters.

tim.plunkett’s picture

Addressed much of #116 in #2679016-2: Misc cleanups

tim.plunkett’s picture

Just a disclaimer:
My quick fixes to some of the issues I saw are not, in my opinion, enough to pay down the outstanding technical debt in this module.

xjm’s picture

Title:[meta] User interface for migration-based upgrades» User interface for migration-based upgrades

Alright:

Therefore, the next steps are:

  1. Generate a new core patch using the corify script and post it here. (Be careful, it may delete your .git directory if you do not put the module in modules/contrib or something like that. You might not want to use a core repo with your own feature branches etc.)
  2. Perform a (hopefully final) manual test of a migration.
  3. Ensure followup issues are filed in https://www.drupal.org/project/issues/migrate_upgrade for feedback not yet addressed. (These issues will be moved to the core queue if this issue is committed.)

If you'd like to see this UI in 8.1.0, please help with these steps within the next 24 hours.

Status:Needs review» Needs work

The last submitted patch, 115: 2281691-migrate-115.patch, failed testing.

alexpott’s picture

Status:Needs work» Needs review
FileSize
1.47 KB
77.13 KB

I've fixed the corify script to not continue on if stuff fails and therefore remove .git directories...

Also I've changed the module name to migrate_drupal_ui because this maintains the patterns we have elsewhere... and reading the two descriptions in the modules' info.yml files makes this seem obvious...

description: 'UI for direct upgrades from older Drupal versions.'

description: 'Contains migrations from older Drupal versions.'

I still think that this should be one module - I don't think

However, we are going to want to provide tools for custom Drupal-to-Drupal migrations in contrib, which will need migrate_drupal but not the upgrade UI... Perhaps they could disable the /upgrade path

from #113 is a really a compelling argument from them not to be one module. In fact I wouldn't be surprised if these custom Drupal-to-Drupal migration module might want to extend the UI introduced here.

I also think the current display name of the module name: Drupal Upgrade is misleading. This new module only provides a UI for this. It's like calling 'Views UI' just 'Views'.

Status:Needs review» Needs work

The last submitted patch, 121: 2281691-121.patch, failed testing.

alexpott’s picture

Status:Needs work» Needs review
FileSize
77.59 KB
471 bytes

Fixing the test fail... and the interdiff is the patch that needs to be applied to core/composer.json

alexpott’s picture

Here's a better corify script that doesn't hard code the location of migrate_upgrade and applies a patch to core/composer.json.

Status:Needs review» Needs work

The last submitted patch, 123: 2281691-123.patch, failed testing.

stevepurkiss’s picture

Issue summary:View changes
FileSize
297.88 KB

"Generate a new core patch using the corify script and post it here." ~ not sure how to do that but been working on upgrading my purkiss.com for a while now and saw the call-out to test so set up 8.1 and ran the script which output:

Remove drush-only files, and git.
Rename the files.
Put the shared trait into migrate_drupal.
Replace module name references (namespaces etc.) in all migrate_drupal_ui files.
Fix up the trait.
 and references to the trait
Cleanup

Then I ran the upgrade which went swimmingly. All the issues it picked up are specific to my site but all the main things like blogs it picked up fine:

Definitely more picked up this time round - am so impressed with the work done on this, thanks and hope it gets into 8.1!

mikeryan’s picture

I also think the current display name of the module name: Drupal Upgrade is misleading. This new module only provides a UI for this. It's like calling 'Views UI' just 'Views'.

There is a difference here - Views UI is a UI for all Views functionality. The underlying API here, migrate_drupal, is a general framework for Drupal-to-Drupal migrations - the migrate_upgrade(_ui) module implements the specific upgrade use case, and provides a UI for it (hence the trait, which provides the upgrade functionality for both this UI and the corresponding drush command).

I'm not really opposed to renaming it. Just another reminder that the core migrate/migrate_drupal modules are not there *only* for Drupal upgrades...

xjm’s picture

Status:Needs work» Needs review

So https://www.drupal.org/pift-ci-job/197873 is being caused by #2679547: Fix random fail in ConfigImportAllTest caused by ModuleInstaller not rebuilding routes immediately, which happens to be surfaced by BigPipe and is not unique to this issue; therefore, we should not block this on that. Retesting to see if we get a passing run.

alexpott’s picture

re #127/@mikeryan but the trait is being moved to migrate_drupal by the corify script... I didn't do that - in fact that was being done in the first corify script you added to this issue.

mikeryan’s picture

Ah, right, forgot about that. I put the upgrade trait in migrate_drupal, because if you're using the drush command you don't need the UI. The edges of the scope are a bit fuzzy here...

alexpott’s picture

Created #2679739: Make the migrate_drupal_ui module part of the migrate_drupal module to discuss the migrate_drupal/_ui merge as a followup.

xjm’s picture

Alright, thanks @alexpott! And thanks @stevepurkiss for testing.

Starting now, let's use the provided patch as the canonical version of this change.

I think we still need followups for @tim.plunkett's feedback about method length and logic flow in a few places.

xjm’s picture

Alright, I've performed a code review of the full patch. Most or all of this can be followups if needed.

  1. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -0,0 +1,222 @@
    +   * @param array $database
    +   *   Database array representing the source Drupal database.
    ...
    +  protected function getConnection(array $database) {
    +    // Set up the connection.
    +    Database::addConnectionInfo('upgrade', 'default', $database);
    ...
    +   * @param array|\Drupal\Core\Database\Database $database
    +   *   Database array representing the source Drupal database.
    ...
    +  protected function getMigrationTemplates(array $database, $source_base_path) {
    +    // Set up the connection.
    +    $connection = $this->getConnection($database);
    

    Hmm the data type stuff here is strange / confusing. How does this even work? Can it actually ever be a connection object? I think it is just only an array, looking at https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%.... If it were the typed object, there would be a fatal based on the typehint, no?

  2. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -0,0 +1,222 @@
    +    catch (\Exception $e) {
    +      // The table might not exist for example in tests.
    +    }
    

    I think @tim.plunkett mentioned this too, but this is way too broad a catch. We should have a followup issue to fix this because it is just swallowing any error. If there is a specific exception that is happening then we should handle it more directly or look at underlying causes.

  3. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -0,0 +1,222 @@
    +   * @param array $migration_templates
    +   *   Migration template.
    ...
    +    $initial_migrations = $builder->createMigrations($migration_templates);
    ...
    +   * @param array $migration_templates
    +   *   Migration template.
    

    The description for this parameter also sounds off. It's an array of migration templates; what data type are the templates? https://api.drupal.org/api/drupal/core%21modules%21migrate%21src%21Migra... makes it look like it is an associative array of parsed yaml for the templates, keyed by ID.

  4. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -0,0 +1,222 @@
    +        if ($source_plugin instanceof RequirementsInterface) {
    ...
    +        if ($destination_plugin instanceof RequirementsInterface) {
    

    What if it's not a RequirementsInterface? What does that mean?

  5. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -0,0 +1,222 @@
    +      // Migrations which are not applicable given the source and destination
    +      // site configurations (e.g., what modules are enabled) will be silently
    +      // ignored.
    +      catch (RequirementsException $e) {
    +      }
    +      catch (PluginNotFoundException $e) {
    +      }
    

    This seems a fragile way to handle this; is this related to the existing followup about detecting applicable migrations?

  6. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -0,0 +1,222 @@
    +      catch (\PDOException $e) {
    +        $version_string = FALSE;
    +      }
    

    We are doing this catch if the system table exists, but not if the key_value table exists.

  7. +++ b/core/modules/migrate_drupal_ui/migrate_drupal_ui.info.yml
    @@ -0,0 +1,11 @@
    +name: Drupal Upgrade
    

    Let's also change the name to "Drupal Upgrade UI".

  8. +++ b/core/modules/migrate_drupal_ui/migrate_drupal_ui.info.yml
    @@ -0,0 +1,11 @@
    +version: VERSION
    

    In #2656994: Experimental modules should have their own version numbers, we will change this to be an alpha version, but that is not in yet.

  9. +++ b/core/modules/migrate_drupal_ui/migrate_drupal_ui.install
    @@ -0,0 +1,16 @@
    + * Install, update and uninstall functions for the migrate_drupal_ui module.
    

    Nit: missing serial comma.

  10. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1227 @@
    +        '#prefix' => $this->t('<p>An upgrade has already been performed on this site.</p>'),
    +        '#description' => $this->t('<p>Last upgrade: @date</p>', ['@date' => $this->dateFormatter->format($date_performed)]),
    ...
    +    return $this->t('<p><strong>Upgrade analysis report</strong></p>');
    

    I think usually we don't include <p> tags inside translatable strings.

  11. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1227 @@
    +      $info[] = $this->t('In general, enable all modules on this site that are enabled on the previous site. For example, if you have used the book module on the previous site then you must enable the book module on this site for that data to be available on this site.');
    

    Followup: link a d.o handbook reference for the modules added to core/replacements/etc.

  12. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1227 @@
    +    // Retrieve the database driver from the form, use reflection to get the
    +    // namespace and then construct a valid database array the same as in
    

    nit: Missing serial comma.

  13. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1227 @@
    +      // Find a migration which has database credentials and use those.
    

    That sounds... magical. How does this work? What if there is more than one?

  14. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1227 @@
    +    catch (\Exception $e) {
    +      $error_message = [
    +        '#type' => 'inline_template',
    +        '#template' => '{% trans %}Resolve the issue below to continue the upgrade.{% endtrans%}{{ errors }}',
    +        '#context' => [
    +          'errors' => [
    +            '#theme' => 'item_list',
    +            '#items' => [$e->getMessage()],
    +          ],
    +        ],
    +      ];
    

    Using an item list and an inline template for a single exception message seems like overkill. I guess this came from the install form?

    Also (again), \Exception is way generic, and using a try/catch like this to handle expected validation code flow is problematic.

  15. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1227 @@
    +        '#markup' => $this->t('All previously-imported content, as well as configuration such as field definitions, will be removed.'),
    

    Nit: "previously" is an adverb, so there is no need for a hyphen.

  16. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1227 @@
    +      ksort($table_data);
    ...
    +        ksort($table_data[$source_module]);
    

    It would be good to document with an inline comment what this sort is and why we are doing it.

  17. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1227 @@
    +        '#description' => $this->t('The following items will not be upgraded. For more information see <a href="https://www.drupal.org/upgrade/migrate"> Upgrading from Drupal 6 or 7 to Drupal 8</a>.'),
    

    Nit: there is a space between the <a> tag and the URL name.

    Also, I thought I'd changed this to use a URL placeholder (because translators should not change or have to deal with URLs generally) but apparently I missed it.

  18. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1227 @@
    +        '#title' => '<ul><li>' . $this->t('@count available upgrade paths', ['@count' => $available_count]) . '</li><li>' . $this->t('@count missing upgrade paths', ['@count' => $missing_count]) . '</li></ul>',
    

    This seems like a misues of #title and of the HTML <ul> tag, and the concatenation does not seem best practice based on https://www.drupal.org/node/2296163. However, I can't actually remember how #title specifically behaves. Still, item lists should be item lists. I guess some depends on how exactly this is used in the UI.

  19. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1227 @@
    +      // Assume we want all those tagged 'Drupal %'.
    ...
    +          if (strpos($tag, 'Drupal ') === 0) {
    

    Is this use of the migration tags documented somewhere?

  20. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1227 @@
    +      batch_set($batch);
    +      $form_state->setRedirect('migrate_drupal_ui.upgrade');
    +      $this->state->delete('migrate_drupal_ui.performed');
    

    What happens if the rollback croaks in the middle of the batch?

  21. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1227 @@
    +  protected function getDatabaseTypes() {
    +    // Make sure the install API is available.
    +    include_once DRUPAL_ROOT . '/core/includes/install.inc';
    +    return drupal_get_database_types();
    +  }
    

    Having this abstracted into a method is a good change.

  22. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1227 @@
    +    return $this->t('Are you sure?');
    

    So, that's really not the most helpful question. :)

  23. +++ b/core/modules/migrate_drupal_ui/src/MigrateMessageCapture.php
    @@ -0,0 +1,48 @@
    +/**
    + * Allows capturing messages rather than displaying them directly.
    + */
    +class MigrateMessageCapture implements MigrateMessageInterface {
    

    This actually seems like something that it could be cool for core to provide. Some day, in the bright future where we improve the messaging system.

  24. +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php
    @@ -0,0 +1,369 @@
    +  /**
    +   * The maximum length in seconds to allow processing in a request.
    +   *
    +   * @var int
    +   */
    +  protected static $maxExecTime;
    

    It could be useful to reference how this gets set (in run() looks like).

  25. +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php
    @@ -0,0 +1,369 @@
    +        $context['message'] = $context['sandbox']['messages'][$index] . "<br />\n" . $context['message'];
    

    Concatenating the messages like this is a little weird and makes me wonder about the markup escaping strategy.

  26. +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php
    @@ -0,0 +1,369 @@
    +          $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'];
    

    Same "hmm" about markup escaping due to the concatenation.

  27. +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php
    @@ -0,0 +1,369 @@
    +    return \Drupal::logger('migrate_drupal_ui');
    

    Could be injected?

  28. +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php
    @@ -0,0 +1,369 @@
    +  /**
    +   * Wraps the translation manager.
    +   *
    +   * @return \Drupal\Core\StringTranslation\TranslationManager
    +   *   The string translation manager.
    +   */
    +  protected static function getTranslation() {
    +    return \Drupal::translation();
    +  }
    

    Why isn't this just injected instead?

  29. +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php
    @@ -0,0 +1,369 @@
    +    // If we had any successes lot that for the user.
    

    Should this be "log that for the user"?

  30. +++ b/core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php
    @@ -0,0 +1,164 @@
    +    // Create and log in as user 1. Migrations in the UI can only be performed
    +    // as user 1.
    

    Technically this isn't true yet, right? Should we link the followup issue?

  31. +++ b/core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php
    @@ -0,0 +1,164 @@
    +    $this->assertText(t('Congratulations, you upgraded Drupal!'));
    

    Minor: this assertText() does not need the t(). But I think we haven't made that consistent in core yet, so out of scope.

xjm’s picture

Posted #2679835: Exception handling in Migrate UI. This covers points 10 and 23 of @tim.plunkett's review in #116, and points 2, 5, part of 6, and part of 14 for my review in #133.

xjm’s picture

#2679845: Inject dependencies in MigrateUpgradeRunBatch addresses points 27 and 28 of my review.

Gábor Hojtsy’s picture

1. Fixed.
2. Already has a followup.
3. Fixed.
4. Don't know :/
5. Don't know :/
6. Already has a followup.
7. Done. It also should have quotes (and also the package name).
8. Not to be done here as you wrote.
9. Done.
10. Done.
11. @todo Do we have such a list somewhere?
12. Done.
13. Done.
14. Message: Don't know :/ Exception: Already has followup.
15. Done.
16. Done.
17. Done.
18. Don't know :/
19. This pattern is used at various places, eg. in the trait. Not sure where could it even be documented.
20. That sounds like a usual batch problem. Dont know :/
21. Good.
22. This is how it looks like: https://www.drupal.org/files/issues/Are%20you%20sure%3F%20%7C%20Hojtsy.h.... Sounds like one for https://www.drupal.org/node/2678638?
23. Yeah.
24. Done.
25. Don't know. :/
26. Don't know. :/
27. Already has a followup.
28. Already has a followup.
29. Done.
30. Done.
31. We also use t() when pressing buttons in the tests. If those use t() this should too.

Gábor Hojtsy’s picture

@xjm points out 7 was not in the patch in fact. I have too many checkouts of this module locally. I double-checked and that was the only change I made there instead of in the patched core.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests

Because this is experimental and alpha, and has dedicated follow-ups for all of the things that weren't addressed, I believe this is ready for inclusion.
If it were not marked experimental or alpha, it would not be ready IMO.

But the functionality this adds, and the flexibility to improve the code while in core, outweighs the technical debt we are adding.

xjm’s picture

Issue tags:+Needs followup

Well, has or will have in a couple cases. :) Thanks @tim.plunkett and @Gábor Hojtsy!

mikeryan’s picture

Trying to fill in some gaps

4. What if it's not a RequirementsInterface? What does that mean?

Not all source plugins necessarily implement RequirementsInterface - if not, they have no requirements which must be met.

5. This seems a fragile way to handle this; is this related to the existing followup about detecting applicable migrations?

Second reference I've seen to this followup but I can't seem to find it - could be be added as a child/relation to this issue? In the meantime, from the exception followup:

I.e., catching PluginNotFoundException and RequirementsException - yes, this is for skipping inapplicable migrations. There are two scenarios - where a migration references a source or destination plugin which is defined by a module which is not enabled (thus, we do not want to perform migration for that module), we get PluginNotFoundException. And if the plugin's module is enabled but there's a missing requirement (e.g., the corresponding module is not enabled in the source database), checkRequirements() throws RequirementsException.

20. What happens if the rollback croaks in the middle of the batch?

Hrrmm, yes, this should be done at the completion of rollback.

25, 26. Concatenating the messages like this is a little weird and makes me wonder about the markup escaping strategy.

What's best practice for accumulating messages so they remain visible during the batch processing?

xjm’s picture

I posted #2679913: Fix use of markup in strings in Migrate UI and removed those changes, because I'm not 100% sure they won't introduce regressions in the string rendering and/or escaping bugs.

alexpott’s picture

Batch messages are treated as markup... see _batch_progress_page()...

    '#message' => array('#markup' => $message),

So this will be xss filtered so all of this concatenation is fine - batches are special. This is documented somewhere...
xjm’s picture

Thanks @mikeryan!

Not all source plugins necessarily implement RequirementsInterface - if not, they have no requirements which must be met.

That makes sense, thanks. I guess we could add an inline comment.

5. This seems a fragile way to handle this; is this related to the existing followup about detecting applicable migrations?
Second reference I've seen to this followup but I can't seem to find it - could be be added as a child/relation to this issue

I was referring to #2569805: For Drupal migration, identify the source module -- it could be it is not related. I added this as a related issue for that.

Hrrmm, yes, this should be done at the completion of rollback.

Let's make that a followup issue.

What's best practice for accumulating messages so they remain visible during the batch processing?

No idea, but we can sort that in #2679913: Fix use of markup in strings in Migrate UI. :)

xjm’s picture

@alexpott, I'll note that on the followup.

The last submitted patch, 137: 2281691-137.patch, failed testing.

mikeryan’s picture

@xjm: #2569805: For Drupal migration, identify the source module is purely about presenting to the user the relationships between source and destination modules, it does not relate to the process of determining what migrations are applicable.

xjm’s picture

Fail is our hook_help() friend again:

fail: [Other] Line 146 of core/modules/system/src/Tests/Module/InstallUninstallTest.php:
'Drupal Upgrade UI module' is on the help page for migrate_drupal_ui

fail: [Other] Line 146 of core/modules/system/src/Tests/Module/InstallUninstallTest.php:
Correct online documentation link is in the help page for migrate_drupal_ui

lol. Fixing that now.

alexpott’s picture

+++ b/core/modules/migrate_drupal_ui/migrate_drupal_ui.install
@@ -0,0 +1,16 @@
+function migrate_drupal_ui_install() {
+  $url = Url::fromUri('base:upgrade')->toString();
+  drupal_set_message(t('The Drupal Upgrade module has been enabled. Proceed to the <a href=":url">upgrade form</a>.', [':url' => $url]));
+}

So this is the only place a user is given the link to go to migrate their site. There is nowhere in the admin ui apart from the configure link on the modules page - and configure is a pretty weird label for this. I guess we should have a followup to have a think about where to put a link in the UI to the page.

The last submitted patch, 141: migrate_ui_2281691-141.patch, failed testing.

xjm’s picture

alexpott’s picture

Whilst working on #2679931: Migrate form should be linked from the UI somewhere I realised the message set in the hook_install implementation will be inconsistent because of the module name... uploading an interdiff with #147 to, potentially, fix on commit.

  • xjm committed c136a85 on 8.1.x
    Issue #2281691 by xjm, mikeryan, Gábor Hojtsy, alexpott, tim.plunkett,...
xjm’s picture

Status:Reviewed & tested by the community» Fixed

Alrighty! I am comfortable committing this as an alpha-stability experimental module, for the reasons @tim.plunkett describes in his RTBC comment in #138. There are a few remaining followups I'd like to file and I'll take care of that in a bit.

A huge thank-you to everyone who collaborated to build this module and make it core-ready. Thanks especially to @Gábor Hojtsy, @abhishek-anand, @mikeryan, @webchick, and @alexpott who teamed up with me to get this to a point where it was committable for 8.1.0-beta1, and also to @tim.plunkett for 11th-hour help with code review and cleanups.

Committed c136a85 and pushed to 8.1.x!

mikeryan’s picture

Just pulled 8.1.x and manually ran a D6 migration, everything went smoothly - thanks everybody!

heddn’s picture

Great job here everyone!

yoroy’s picture

Nice!

#2678638: Refinements for Migrate UI might need more recent screenshots than the ones I made based on Gabors screencast in #69. Could somebody verify? Thank you.

xjm’s picture

Issue tags:+8.1.0 release notes

Status:Fixed» Closed (fixed)

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