Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
- Overview page (at http://example.com/upgrade), describing what is to happen and how to prepare for it.
- Credentials page - get the DB info for the source site, as well as the address from which to fetch the files.
- 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.
- Batch - the migrations are run through the normal Batch API.
- 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:
- 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).
- 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.
- Otherwise, we skip the credentials page (we already have that information) and go directly to the report/confirmation page, proceeding normally from there.
Comment | File | Size | Author |
---|---|---|---|
#151 | 2281691.quick-fix-on-commit.txt | 684 bytes | alexpott |
#147 | interdiff-hook-help.txt | 1.13 KB | xjm |
#147 | migrate-2281691-147.patch | 78.26 KB | xjm |
#141 | interdiff_strings.txt | 1.36 KB | xjm |
#141 | migrate_ui_2281691-141.patch | 78.25 KB | xjm |
Comments
Comment #1
mikeryanComment #2
mikeryanComment #3
mikeryanComment #4
mikeryanComment #5
mikeryanComment #6
mikeryanComment #7
mikeryanComment #8
mikeryanComment #9
mikeryanComment #10
mikeryanFor 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.
Comment #11
mikeryanComment #12
mikeryanThat's it, all the known child issues at this time have been added.
Comment #13
mikeryanSo, 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.
Comment #14
benjy CreditAttribution: benjy commentedWould be nice to have an update on this issue of the current state of the UI, big outstanding issues etc.
Comment #15
mikeryanAdded context issue.
Comment #16
mikeryanSorry 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?
Comment #17
benjy CreditAttribution: benjy commentedAlso, 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.
Comment #18
mikeryanI would like to see it broken up, I see a few different things going on there it'd be easier to review separately:
On a quick once-over I have some comments/questions, but it'll be easier to discuss in more specific issues.
Thanks!
Comment #19
benjy CreditAttribution: benjy commentedSorry, I should have mentioned there is an overview on the sandbox overview page.
Comment #20
iantresman CreditAttribution: iantresman commentedJust 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.
Comment #21
webchickEscalating 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.
Comment #22
mikeryanAn 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.
Comment #23
webchickSo 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.
Comment #24
webchickTesting instructions + screenshot at https://www.drupal.org/node/2257723
Comment #25
mikeryanQuick 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?
Comment #26
andypostis this UI depends on
migrate_drupal
or migrate module itself?Comment #27
mikeryanYes - 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...
Comment #28
mikeryanI should point out we need some discussion around #2548977: How best to persist source database credentials?.
Comment #29
mikeryanOh, 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).
Comment #30
webchickI would just stick it in the Migrate Drupal module directly, personally. That might not be popular. :)
Comment #31
benjy CreditAttribution: benjy at PreviousNext commentedI 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.
Comment #32
mikeryanHere'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.
Comment #33
andypostso it depends on migrate_drupal and dblog
useless
module do different
tbd
Comment #34
mikeryanJust 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.
Comment #35
phenaproximaI 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.
Comment #36
mikeryanWell, 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...
Comment #37
webchickPart #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:
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 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:
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.
Comment #38
yoroy CreditAttribution: yoroy as a volunteer commentedInitial 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.
Comment #39
hass CreditAttribution: hass commentedWe may better connect to the source database and detect the source version automatically. May add one extra step or use ajax.
Comment #40
webchickHere'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:
Incidentally, if you fill in total garbage here, the error messages are quite obtuse:
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:
So far, so good.
Then, you hit a problem, such as:
Example output:
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:
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):
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:
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.
Comment #41
webchickTwo 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.)
Comment #42
webchickJust a quick edit to remove display of those screenshots so you can still see the patch.
Comment #43
mikeryanAnything marked "done" here is in the code for the next patch, more complex things will come.
Good point - for the moment, I'm adding the explicit dependency to the .info.yml file. But, is requiring dblog a good idea?
Done.
hook_help() was there, but not for the help.page.migrate_drupal_ui route. Added some initial help text there.
Hmm, it is there and working for me (at least with the changes I've made so far, none of which seemed specifically applicable)?
Done.
Will do. Note that actually handling private files for D7 depends on #2547125: D7 file migration should allow selecting public/private/all files.
Done for the moment, D7 variant to be added as part of the above.
Done.
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?
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'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...
Comment #44
webchickIncidentally, 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
Comment #45
mikeryanWill look into how the installer nicifies the messages.
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://...
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).
Will look at that, MigrateMessageCapture will need to get the message levels.
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.
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.
"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....
Comment #46
Bojhan CreditAttribution: Bojhan commentedMike 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?
Comment #47
mikeryanSome of the kinds of messages seen in Angie's examples are:
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.
Do you have a specific example? They look like I would expect.
Going back to bits I missed from Angie:
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.
Done.
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
Backup all your shit! Now!
(dang, style="color: red" doesn't work)
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...
Comment #48
mikeryanSo, 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:
Thoughts?
Comment #49
webchickWell, 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!]
Comment #50
mikeryanI 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.
Comment #51
webchickYep, agreed.
Comment #52
mikeryanOK, I've backported all the work I've done here (plus a little more message tweaking) to migrate_upgrade). Next steps:
Comment #53
mikeryanAttached 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:
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)
Comment #54
mikeryanComment #55
webchickFirst, 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:
1) It first warns you about what the process you're about to engage in, and how to take proper precautions.
2) Next, it shows you what it's about to do and gives you a chance to review it before proceeding:
3) It performs operations in a batch...
4) If there were problems, it tells you what happened, with something you can go and Google more to figure out...
5) You're able to review detailed logs to determine even more about what happened in order to help with said Googling...
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:
7) Repeat until you get a green board (see no more error messages) and then you know you are done:
Comment #56
webchickNext, 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:
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:
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:
At the bottom of the form you'll see a variety of rows like this:
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:
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:
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:
No comment, that all looks fine.
If you attempt to re-run the migration, this is what you see:
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.
Comment #57
webchickFinally, 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)
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:
That last point is particularly bad, because the only way to figure out if it's "NBD" or "OMG data loss" is to:
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. :(
Comment #58
yoroy CreditAttribution: yoroy as a volunteer commentedThis is my understanding of the upgrade process:
Webchick and I discussed mostly what we should be doing at step 2, see #57 for the different scenarios.
Comment #59
webchickOk, 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.
Comment #60
yoroy CreditAttribution: yoroy as a volunteer commentedUpdated 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?
Comment #61
mikeryanAll 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.
Comment #62
mikeryan*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:
Comment #63
Bojhan CreditAttribution: Bojhan commentedComment #64
Bojhan CreditAttribution: Bojhan commentedI'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
Comment #65
mikeryanAt 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.
Comment #66
mikeryanUpdated issue summary.
Comment #67
mikeryanHaven'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?
Comment #68
catchUpdating to show there's a patch.
Comment #69
Gábor HojtsyHere 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:
Comment #70
Gábor HojtsyRolled a new version of the patch with the script in #67, thanks. Moving to 8.1 for testing.
Comment #72
xjmThe
hook_help()
failure is just about having a standard format of help text on the help page; I'll submit a fix for that.Comment #73
xjmPosted #2666418: hook_help() does not match the prescribed standard and so fails core tests.
Comment #74
xjmIt 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.Comment #75
xjmFail. Actual combined patch.
Comment #78
Gábor HojtsyLooks like the only other fail now is the missing composer.json entry.
Comment #79
xjmHere's a fix for that too. I guess we could either add this to the script or apply manually.
Comment #80
xjmStarted 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.So I'd expect to see some dependency for this injected here. Assuming this code only runs if Drush is available?
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.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.I think using
formatPlural()
withint()
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 justformatPlural()
.Comment #81
xjm@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:
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.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 oft()
. 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.update.php
, and Migrate is actually much safer because unlikeupdate.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.Comment #82
mikeryanDrushLogMigrateMessage.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.
(referencing the $moduleUpgradePaths array) - I'll fix this up in the contrib module.
For l.d.o, we need to have the literal strings directly in the t() and formatPlural() calls, correct?
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.
This all looks good to me.
One other thing, just want to get the list of committers to migrate_upgrade in here:
Comment #83
mikeryanSpeaking of formatPlural(), I see inconsistent results with single counts like...
Any ideas why that might be happening?
Comment #84
mikeryanUpdated corify (extra credit: have corify do the edit to composer.json) and the patch itself with the above changes plus the menu_links fix.
Comment #85
mikeryanSpeaking of the menu_link fix, please add Jo Fitzgerald to the credits list.
Comment #86
mikeryanHere's the new corify (didn't notice that d.o had silently ignored my upload without the .txt extension).
Comment #87
xjm@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
Comment #88
xjmComment #89
andypostsuppose summary should be updated with current UI
Comment #90
quietone CreditAttribution: quietone as a volunteer commentedScreenshots 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.
Comment #91
Bojhan CreditAttribution: Bojhan commented@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" ?
Comment #92
xjmThanks @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.)
Comment #93
xjmSorry, I should also have linked the background issues:
#2649768: [meta] No definition of "Experimental" & not nearly enough warning
#2657062: [policy, no patch] Experimental module policy and handbook page
#2656994: Experimental modules should have their own version numbers
Comment #94
xjmThis 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
Comment #95
yoroy CreditAttribution: yoroy as a volunteer commentedFor 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.
Comment #96
yoroy CreditAttribution: yoroy as a volunteer commentedComment #97
xjmThanks @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.
Comment #98
cilefen CreditAttribution: cilefen commentedRandom comment... ;-)
Comment #99
Luukyb CreditAttribution: Luukyb commentedJust random comment following the message from xjm, this is getting closer to core!!
Comment #100
yoroy CreditAttribution: yoroy as a volunteer commentedAh sorry, that was what I meant to say: the string changes can be followups :)
Comment #101
xjmUpdating credit; thanks @quietone, @Luukyb, and @cilefen for working on the contrib module!
Comment #102
er.pushpinderrana CreditAttribution: er.pushpinderrana as a volunteer and at Publicis Sapient for Publicis Sapient commentedThanks 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 :).
Comment #103
hussainwebThanks @xjm for pointing me here.
Comment #104
xjmSo here is our current status to the best of my knowledge:
Updating credit, also. Thanks @hussainweb and @er.pushpinderrana!
Comment #105
aleksipRandom comment, thanks @xjm and everyone involved!
Comment #106
anavarreThanks for the ping, @xjm
Comment #107
xjmUpdating credit, thanks!
Comment #108
alexpottI'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
Comment #109
alexpottAfter 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.
Comment #110
yoroy CreditAttribution: yoroy as a volunteer commentedComment #111
Anonymous (not verified) CreditAttribution: Anonymous commentedMaking a comment... thanks for the ping @xjm. Cool to see this is getting merged in!
Comment #112
xjmUpdating 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?
Comment #113
mikeryanYes, 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?
Comment #114
xjmHmm, 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.
Comment #115
tim.plunkettJust a note, if you place the module in
/modules
and not in/modules/contrib
as I do, then the corify script will fail on thecd
, and then happilyrm -Rf .git
in your core checkout. Ouch.Here is a newly generated patch from the 8.x-1.x branch for review purposes.
Comment #116
tim.plunkett(minor) Missing blank line
Missing leading slash
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.
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.
No need to wrap this, please but it on one line.
These should be injected. Also, entityManager is deprecated.
This should use the protected method.
This code is incorrectly formatted, and we usually don't commit uncommented code.
===
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.
One line, please
This is a very large else block. Can this function be rewritten to avoid that, or broken into smaller methods?
This is an odd way to do this, what other #theme would be here?
This needs to be a FQCN
You can use
MigrateUpgradeRunBatch::class
here, and anywhere else this refers to class names as stringsThis is another very large method that does a lot. Any way to break it up?
Can you put the event dispatcher into a local variable?
Use
static::class
instead of get_class()These are oddly formatted. Also, doesn't appending the message to the end of a translated string mess this up?
One line each please
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.
Docs clash. Is it an array or an object?
Is there a classed exception to catch or throw instead? Using \Exception is not as ideal
Array or string?
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.
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.
Comment #117
tim.plunkettAddressed much of #116 in #2679016-2: Misc cleanups
Comment #118
tim.plunkettJust 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.
Comment #119
xjmAlright:
Therefore, the next steps are:
.git
directory if you do not put the module inmodules/contrib
or something like that. You might not want to use a core repo with your own feature branches etc.)If you'd like to see this UI in 8.1.0, please help with these steps within the next 24 hours.
Comment #121
alexpottI'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...I still think that this should be one module - I don't think
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'.Comment #123
alexpottFixing the test fail... and the interdiff is the patch that needs to be applied to core/composer.json
Comment #124
alexpottHere's a better corify script that doesn't hard code the location of
migrate_upgrade
and applies a patch to core/composer.json.Comment #126
Anonymous (not verified) CreditAttribution: Anonymous as a volunteer commented"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:
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!
Comment #127
mikeryanThere 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...
Comment #128
xjmSo 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.
Comment #129
alexpottre #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.
Comment #130
mikeryanAh, 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...
Comment #131
alexpottCreated #2679739: [Policy, no patch] Make the migrate_drupal_ui module part of the migrate_drupal module to discuss the migrate_drupal/_ui merge as a followup.
Comment #132
xjmAlright, 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.
Comment #133
xjmAlright, I've performed a code review of the full patch. Most or all of this can be followups if needed.
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?
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.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.
What if it's not a RequirementsInterface? What does that mean?
This seems a fragile way to handle this; is this related to the existing followup about detecting applicable migrations?
We are doing this
catch
if the system table exists, but not if the key_value table exists.Let's also change the name to "Drupal Upgrade UI".
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.
Nit: missing serial comma.
I think usually we don't include
<p>
tags inside translatable strings.Followup: link a d.o handbook reference for the modules added to core/replacements/etc.
nit: Missing serial comma.
That sounds... magical. How does this work? What if there is more than one?
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.Nit: "previously" is an adverb, so there is no need for a hyphen.
It would be good to document with an inline comment what this sort is and why we are doing it.
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.
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.Is this use of the migration tags documented somewhere?
What happens if the rollback croaks in the middle of the batch?
Having this abstracted into a method is a good change.
So, that's really not the most helpful question. :)
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.
It could be useful to reference how this gets set (in
run()
looks like).Concatenating the messages like this is a little weird and makes me wonder about the markup escaping strategy.
Same "hmm" about markup escaping due to the concatenation.
Could be injected?
Why isn't this just injected instead?
Should this be "log that for the user"?
Technically this isn't true yet, right? Should we link the followup issue?
Minor: this
assertText()
does not need thet()
. But I think we haven't made that consistent in core yet, so out of scope.Comment #134
xjmPosted #2679835: Improve 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.
Comment #135
xjm#2679845: Inject dependencies in MigrateUpgradeRunBatch addresses points 27 and 28 of my review.
Comment #136
Gábor Hojtsy1. 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.
Comment #137
Gábor Hojtsy@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.
Comment #138
tim.plunkettBecause 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.
Comment #139
xjmWell, has or will have in a couple cases. :) Thanks @tim.plunkett and @Gábor Hojtsy!
Comment #140
mikeryanTrying to fill in some gaps
Not all source plugins necessarily implement RequirementsInterface - if not, they have no requirements which must be met.
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.
Hrrmm, yes, this should be done at the completion of rollback.
What's best practice for accumulating messages so they remain visible during the batch processing?
Comment #141
xjmI 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.
Comment #142
alexpottBatch messages are treated as markup... see _batch_progress_page()...
So this will be xss filtered so all of this concatenation is fine - batches are special. This is documented somewhere...
Comment #143
xjmThanks @mikeryan!
That makes sense, thanks. I guess we could add an inline comment.
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.
Let's make that a followup issue.
No idea, but we can sort that in #2679913: Fix use of markup in strings in Migrate UI. :)
Comment #144
xjm@alexpott, I'll note that on the followup.
Comment #146
mikeryan@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.
Comment #147
xjmFail is our
hook_help()
friend again:lol. Fixing that now.
Comment #148
alexpottSo 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.
Comment #150
xjmFiled #2679929: Reduce method and control structure length/complexity in Migrate UI and #2679933: Confirm form theming in Migrate UI from @tim.plunkett's earlier review.
Comment #151
alexpottWhilst 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.
Comment #153
xjmAlrighty! 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!
Comment #154
mikeryanJust pulled 8.1.x and manually ran a D6 migration, everything went smoothly - thanks everybody!
Comment #155
heddnGreat job here everyone!
Comment #156
yoroy CreditAttribution: yoroy as a volunteer commentedNice!
#2678638: [META] Usability 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.
Comment #157
xjm