Drupal 6 will be the first release where some admins will never have seen settings.php, ever. So asking them to edit it when they upgrade might come as a shock.

For those who've never touched their settings.php, I suggest we allow update.php to write a new file (exactly the same as the installer), then run update.php after that.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Priority: Normal » Critical

I agree, and this is release-critical.

Freso’s picture

And then there are those of us who have added a good deal of stuff into their settings.php, so it shouldn't (blindly) overwrite a custom settings.php.

dmitrig01’s picture

what about a settings-7.php for Durpal 7 settings, then it knows to look for that in conf_path. settings-8.php for Drupal 8 settings, etc.

chx’s picture

Status: Active » Needs review
FileSize
9.71 KB

I think Dmitri's solution might just work. First, untested attempt attached.

chx’s picture

FileSize
8.88 KB

Wrong patch, sry!

chx’s picture

FileSize
9.03 KB

Well, this is actually a lot simpler than we make it.

chx’s picture

I need to make global $db_url 'cos we load settings.php in bootstrap.inc and you cant load it ever again cos if someone added a function to there then the reward for our efforts would be a nice fatal error.

chx’s picture

FileSize
9.25 KB

Damien points out that even if settings.php wont be rewritten I still enforce its writeability. Fixed.

chx’s picture

FileSize
12.22 KB

Reroll against HEAD. I have moved again the install_check_requirements from install.php to install.inc and applied mine change so I am not rolling back whatever change that function had.

chx’s picture

FileSize
9.01 KB

With less debug and against HEAD.

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

Status: Needs work » Needs review

Re-submitting for testing after snafu this weekend.

webchick’s picture

Title: Allow update.php to regenerate settings.php » UNSTABLE-4 Blocker: Allow update.php to regenerate settings.php

Ok. Now that some bigger fish are in (DST, SQLite, etc.) I'm adding this to the list of UNSTABLE-4 blockers. I would like to get UNSTABLE-4 rolled the first week of December, and this patch is critical to making upgrades work for mere mortals.

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
9.2 KB

Note to anyone rerolling this patch in the future: should the install.php hunk fail, just move-paste the function to install.inc and change the first line to $requirements = $profile ? drupal_check_profile($profile) : array();.

catch’s picture

tried this but ran into some issues. chx couldn't reproduce - so posting my results here in the hope that a third person can test it and confirm either way:
[error] [client 127.0.0.1] PHP Fatal error: Call to undefined function drupal_set_session() in /home/catch/www/7/includes/bootstrap.inc on line 983
[Sun Feb 08 18:34:46 2009] [error] [client 127.0.0.1] PHP Stack trace:
[Sun Feb 08 18:34:46 2009] [error] [client 127.0.0.1] PHP 1. {main}() /home/catch/www/7/update.php:0
[Sun Feb 08 18:34:46 2009] [error] [client 127.0.0.1] PHP 2. update_check_requirements() /home/catch/www/7/update.php:705
[Sun Feb 08 18:34:46 2009] [error] [client 127.0.0.1] PHP 3. drupal_set_message() /home/catch/www/7/update.php:651

Then:
chx: I added include_once session.inc to the top of update.php
chx: now I get:
[Sun Feb 08 18:59:25 2009] [error] [client 127.0.0.1] PHP Fatal error: session_start(): Failed to initialize storage module: user (path: /var/lib/php5) in /home/catch/www/7/includes/session.inc on line 175
[Sun Feb 08 18:59:25 2009] [error] [client 127.0.0.1] PHP Stack trace:
[Sun Feb 08 18:59:25 2009] [error] [client 127.0.0.1] PHP 1. {main}() /home/catch/www/7/update.php:0
[Sun Feb 08 18:59:25 2009] [error] [client 127.0.0.1] PHP 2. update_check_requirements() /home/catch/www/7/update.php:708
[Sun Feb 08 18:59:25 2009] [error] [client 127.0.0.1] PHP 3. drupal_set_message() /home/catch/www/7/update.php:654
[Sun Feb 08 18:59:25 2009] [error] [client 127.0.0.1] PHP 4. drupal_set_session() /home/catch/www/7/includes/bootstrap.inc:983
[Sun Feb 08 18:59:25 2009] [error] [client 127.0.0.1] PHP 5. drupal_session_start() /home/catch/www/7/includes/session.inc:222
[Sun Feb 08 18:59:25 2009] [error] [client 127.0.0.1] PHP 6. session_start() /home/catch/www/7/includes/session.inc:175

I also found an interesting side-effect - if you've got an already installed Drupal 7 site, wipe the settings.php and replace it with an empty one, you can go to install.php, eventually get redirected to update.php and regenerate your settings without having to go in and recreate the settings manually. This was unintended according to chx, but doesn't do any harm I think.

chx’s picture

catch, the thing is -- you cant have a session early on because the DB is not there! So .. wtf?

David_Rothstein’s picture

Rewriting settings.php seems like a good approach here. Note that while discussing a different (not as good) approach to this problem at http://drupal.org/node/303889#comment-1078298 I wrote a function which parsed and rewrote the old $db_url string that I think was more robust than the code currently here.

I don't have time to roll it into the patch right now, but I'm pasting the function here for reference. I'm wondering if the mysql vs mysqli thing could have something to do with the problem @catch experienced above?

/**
 * Parse database connection URLs (in the old, pre-Drupal 7 format) and
 * return them as an array of database connection information.
 */
function _db_parse_url($db_url) {
  $databases = array();
  if (!is_array($db_url)) {
    $db_url = array('default' => $db_url);
  }
  foreach ($db_url as $database => $url) {
    $url = parse_url($url);
    $databases[$database]['default'] = array(
      // MySQLi uses the mysql driver.
      'driver' => $url['scheme'] == 'mysqli' ? 'mysql' : $url['scheme'],
      // Remove the leading slash to get the database name.
      'database' => substr(urldecode($url['path']), 1),
      'username' => urldecode($url['user']),
      'password' => isset($url['pass']) ? urldecode($url['pass']) : '',
      'host' => urldecode($url['host']),
      'port' => isset($url['port']) ? urldecode($url['port']) : '',
    );
  }
  return $databases;
}
chx’s picture

FileSize
9.28 KB

David, extremely nice catch about mysqli. The rest is not really different from my code, I think.

webchick’s picture

Title: UNSTABLE-4 Blocker: Allow update.php to regenerate settings.php » Allow update.php to regenerate settings.php
Issue tags: +Release blocker
chx’s picture

FileSize
9.33 KB
webchick’s picture

Status: Needs review » Needs work

Testing method:
1. Downloaded and set up a local webchick.net (D6) site.
2. Visit http://localhost/webchick.net, verified it was working.
3. cvs up -dPC -r HEAD
4. Visit http://localhost/webchick.net again.

Before patch:

Redirect to install.php, where I get:

<!--[if lt IE 7]> Fatal error: Call to undefined function garland_get_ie_styles() in /Users/webchick/Sites/webchick.net/themes/garland/maintenance-page.tpl.php on line 24 Call Stack: 0.0039 380132 1. {main}() /Users/webchick/Sites/webchick.net/install.php:0 0.0069 606676 2. install_main() /Users/webchick/Sites/webchick.net/install.php:1183 0.1734 5689604 3. install_select_profile() /Users/webchick/Sites/webchick.net/install.php:98 0.1854 6264948 4. theme() /Users/webchick/Sites/webchick.net/install.php:434 0.1855 6268308 5. call_user_func_array() /Users/webchick/Sites/webchick.net/includes/theme.inc:628 0.1855 6268308 6. theme_install_page() /Users/webchick/Sites/webchick.net/includes/theme.inc:0 0.1863 6275648 7. theme_render_template() /Users/webchick/Sites/webchick.net/includes/theme.maintenance.inc:149 0.1867 6337392 8. include('/Users/webchick/Sites/webchick.net/themes/garland/maintenance-page.tpl.php') /Users/webchick/Sites/webchick.net/includes/theme.inc:1016 

After patch:
Same thing. ;(

chx’s picture

Status: Needs work » Needs review
FileSize
9.68 KB

Grrr! The sessions patch broke this nicely as drupal_set_message calls drupal_set_session... which wont fly really as session.inc is not even loaded.

webchick’s picture

Status: Needs review » Needs work

After some futzing around, I figured out the reason I was getting that error was because I had a hacked version of Garland sitting in sites/all/themes. Once I removed that things started working much better. :)

update.php now completes, however I get Fatal error: Call to undefined function field_attach_load() in /Users/webchick/Sites/webchick.net/modules/node/node.module on line 917

chx’s picture

Status: Needs work » Needs review
FileSize
10.25 KB
webchick’s picture

Status: Needs review » Fixed

Tested this with webchick.net, and as long as I switch to Garland first, this latest patch updates without incident. I'm sure there are many many more edge cases we'll run across, but this fixes the main issue. Committed to HEAD with small doxygen improvement. Thanks so much! :D

David_Rothstein’s picture

FileSize
39.35 KB

It's definitely good this went in, but trying out the new version I found several problems and wrote a patch to fix them. Here are the issues I found:

  1. When visiting update.php with a settings.php file that is not yet writable, the error message you get looks like this:

    This makes reference to the install process and has some other weird things too (e.g., the parenthetical remark at the end). Also, if you click on the Continue button, it redirects you to install.php instead. Overall, since the message we're sending here is different from install.php, maybe it's actually best not to try reusing that code here.
  2. Regarding this:
    -    if (!isset($_SESSION['messages'])) {
    +    if (!isset($_SESSION['messages']) && function_exists('drupal_set_session')) {

    I'm not sure that putting that special-case code into drupal_set_message() just to deal with the update.php situation is a good idea. I think update.php should deal with the special cases itself.
  3. I think we do need to use the function from #18 (above) for parsing the URL, because the other thing that function allowed was the possibility that $db_url is an array containing more than one database. Since D6 allows this, I think we should support that conversion in the upgrade path too.
David_Rothstein’s picture

Status: Fixed » Needs review
FileSize
19.21 KB

Here is the patch that fixes the above issues. Mostly, it moves the install_check_requirements() function back to install.php, and then implements a separate, cleaner check in update.php itself that is optimized for the update.php situation.

David_Rothstein’s picture

FileSize
68.4 KB

Also, here's a screenshot showing what the update.php requirements check looks like with the new patch.

We might want to tweak some things and perhaps move the order of operations around a bit (so that this information isn't exposed until after the access check is performed), but overall, this should be good because it makes the requirements checking look similar to the way it is done in install.php as well as on the Status Report page.

Bojhan’s picture

So does this patch, at that verify requirements page to the update process? Or did I always just miss that.

Dries’s picture

I reviewed this patch and it looks good. I haven't tested it yet. I'll commit it after a test -- either by myself or someone else. Thanks!

David_Rothstein’s picture

Well, no one else tested it, so I went and tested my own patch again - and it still works :)

However, as I hinted at in #29, I do think it's important to make sure we don't show this page until the access check has been performed (otherwise we are printing tons of new information about the server for anyone who comes along to see). This is fixed in the attached patch - which has also been tested by me and appears to work fine...

@Bojhan: Yes, the screenshot shown in #29 is added by this patch. Before, the checks were performed, but errors were printed to the screen as shown in the screenshot from #27. So basically I'm switching it to use the same style used elsewhere in Drupal (e.g., install.php and the status reports page), which hopefully should be a good thing!

catch’s picture

Status: Needs review » Needs work

Until #278592: Sync 6.x extra updates with HEAD is in I don't see how we can remove update_fix_d6_requirements()

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
16.75 KB

Well, it's not like direct Drupal 5 => Drupal 7 updates are working now anyway :) But as long as there's a separate issue to track it, I agree there's no reason to remove it here.

New patch attached.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Sure. My goal was to let an update happen. Making it pretty, that's another thing :) thanks.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Release blocker

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