Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#34 | update_settings_php_cleanup_3.patch | 16.75 KB | David_Rothstein |
#32 | update_settings_php_cleanup_2.patch | 19.91 KB | David_Rothstein |
#29 | update_requirements_new.png | 68.4 KB | David_Rothstein |
#28 | update_settings_php_cleanup.patch | 19.21 KB | David_Rothstein |
#27 | update_requirements_old.png | 39.35 KB | David_Rothstein |
Comments
Comment #1
webchickI agree, and this is release-critical.
Comment #2
Freso CreditAttribution: Freso commentedAnd 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.
Comment #3
dmitrig01 CreditAttribution: dmitrig01 commentedwhat 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.
Comment #4
chx CreditAttribution: chx commentedI think Dmitri's solution might just work. First, untested attempt attached.
Comment #5
chx CreditAttribution: chx commentedWrong patch, sry!
Comment #6
chx CreditAttribution: chx commentedWell, this is actually a lot simpler than we make it.
Comment #7
chx CreditAttribution: chx commentedI 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.
Comment #8
chx CreditAttribution: chx commentedDamien points out that even if settings.php wont be rewritten I still enforce its writeability. Fixed.
Comment #9
chx CreditAttribution: chx commentedReroll 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.
Comment #10
chx CreditAttribution: chx commentedWith less debug and against HEAD.
Comment #12
webchickRe-submitting for testing after snafu this weekend.
Comment #13
webchickOk. 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.
Comment #15
chx CreditAttribution: chx commentedNote 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();
.Comment #16
catchtried 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.
Comment #17
chx CreditAttribution: chx commentedcatch, the thing is -- you cant have a session early on because the DB is not there! So .. wtf?
Comment #18
David_Rothstein CreditAttribution: David_Rothstein commentedRewriting 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?
Comment #19
chx CreditAttribution: chx commentedDavid, extremely nice catch about mysqli. The rest is not really different from my code, I think.
Comment #20
webchickComment #21
chx CreditAttribution: chx commentedComment #22
webchickTesting 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:
After patch:
Same thing. ;(
Comment #23
chx CreditAttribution: chx commentedGrrr! 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.
Comment #24
webchickAfter 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
Comment #25
chx CreditAttribution: chx commentedComment #26
webchickTested 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
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedIt'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.
Comment #28
David_Rothstein CreditAttribution: David_Rothstein commentedHere 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.
Comment #29
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, 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.
Comment #30
Bojhan CreditAttribution: Bojhan commentedSo does this patch, at that verify requirements page to the update process? Or did I always just miss that.
Comment #31
Dries CreditAttribution: Dries commentedI 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!
Comment #32
David_Rothstein CreditAttribution: David_Rothstein commentedWell, 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!
Comment #33
catchUntil #278592: Sync 6.x extra updates with HEAD is in I don't see how we can remove update_fix_d6_requirements()
Comment #34
David_Rothstein CreditAttribution: David_Rothstein commentedWell, 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.
Comment #35
chx CreditAttribution: chx commentedSure. My goal was to let an update happen. Making it pretty, that's another thing :) thanks.
Comment #36
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!