Updated: Comment #3

Problem/Motivation

In drupal 7 it used to be possible to just touch settings.php and run the installer.

Since people have habits and they have relied on them in the past, they will try d8 with their habits before reading and following the install readme.

Proposed resolution

Lets make this reasonable habit work.

steps to reproduce

  1. remove sites directory
  2. drop database
  3. create an empty settings.php, for example: touch settings.php; (do NOT copy default settings file)
  4. install using the UI installer

without this fix:
the page source during the install gets to:

$databases['default']['default'] = array (
  'database' => 'd8',
  'username' => 'root',
  'password' => 'root',
  'host' => 'localhost',
  'port' => '',
  'namespace' => 'Drupal\\Core\\Database\\Driver\\mysql',
  'driver' => 'mysql',
  'prefix' => '',
);
$drupal_hash_salt = 'L0Q3rV7O67pT3c9DA63z5TYReVP7D81BU-igrkbIhpg';
$settings['install_profile'] = 'standard';
$config_directories['active']['path'] = 'config_E01oONi5L8jJLLxa-1n_jqVcgxVTCqdNLwH65-lnMFc/active';
$config_directories['staging']['path'] = 'config_E01oONi5L8jJLLxa-1n_jqVcgxVTCqdNLwH65-lnMFc/staging';
<!DOCTYPE html>
<html lang="" dir="ltr" class="install-background">
<head>
  <title>Database configuration | Drupal</title>
  <meta charset="utf-8" />
<link rel="shortcut icon"

also gives warnings like

Warning: Cannot modify header information - headers already sent by (output started at /foo/sites/default/settings.php:15) in drupal_send_headers() (line 1152 of core/includes/bootstrap.inc).

With the patch:
works normally.

Remaining tasks

  • (done) add test
  • review

User interface changes

No.

API changes

No.

CommentFileSizeAuthor
#20 2122693.20.patch793 bytesalexpott
#15 2122693.14.test-only.patch2.67 KBsun
#14 2122693.14.patch3.3 KBjayeshanandani
#12 2122693.12.patch2.05 KBjayeshanandani
#12 interdiff.txt2.06 KBjayeshanandani
#1 2122693.1.patch1.89 KBalexpott
#2 installerrorempty.png784.94 KBYesCT
#3 interdiff-2122693-1-3.txt1.81 KBYesCT
#3 2122693.3.patch2.11 KBYesCT
#5 2122693-BMDan.patch5.35 KBBMDan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Active » Needs review
FileSize
2122693.1.patch1.89 KB
YesCT’s picture

YesCT CreditAttribution: YesCT commented
FileSize
installerrorempty.png784.94 KB

after a few steps in the installer, (and scrolling over to the right a long long way) see:

installerrorempty.png

YesCT’s picture

YesCT CreditAttribution: YesCT commented
FileSize
interdiff-2122693-1-3.txt1.81 KB
2122693.3.patch2.11 KB

php storm notified me that variable_get() is deprecated but seems we might need to use it here.

this fixes some comments.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes

template and add steps and example of the settings in the page source.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes

Updated issue summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow commented

Manually tested

1. Installed Drupal without: touch sites/default/settings.php - Worked greate
2 Installed Drupal with: touch sites/default/settings.php - Had same error as #2
3. Applied patch from #3
4. Installed Drupal with: touch sites/default/settings.php - Worked great

Note to re-install drupal 8:
1. dropped database
2. rm -rf sites
3. git checkout sites

Next step code review

BMDan’s picture

BMDan CreditAttribution: BMDan commented
FileSize
2122693-BMDan.patch5.35 KB

@alexpott, @YesCT, and others put in yeoman's work to track down this bug at the code sprint at BADCamp after I triggered it by means of my lazy install practices. Since that time, I've spent quite a few hours in the air disconnected from d.o, so I created a parallel patch that takes a slightly different approach. While I was at it, I also made REQUIREMENT_WARNING clearly identify what it means by "try again", since it bothered me that its semantics were painfully unclear.

Status: Needs review » Needs work

The last submitted patch, 2122693-BMDan.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented 
+++ b/core/includes/install.core.inc
@@ -2419,12 +2421,19 @@ function install_check_requirements($install_state) {
+    elseif (!$exists || $settings_md5 == 'd41d8cd98f00b204e9800998ecf8427e') {

This looks like it might cause issues for hosting companies providing their own settings.php file. I'm inclined to think that the solution in #3 looks like a nice way of preventing the regression.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue summary: View changes

added the text of the warning, so it is google-able

BMDan’s picture

BMDan CreditAttribution: BMDan commented

As I see it, there are three different cases here:

  1. settings.php matches default_settings? Great, continue.
  2. settings.php is blank? Great, (attempt to) copy default and continue.
  3. "Other"—settings.php is not blank, but doesn't match default, either? We should make sure nothing untoward is afoot. Hosting companies can easily avoid this warning simply by modifying the default as well as the main file.

I happen to believe that #3 deserves special consideration. But it's not a big deal.

Regardless, I think file_md5() is a better option than file_get_contents if we can swing it. file_md5() doesn't hit us with the memory usage problems of file_get_contents. Admittedly, this should be a rare problem, but if it's this trivial to avoid triggering it, we should at least try.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue summary: View changes
Related issues: +#2155701: Installer starts with fatal error/exception "table 'semaphore' not found" if settings.php contains $databases already, +#2156401: Write install_profile value to configuration and only to settings.php if it is writeable
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

#3 is the proper approach from my perspective — there's no reason to throw a requirements warning/error, if the file happens to be empty, but can be rewritten.

However, two remarks:

  1. We should use trim($content) === '', so as to account for white-space (when people use echo > settings.php instead of touch settings.php, and also use strict comparison to ensure that it's really an empty string.
  2. The added test assertion was copied from an existing and can be simplified.
jayeshanandani’s picture

jayeshanandani CreditAttribution: jayeshanandani commented
Assigned: Unassigned » jayeshanandani
jayeshanandani’s picture

jayeshanandani CreditAttribution: jayeshanandani commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.06 KB
2122693.12.patch2.05 KB

Updated the #3 patch with modifications as per the comments.
Attaching interdiff for more review.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Thanks! :-)

The last remaining step is to write a test for this bug. We weren't able to add tests for such bugs in the past, which explains the poor test coverage of the installer, but since very recently, we're able to write automated tests for the interactive installer. :)

Doing so is simple:

  1. Create core/modules/system/lib/Drupal/system/Tests/Installer/InstallerEmptySettingsTest.php
  2. class InstallerEmptySettingsTest extends InstallerTestBase
  3. Implement function setUp() to touch($this->siteDirectory . '/settings.php'); and call parent::setUp() afterwards.
  4. Add a test method to confirm that installation succeeded. See InstallerTranslationTest for an example.
  5. Confirm that the test fails without the functional code changes.
jayeshanandani’s picture

jayeshanandani CreditAttribution: jayeshanandani commented
FileSize
2122693.14.patch3.3 KB

Added the missing tests.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
2122693.14.test-only.patch2.67 KB

Now let's confirm that both tests actually catch the bug. :)

Status: Needs review » Needs work

The last submitted patch, 15: 2122693.14.test-only.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Reviewed & tested by the community

Alrighty :-) Both tests failed as expected.

The test failures of the new InstallerEmptySettingsTest are quite monumental, but I don't see how we could error out earlier.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Well, we definitely need to fix this, cos without that we have an information disclosure vulnerability. It feels a little jankalicious to be making up PHP in a string like this, but I don't immediately have grander suggestions. So...

Committed and pushed to 8.x. Thanks!

  • Commit cd7841b on 8.x by webchick: 
    Issue #2122693 by jayeshanandani, YesCT, sun, alexpott, BMDan: Installer...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: Installer does not work on a completely empty settings.php » [HEAD BROKEN] Installer does not work on a completely empty settings.php
Status: Fixed » Needs review
FileSize
2122693.20.patch793 bytes

This is currently break head :( due to #2219009: Improve DX of Settings class

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Reviewed & tested by the community

That class already has use Drupal\Component\Utility\Settings; so this looks good.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • Commit fba6f74 on 8.x by catch: 
    Issue #2122693 by alexpott: [HEAD BROKEN] follow-up to use Settings...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Title: [HEAD BROKEN] Installer does not work on a completely empty settings.php » Installer does not work on a completely empty settings.php

Reverting title.

Status: Fixed » Closed (fixed)

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