Allow update.php to regenerate settings.php
catch - September 4, 2008 - 21:47
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | update system |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | Release blocker |
Description
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.
#1
I agree, and this is release-critical.
#2
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.
#3
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.
#4
I think Dmitri's solution might just work. First, untested attempt attached.
#5
Wrong patch, sry!
#6
Well, this is actually a lot simpler than we make it.
#7
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.
#8
Damien points out that even if settings.php wont be rewritten I still enforce its writeability. Fixed.
#9
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.
#10
With less debug and against HEAD.
#11
The last submitted patch failed testing.
#12
Re-submitting for testing after snafu this weekend.
#13
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.
#14
The last submitted patch failed testing.
#15
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();.#16
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.
#17
catch, the thing is -- you cant have a session early on because the DB is not there! So .. wtf?
#18
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?
<?php/**
* 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;
}
?>
#19
David, extremely nice catch about mysqli. The rest is not really different from my code, I think.
#20
#21
#22
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:1016After patch:
Same thing. ;(
#23
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.
#24
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
#25
#26
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
#27
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:
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.
- 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.
#28
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.
#29
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.
#30
So does this patch, at that verify requirements page to the update process? Or did I always just miss that.
#31
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!
#32
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!
#33
Until #278592: Sync 6.x extra updates with HEAD is in I don't see how we can remove update_fix_d6_requirements()
#34
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.
#35
Sure. My goal was to let an update happen. Making it pretty, that's another thing :) thanks.
#36
Committed to CVS HEAD. Thanks!
#37
Automatically closed -- issue fixed for 2 weeks with no activity.