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.
setting
$conf = array(
'anonymous' => 'Visitor',
'site_name' => 'Drupal 6 Testing Site',
'site_mail' => 'mail@example.com',
in ./sites/default/settings.php
will give the notice: Drupal already installed during installation.
You won't be able to set an administrator account and when visiting your site, you won't be able to login.
Comment | File | Size | Author |
---|---|---|---|
#23 | 212284-23-install-security-check.patch | 822 bytes | alexanderpas |
#20 | 212284-install-security-check.patch | 842 bytes | alexanderpas |
#19 | 212284-19.install-security-check.patch | 842 bytes | dww |
#12 | 212284-install-security-check.patch | 1.12 KB | Damien Tournoud |
#10 | 212284-install-security-check-d7.patch | 1.12 KB | Damien Tournoud |
Comments
Comment #1
agentrickardIs this an installer problem or a documentation problem?
I think documentation. Since the installer (re)writes settings.php, you should not be editing that file by hand before running install.php.
Comment #2
alexanderpas CreditAttribution: alexanderpas commentedit is NOT a documentation problem.
installer doesn't write to
settings.php
when$db_url
is already pointing to a EMPTY database.think about re-installations after dropping all tables in your drupal database.
also, think about multisite installations.
Comment #4
agentrickardFair enough, thanks.
Comment #5
dvessel CreditAttribution: dvessel commentedHow is this critical? This is how Drupal 5 and earlier behaves. You drop the db, then scrap your settings.php and start over.
Comment #6
theborg CreditAttribution: theborg commentedThe security check in the 'configure' stage of the installation is not compatible with the possibility of overiding variables.
Comment #7
alexanderpas CreditAttribution: alexanderpas commentedmore detailed:
The security check in the 'configure' stage of the installation is not compatible with the possibility of overiding variables: site_name and site_mail
putting this back to critical because it'll BREAK the installation since it'll leave you with the
placeholder-for-uid-1
in the database without you being able to set an administrator account for yourself.remember, you can reproduce this behavior just by setting:
site_name
orsite_mail
in settings.phpnote that site_name is actually used as an example in settings.php
Comment #8
catchIt's trivial to comment these out before you reinstall. If you're editing this part of settings.php, then you know how to do this. So won't fixing this.
I've reinstalled drupal dozens of times with db information in the settings.php, that works fine, and would be critical if broken, this is not the same thing at all.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's reopen.
This "security check" feels really dumb to me. It makes it impossible to have more complex installation profiles and makes it impossible to reinstall a site that has its site name configured in
settings.php
(which is a very good idea, because it makes the proper site name appears in the maintenance page when the database is not available).Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedI suggest we just remove the check, which is just babysitting broken installations. If the installation is missing a variable, it is very likely it is missing plenty others. There is nothing we can to do protect users messing up with the database against themselves.
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commentedBumping this.
Comment #12
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #13
dwwThis stupidity keeps nailing me, too. DamZ is right. This is a pointless check that actively harms people trying to do the right thing. Tested manually and it works to (re)install when you already have site_name defined in settings.php.
Comment #14
dwwp.s. I also checked and this is *not* the only time we throw that exception, so we still want to keep the install_already_done_error() function around for the other valid cases when we hit that condition and want to abort the installation.
Comment #15
chx CreditAttribution: chx commentedgiven the impossibility to test this (no, Virginia, we are NOT changing settings.php for a test) I agree with it.
Comment #16
catchI'm fine with just removing this.
Issue is not tagged for backport, but looks like it should be?
Comment #17
catch#12: 212284-install-security-check.patch queued for re-testing.
Comment #19
dwwRerolled for new directory layout.
Comment #20
alexanderpas CreditAttribution: alexanderpas commentedhave a re-roll
edit: ninja'ed
Comment #21
agentrickardLooks good. Hits the target and tests are green.
Comment #22
catchCommitted/pushed to 8.x.
Looks backportable to me.
Comment #23
alexanderpas CreditAttribution: alexanderpas commentedthis should do it.
Comment #24
agentrickardHooray for removing old cruft.
Comment #25
webchickI guess given that this has 3-4 members of the security team saying +1, the security check is ok to remove. I'm still not quite sure how people are actively hitting this bug in the field, though. Hrm.
Anyway, committed and pushed to 7.x. Thanks!
Comment #26
dww@webchick: To reproduce:
- Get a clean copy of core (prior to the fix, of course)
- Put
$conf['site_name'] = 'local D7 test site';
in your settings.php (perhaps along with your DB credentials to save time).- Visit install.php or try to run
drush site-install
I don't think that's a crazy edge-case. It's pretty common to want to specify site_name in settings.php. Once you do that, you can no longer reinstall the site (e.g. rebuilding a site from a .make file and features or something).
Anyway, yay this is gone from D7 and D8, thanks! Is it worth back-porting this all the way to D6? The same basic bug is there, but the code in D6 lives directly in install.php so this needs to be ported if we want it. I'm in favor, but I don't want to spend energy if it's going to be shot down. Thoughts?
Thanks,
-Derek