At the top of the upgrade form, we need to tell the administrator a bunch of important stuff, including:

  • What is this form, and why do you want to use it (importing configuration/content from a Drupal 6/7 site).
  • What do you have to do before trying to fill in and submit this form? Access to source database and files...
  • Backup the D8 site!
  • Link to more details at https://drupal.org/upgrade
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

er.pushpinderrana’s picture

Version: » 8.x-1.x-dev
Status: Active » Needs review
FileSize
832 bytes

Please review.

hussainweb’s picture

Status: Needs review » Needs work
  1. +++ b/migrate_upgrade.module
    @@ -1 +1,16 @@
    +/**
    + * Implement hook_help().
    + */
    +function migrate_upgrade_help($route_name, Request $request) {
    

    I think the standard format for this is "Implements hook_help()." There is a 's' suffix. Just nitpicking.

  2. +++ b/migrate_upgrade.module
    @@ -1 +1,16 @@
    +      $output .= '<p>' . t('This form is used for importing configuration/content from a Drupal 6/7 site. Before submit this form, give access to source database and files. Backup the D8 site. ');
    +      $output .= t('For more details, see <a href="@upgrade">Upgrading from previous versions</a>.', array('@migrate'  => 'https://www.drupal.org/upgrade')) . '</p>';
    +      return $output;
    

    I am not too sure about two translated strings for the same context. I think it should be a single string passed to t() but I would leave it to other's judgement.

  3. +++ b/migrate_upgrade.module
    @@ -1 +1,16 @@
    +      $output .= t('For more details, see <a href="@upgrade">Upgrading from previous versions</a>.', array('@migrate'  => 'https://www.drupal.org/upgrade')) . '</p>';
    +      return $output;
    +  }
    

    I am not sure if it is a good idea to return from a switch-case block. Can you verify if this is a practice in other core modules?

  4. +++ b/migrate_upgrade.module
    @@ -1 +1,16 @@
    +  }
    +}
    \ No newline at end of file
     
    

    Nitpick: There should be a newline at the end of the file.

er.pushpinderrana’s picture

Status: Needs work » Needs review
FileSize
846 bytes

@hussainweb, thanks!

I am agree with you on point 1,2 & 4 but point 3 is fine as this std is followed in core modules as well. You can see the system.module file.

hussainweb’s picture

Status: Needs review » Needs work

I think you forgot the . "</p>" at the end of the line.

If returning from a switch-case is practice in other modules, then I am fine with it.

hussainweb’s picture

I read the help message now and I see a few improvements:

  1. It might be better to write Drupal 6 or 7 site.
  2. "Before submit this form, give access to source database and files." - This sentence could be improved. For one thing, it should be "Before submitting" in the beginning and "give access to" could be "give details for the".
  3. I don't think we should use "D8" here. You should use "Drupal 8" but in this case, I think you should say "current site".
er.pushpinderrana’s picture

Status: Needs work » Needs review
FileSize
4.02 KB

ohh, I forget to close </p> tag. Thanks!

"Before submit this form, give access to source database and files." - This sentence could be improved. For one thing, it should be "Before submitting" in the beginning and "give access to" could be "give details for the".

This is for source site file access (default/files) and db access. Now let's hold this for final review from mikeryan.

Thanks.

hussainweb’s picture

Status: Needs review » Needs work

It doesn't look like the correct patch.

er.pushpinderrana’s picture

Status: Needs work » Needs review
FileSize
859 bytes

I think I need to take some rest now. Thanks @hussainweb :)

hussainweb’s picture

FileSize
1.06 KB
916 bytes

I know the feeling. :)

Anyway, I just realized what you meant by giving access to source database and files. I have modified it and fixed the grammar as I suggested earlier. Patch attached.

amitgoyal’s picture

FileSize
916 bytes
1.18 KB

I believe we are using "!" for links in t() rather "@" in core. For example, see hook_help in quickedit.module.

mikeryan’s picture

Status: Needs review » Needs work

I think we need a bit more here.

This form is used for importing configuration and content from a Drupal 6 or 7 site

I'd like to see this not be specific about the versions - we will have contrib extending migrate_upgrade to support at least D5 and D8, and it'd be nice if the extensions didn't have to rewrite the help text.

Make sure you have backups of your current site before submitting this form.

I'm afraid that "current site" might be interpreted as the source site - it's the destination (D8) site that should be backed up, since it's the one that's being modified.

Also, ensure that the source database and files are accessible.

It needs to be clear that they need to be accessible to the D8 site - that, for example, any firewalls allow the server D8 is running on to access the database and files.

tadityar’s picture

Status: Needs work » Needs review
FileSize
958 bytes
1.07 KB

I've changed the help text. Please review :)

mikeryan’s picture

Status: Needs review » Needs work

This patch totally breaks the site with "Recoverable fatal error: Argument 2 passed to migrate_upgrade_help() must be an instance of Symfony\Component\HttpFoundation\Request, instance of Drupal\Core\Routing\CurrentRouteMatch given in migrate_upgrade_help()". For some reason api.drupal.org doesn't have hook_help(), but https://api.drupal.org/api/drupal/core!modules!node!node.module/function... is consistent with the error - the 2nd arg should be RouteMatchInterface rather than Request.

On the text itself, "previous versions of Drupal site" needs to be something like "previous version of your Drupal site".

hussainweb’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
999 bytes

Changing the text in several places and fixing the problem with the type of the second argument. That argument was not used and hence there wasn't much to do.

mikeryan’s picture

Status: Needs review » Fixed

Committed (removing the reference to backing up the source site - the source database is not altered by the migration process), thanks!

Status: Fixed » Closed (fixed)

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