Improve upgrade procedure followed by site-upgrade to better conform to UPGRADE.txt

Note that the alias specified to the site-upgrade command should point to the destination site. The source of the site upgrade is always @self. It is not valid to say drush site-upgrade @self, because the destination of the site upgrade must be a different site than the source.

I was looking at this a bit last night, and it seems that site upgrade still is not working. I got to the point where settings.php is being created. There is some code in the function drush_upgrade_fix_db_url that reads as follows:

  if (drush_drupal_major_version() <= 6) {
    $append .= "\n" . '$db_url = \'' . $new_url . '\';';
  }
  else {
    $databases = $GLOBALS['databases'];
    $databases['default']['default']['database'] = $target_alias_databases['default']['default']['database'];
    $append .= "\n" . '$databases = ' . var_export($databases, TRUE) . ';';
  }

The thing is, though, that drush_drupal_major_version() operates on the bootstrapped site, which is the source site, which in this case is Drupal 6, so it makes a $db_url. It seems that this test should be on the Drupal version of the destination site -- in this case, Drupal 7, which is to say that it should make the $databases record instead.

I didn't fully track this down though; I will return to it again later.

IIRC, drupal7 will upgrade just fine using a DB6 style settings.php. In that case, Drupal reads out the info it needs and then appends a long form $databases array to settings.php.

Okay, cool, thank you. I guess then my problem is that my db_url was not being composed correctly. I'll keep investigating; this may be a different issue than the OP's, who, it seems, has mysql permission issues.

site-upgrade worked for me in the past; still might be a user error.

Here are some suggested improvements for site-upgrade

1. If the root of the target == the root of @self, then print an error and exit
2. Currently, if the target root folder exists, then the Drupal-7 download will be skipped, causing failures later on. The download of Drupal-7 should be done if there is an empty folder at the target (someone might want to create the folder and set write permissions on just that folder). If the target is not empty, but there is no d7 install there, then the situation should be detected and an error should be printed.
3. If there is no $databases or $db_url defined in the target alias, then then the site-upgrade will generate a corrupt $db_url and continue, resulting in errors later in the process. This situation should be detected and reported sooner.
4. If you use drush @source site-upgrade ... instead of drush --root=... --uri=... site-upgrade ... then the sql-sync will always fail. I'm sure this used to work in the past, but now what is going on is that sup calls sql-sync with the source "@self", but now the @self record is a bit smarter than it used to be, and it knows its original name. The database is fetched and stored under the cache under the original name, and sql-sync fails to find it under "@self". The db-fetch-and-cache code needs to get a little smarter to keep up with other changes in the code.

I will continue to work on these, esp. the last one.

Edit: Change to an ordered list above. A patch to fix #4 is in #1040052: self alias caching

This patch improves the error reporting per items 1-3 in #4, above. n.b. item 4 in #4 was committed as part of #1040052: self alias caching.

Regarding the missing $db_url in the target alias, site-upgrade will now use the target alias name as a basis to generate a database name if one is not provided, logging a warning if it does so.

Edit: Note that some of the changed lines in includes/sitealias.inc are simply the removal of tabs.

+++ commands/core/upgrade.drush.inc	26 Jan 2011 00:35:12 -0000
@@ -152,21 +165,32 @@ function drush_upgrade_site_upgrade($tar
+  drush_print_r($target_alias_databases);

debug line?

+++ commands/core/upgrade.drush.inc	26 Jan 2011 00:35:12 -0000
@@ -152,21 +165,32 @@ function drush_upgrade_site_upgrade($tar
+  if ($target_version <= 6) {

Are we trying to make this work for D5 => D6 upgrade? Not worth the support hassle, IMO. Or we don't mean 'target' here.

Actually #5 is broken. :( Working on it...

I ran into some issues with site-upgrade that were inconvenient at times. For example:

1. views uses a class called views_object, which it expects to be loaded by the Drupal 7 automatic class loader. However, this class loader does not work until updatedb is called on Drupal core. If you have already downloaded views, though, updatedb will attempt to run views' update actions, which will cause the update to fail with an unresolved reference to views_object. This happens whenever views is present, even if it is not enabled.
2. views-7.x introduces a new dependency on ctools that views-6.x did not have. This means that the user must figure out that ctools must be downloaded.
3. you must reset your theme to garland, as custom themes must be ported before they will work (of course); forgetting to follow this bit of advice from UPGRADE.txt can cause your site to WSOD.

If you followed the instructions in UPGRADE.txt, then site-upgrade worked fine, but I wanted it to be a bit more automated, so I can run through my site upgrades without having to step through UPGRADE.txt like a recipe book. I don't want to take my site offline and disable all of my non-core modules by hand. Drush should follow the instructions for us!

This patch does in fact straighten out a lot of the issues, and makes site-upgrades go a lot more smoothly with less preparation. There is documentation in the code, but the changes include:

1. Non-core modules are not downloaded up-front as they used to be; the site-upgrade starts off with only Drupal core; updatedb is called before any non-core modules are downloaded, which avoids problems with the updatedb of core
2. If the target alias does not define a database, site-upgrade will create a db spec for you.
3. After the database is sync'ed, all non-core modules are disabled, and the theme is reset to garland.
4. After the non-core modules are downloaded, drush orders them all, placing the modules with no dependencies at the beginning of the list, and other modules after their dependencies. This avoids problems such as #1012964: cache_token doesn't exist: TRUNCATE {cache_token} ; Array ( ) in cache_clear_all(), which you will see if you try to run updatedb on pathauto before it has run on token. Drush insures that updatedb is run on modules in the order they appear in this list.
5. site-upgrade no longer needs to call _pm_get_project_path
6. site-upgrade no longer needs to use 'bootstrap_cancel'; non-core modules are downloaded after updatedb is ran on Drupal core, and the target site is bootstrapped to download the non-core modules.
7. updatedb execution can take a long time, and backend_invoke does not show any output until the whole thing is done. Also, backend_invoke changes the order that log messages appear in the output, which can be confusing when there are warnings and errors in an update action. This patch introduces "interactive" backend invokes. In "interactive" mode, the script cannot receive any of the function results from backend_invoke; however, the output is printed as it becomes available, and log messages are ordered correctly in the output. Interactive mode also makes it possible for users to answer prompts (drush_confirm and drush_choice), even on remote machines; however, site-upgrade does not use this facility, and always passes --yes to its subprocesses. (n.b. we could potentially enhance interactive mode per #1003990: Print real time feedback from non-interactive --backend commands, but I don't know if that's valuable; for site-upgrade, it seems better to keep everything linear in one window.)
8. This patch also gives pm-enable the ability to download and enable its dependencies, if it can map from the extension name to the project name. ctools is special-cased, so that views-7.x will work. drush builds a persistent cache of extension -> project mappings whenever pm-download is called. This code is still in early stages, and can be enhanced in a number of useful ways (e.g. consult drupal.org or drush.ws for a complete directory of available modules, per #112692: Drupal ports collection - automate package installation and dependency checking.)

Also attached to this issue are the scripts I used to test this. Customize the 00-00 script to your environment. Yes, I should use the unit-testing framework... If you run the scripts, you will see drupal-6.20 downloaded, and a couple of modules added to it prior to a site-upgrade to 7.x-dev. The updatedb operation for the views module emits a bunch of spurious warnings, but I believe this is a normal situation for a views-7.x upgrade.

As you might guess from the description, this patch is kind of large -- regrettable, but these issues were all interrelated, and needed to be fixed together to make site-upgrade breeze through complex upgrade dependencies without causing a lot of head scratching. There is more work to be done, e.g. integration with #1043922: drush_backend_set_result() can be overwritten, but it's at a point where it's ready for review and input.

Wow. Making site-upgrade more user friendly is a terrific goal.

I actually finished unit tests for site-upgrade yesterday. See https://github.com/Cyrve/Drush-PHPUnit/blob/master/siteUpgradeTest.php

With this patch applied, the test still passes so we are off to a great start. We aren't done because site-upgrade crashes on an infinite loop in drush_upgrade_order_extensions(). Maybe it is because I have a completely virgin D6 site?

#4 is the only point I object to. Ordering of update functions is the responsibility of contrib modules. New in D7 is the ability for update functions to depend on other update functions. It sounds like a dependency is not working in your pathauto/token scenario. Given the existence of dependencies, I think it should be possible to run updatedb once, against both core and contrib modules. But I am OK with breaking that apart for clarity.

I will review the code ASAP.

-moshe

I could not reproduce the infinite loop. This could happen if you have modules that declare circular dependencies.

Also, I tried removing the call to reorder the extensions, and found that the upgrade still worked fine. I'm pretty sure I was having problems updating pathauto before token before; I don't know.

Will keep working on this.

Code looks good to me. Get rid of the ordering and perhaps we have a winner. Do we really need run updatedb after moving each contrib module into the target. Seems excessively cautious.

Need jonhattan or others to review the pm changes.

Thats a clever hack to use the D6 code and not yet upgraded D7 DB.

Moving the contrib modules around before running updatedb was critical when ordering the extensions, because updatedb will update everything it can find, enabled or not. When the extension ordering is removed, all of that other code disappears as well. Too bad I got an erroneous failure result when I tried it this way the first time; it's working fine now, and the code is much simpler.

I'll write up a help topic on upgrading once the code is rtbc.

+++ commands/core/upgrade.drush.inc	30 Jan 2011 17:48:40 -0000
@@ -31,10 +31,20 @@ function upgrade_drush_command() {
+      'no-modules' => 'Stop after updatedb; do not download and enable new versions of the site\'s modules.',
+      'no-enable' => 'Download and update, but do not enable the new version of the site\'s modules.',

Confusing. Could we call this core-only? "after updatedb" is ambigous.

Whats the user case for no-enable?

+++ commands/core/upgrade.drush.inc	30 Jan 2011 17:48:40 -0000
@@ -44,35 +54,76 @@ function upgrade_drush_command() {
+  // TODO: we could warn if the current version of Drupal is not
+  // the recommended release (Upgrade.txt recommendation.)

Drupal core now enforces this requirement. updatedb will halt. No need for drush to take action.

+++ commands/core/upgrade.drush.inc	30 Jan 2011 17:48:40 -0000
@@ -82,93 +133,164 @@ function drush_upgrade_site_upgrade($tar
+    if (drush_get_error()) return -1; // Early exit if we see an error.

Should be return FALSE now?

Powered by Dreditor.

Those are good suggestions. Here is an updated patch with those changes made. Also, "seven" is used for the admin theme if it has already been set as the admin theme in the d6 site; otherwise, "garland" is used. Some other simplifications were done, and drush topic docs-upgrade was added.

I tested the patch and it worked first try. RTBC. Minor buglets below. Also, lets commit to HEAD and then discuss with Mark about Drush4.

+++ commands/core/upgrade.drush.inc	2 Feb 2011 01:19:17 -0000
@@ -81,94 +127,170 @@ function drush_upgrade_site_upgrade($tar
+    // TODO: UPGRADE.txt says to delete sites/default/default.settings.php.  Is this necessary?

Not necessary. Thats some legacy cruft. Lets remove the TODO.

+++ commands/core/upgrade.drush.inc	2 Feb 2011 01:19:17 -0000
@@ -81,94 +127,170 @@ function drush_upgrade_site_upgrade($tar
+      // Make an alias record that uses the FILES from @self and the DATABASE from $target.

Lets say CODE instead of FILES just to remove any ambiguity with uploaded files directory

+++ commands/core/upgrade.drush.inc	2 Feb 2011 01:19:17 -0000
@@ -81,94 +127,170 @@ function drush_upgrade_site_upgrade($tar
+      // set theme back to garland per Upgrade.txt

Code comments should ideally start with capital and end with period as per http://drupal.org/coding-standards

Also, there are some leading whitespace cleanups needed.

Committed.

While the drush-4 vs. drush-5 question does not affect me personally, since I'm tracking drush-HEAD on all of my servers, I'm partial to seeing this in drush-4. A lot of people will be doing 6-to-7 upgrades with drush-stable, and having this will save a lot of time.

Backported to 4.x. Would very much like another maintainer to verify before I close however.

#1070558: Allow drush sup to be run multiple times, re-using same code each time further improves site-upgrade; if we could get that committed to HEAD and then 4.x, I'd be happy to confirm both at the same time.

Edit: I mean that patch and this patch at the same time on drush-4.x.

I tried this out and it worked nicely. I like all the messages like telling you that you need to use Content Migrate. The biggest problem I saw is that my non-core modules did not get re-enabled in the D7 site. As far as i could see, everything else worked fine.