Problem/Motivation

Follow-up to #2905222: Migrate UI: Initial Screen UX improvements. @xjm made some review comments that will be addressed here:

  1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -142,16 +142,26 @@ public function buildOverviewForm(array $form, FormStateInterface $form_state) {
    -        '#markup' => '<p>' . $this->t('Upgrade a site by importing it into a clean and empty new install of Drupal 8. You will lose any existing configuration once you import your site into it. See the <a href=":url">online documentation for Drupal site upgrades</a> for more detailed information.', [
    +        '#markup' => '<p>' . $this->t('Upgrade a site by importing its database and files into a clean and empty new install of Drupal 8. See the <a href=":url">Drupal site upgrades handbook</a> for more information.', [

    We're not importing the site's database and files. We're importing the data from its database and files.

  2. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -142,16 +142,26 @@ public function buildOverviewForm(array $form, FormStateInterface $form_state) {
    +      $legend[] = $this->t('<em>Old site:</em> the site you want to upgrade.');
    +      $legend[] = $this->t('<em>New site:</em> this empty Drupal 8 installation you will import the old site to.');
    +
    +      $form['legend'] = [
    +        '#theme' => 'item_list',
    +        '#title' => $this->t('Definitions'),
    +        '#list_type' => 'ul',
    +        '#items' => $legend,
    +      ];

    If it's a list of definitions, should it use a definition list?

  3. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -142,16 +142,26 @@ public function buildOverviewForm(array $form, FormStateInterface $form_state) {
    +      $info[] = $this->t('You may need multiple tries for a successful upgrade so <strong>backup the database</strong> for this new site. The upgrade will change it and you may want to revert to its initial state.');

    "Back up" needs to be two words when it's a verb. (It's correct in HEAD.)

    Also "you may need multiple tries" is kind of confusing ("a try" as a noun might not be a familiar expression); "attempts" might be better. But reading this I might wonder why I might need multiple attempts; it's not like shooting a basketball.

    Finally, we aren't using the defined term "new site" here, and if I'm starting from a "clean and empty new install" then why would I want to back up the database of that?

  4. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -160,12 +170,13 @@ public function buildOverviewForm(array $form, FormStateInterface $form_state) {
    +        '#title' => $this->t('Steps to prepare for the upgrade'),

    How about "Preparation steps"?

Proposed resolution

Address comments.

Remaining tasks

Make changes.

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jo Fitzgerald created an issue. See original summary.

jofitz’s picture

Status: Fixed » Active
jofitz’s picture

Status: Active » Needs work
FileSize
2.44 KB

I have made a start on the changes, but need some help/clarification about a couple of things. There's no need to set the status to "Needs review" and trouble the testbot because this is still a work in progress.

  1. Corrected the sentence.
  2. How can this be changed to a definition list?
  3. Corrected "backup" and "tries", but I'm not sure how to resolve the apparent contradiction in the sentence about backing up the new site.
  4. Edited the title.

Todo:
2. Make $form['legend'] into a definition list.
3. Re-write the sentence about backing up the new site.

neerajsingh’s picture

On item number 3, Yes I agree to @xjm.
We need not backup database as this would be a new installation of Drupal8.

In-fact if we restore the database and try again to perform the migrate, it migrates only the new changes(between the previous migration and the next attempt) in the legacy(Drupal6/7) system and not the complete application.

Step to re-generate the scenario in the above sentence:

  1. Install a Drupal-6 application.
  2. Add an article at Drupal-6 site, say article-1.
  3. Install a new Drupal-8 application.
  4. Enable migrate UI(and dependents) module on Drupal-8 application.
  5. Take a database backup of Drupal-8 application.
  6. Perform migrate from Drupal-6 application to drupal-8.
  7. Restore database of Drupal-8, that was taken at Step-5.
  8. Create another article at Drupal-6 application, say article-2.
  9. Perform the migration again from Drupal-6 application to drupal-8.
  10. Now on Drupal-8 application we will have only article-2. Ideally we should have both the articles.

Hence, I would suggest to re-install Drupal8 setup in this case, instead of re-storing the backup database.

heddn’s picture

You may want to run multiple tries for a successful upgrade so <strong>backup the database</strong> for this new site. The upgrade will change it and you may want to revert to its initial state.

I think this was more important when incremental migrations were a thing via the UI. Since it isn't a thing right now, adding special instructions to back up is pointless and something I personally have never done.

So, if we can agree on removing the line, all that is left is a definition list.

heddn’s picture

Issue tags: +Novice
Related issues: +#2905491: Mark Migrate Drupal UI as stable
heddn’s picture

Should this get postponed on #54898: Add a description-list.html.twig template (ex. definition list)? Or just hack up the markup without a theme layer? Or just say good enough is good enough?

xjm’s picture

I don't think we need to postpone this. We could either hardcode the markup in the render array, or just to create a specific template. There are a couple examples in core. If you grep for <dl> most are in hook_help() which is not currently themeable and just concatenated and then converted to markup (so not really applicable yet as this isn't a hook_help()), but there are also examples like these:
core/themes/stable/templates/admin/block-content-add-list.html.twig

jofitz’s picture

Status: Needs work » Needs review
FileSize
2.15 KB
3.05 KB
  • Removed the line about "...run multiple tries...".
  • Hard-coded the definition list.

Status: Needs review » Needs work

The last submitted patch, 9: 2908699-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jofitz’s picture

Status: Needs work » Needs review
FileSize
3.94 KB
6.16 KB
  • Updated the tests to match the text changes.
  • Corrected a coding standards error.
quietone’s picture

Status: Needs review » Needs work

This looks good, thanks everyone. Just one small nit.

importing the data from its database and files into
This could be read as importing the data from the files and not importing the files. I know it is just a quirk but perhaps it should be explicit that the fiies are also imported?

xjm’s picture

Issue tags: -Migrate critical

This is just some improvement for the UI text, but it doesn't need to block the stable release.

jofitz’s picture

Status: Needs work » Needs review
FileSize
1023 bytes
6.17 KB

Adjusted the sentence structure to avoid confusion.

Status: Needs review » Needs work

The last submitted patch, 14: 2908699-14.patch, failed testing. View results

jofitz’s picture

Status: Needs work » Needs review
FileSize
3.18 KB
6.18 KB

Forgot to update the tests too!

heddn’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Vienna2017

This is now good to go. The feedback from the IS is all addressed and all the nits responded to.

The last submitted patch, 3: 2908699-3.patch, failed testing. View results

Gábor Hojtsy’s picture

Title: Migrate UI: Further Screen UX improvements » Several textual and markup improvements to the Migrate Upgrade UI
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Superb, thanks all!

  • Gábor Hojtsy committed 17bf468 on 8.5.x
    Issue #2908699 by Jo Fitzgerald, heddn, xjm: Several textual and markup...

Status: Fixed » Closed (fixed)

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