There are several steps that need to take place during installation in order to get the configuration system up and running. Right now this functionality lives in install_settings_form_submit() in install.core.inc. Unfortunately there are situations where that function never gets called. For instance, if your settings.php already has database information hand-entered into it, or you are performing an unattended installation. Therefore this code needs to be moved somewhere more appropriate. where it will always be executed. We also need to allow for the usecase where someone wants to hand-code in a configuration location and leave their settings.php read-only (just like we do with the database settings.)

Files: 
CommentFileSizeAuthor
#29 1464944-29.config-install.patch10.79 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 36,613 pass(es). View
#29 config-install.interdiff.txt633 bytesalexpott
#26 1464944-26.config-install.patch10.25 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 36,599 pass(es). View
#26 interdiff-16-26.txt4.63 KBDavid_Rothstein
#21 146944-21.patch11.14 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 36,590 pass(es). View
#21 146944-19-interdiff.txt1.4 KBalexpott
#19 146944-19.patch9.93 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 36,591 pass(es). View
#19 146944-16-interdiff.txt728 bytesalexpott
#16 146944-16.patch9.39 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 36,587 pass(es). View
#14 1464944-14.config-install.patch9.44 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 35,900 pass(es). View
#8 1464944-8.config-install.patch5.29 KBksenzee
PASSED: [[SimpleTest]]: [MySQL] 35,683 pass(es). View
#3 1464944_installer.patch4.86 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 35,137 pass(es). View
#1 1464944_installer.patch3.54 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 35,106 pass(es). View

Comments

heyrocker’s picture

Status: Active » Needs review
FileSize
3.54 KB
PASSED: [[SimpleTest]]: [MySQL] 35,106 pass(es). View

Here's a first pass at this. It will work if you install with a value already set for your config directory in settings.php, as well as going through the normal installation process. The only part I was wondering is whether I should be throwing an Exception in install_verify_settings(), but that already happens if your database info is wrong and I think if you've specified a config directory that isn't there then its worth aborting over.

cosmicdreams’s picture

In order to manually test the functionality you describe in the OP, what should I set in my settings.php so that I can manually set the config directory?

heyrocker’s picture

FileSize
4.86 KB
PASSED: [[SimpleTest]]: [MySQL] 35,137 pass(es). View

First, you use this patch instead of the last one which had a bug.

Then do the following:

- Open your settings.php and set $config_directory_name to be whatever you want.
- Create a directory named the same as above in sites/default/files, and make sure it is writeable by the web server
- Install

After installation is done, you should have several xml files in this directory.

You should also test a normal installation with a fresh settings.php, in which case you should get a randomly named directory under files, with xml files in it.

A final test would be an installation in which you set both the config directory AND the database settings by hand in settings.php. This should result in the same result as the first test.

cosmicdreams’s picture

Manually tested this patch.

Setup steps

  • added the variable declaration that heyrocker requested in my settings.php
  • dropped my current dev site's database
  • created the appropriately named folder in the /sites/default/files directory

Results

  • When the appropriately named folder didn't have the proper permissions, I received a helpful error message
  • When I allowed the web server to write to the folder, I was allowed to proceed with the installation.
  • Have to chose which installation profile I wanted to install, I immediately started the installation process (as expected)
  • Installation and final config steps occurred without any reported errors.
  • After installation all of the files per properly stored in the chosen directory.
catch’s picture

 function install_system_module(&$install_state) {
+  global $config_directory_name;
+
+  // Actually create the config directory named above.

Could we add another helper function so it's not inside this one? Even if it means changing the name of the install step and having the same one cover both tasks, just seems an odd place to put this.

heyrocker’s picture

It makes a certain amount of sense to me but I can see why its weird. I mean, this same work will be part of system module's upgrade path in #1464554: Upgrade path for image style configuration too. Ultimately, it didn't make any sense to me to put it anywhere else, and even just changing the name of this function is a much more invasive change to the installer which I'm somewhat reluctant to do given its fragility and lack of automated testing. In the annals of 'Horrors Committed In The Name Of Installing Drupal' I feel like this barely registers. Maybe a followup?

heyrocker’s picture

Issue tags: +Configuration system

tagging

ksenzee’s picture

FileSize
5.29 KB
PASSED: [[SimpleTest]]: [MySQL] 35,683 pass(es). View

I'm not any more excited about changing things in the installer than heyrocker is. Here's a compromise patch that doesn't change the name of the task, but does break the config system stuff out into a helper function. I didn't change anything else but comments.

I've tested this in every way I could think of, via drush si and install.php, with $config_directory_name set in settings.php and without it, with an existing database and without it, with a directory already existing with correct permissions and without them, and with no directory in place. It seems to work fine.

David_Rothstein’s picture

Component: configuration system » install system
Status: Needs review » Needs work
  1. I tried installing Drupal with this patch and a non-writable settings.php file (with database credentials already included in it). The result was that Drupal installed "successfully" but $config_directory_name was still an empty string. So, this needs work.

    Mainly the issue seems to be that install_verify_settings() should be returning FALSE when $config_directory_name is empty, but the patch doesn't actually make it do that?

  2. Do we also have to deal with the case of an empty $config_signature_key? Currently, it seems like the code allows you to leave that blank in settings.php and doesn't have fallback logic to deal with that case (like drupal_get_hash_salt() does).
  3. The only part I was wondering is whether I should be throwing an Exception in install_verify_settings(), but that already happens if your database info is wrong and I think if you've specified a config directory that isn't there then its worth aborting over.

    As far as I know, install_verify_settings() always returns a boolean, and any exceptions are caught (at least they are certainly supposed to be). So the same should be true for any new code added here.

    If your database info is wrong in settings.php, the current behavior is that the database configuration screen will be displayed to you and you get a chance to input new database info that is actually correct. Since there's no UI in the installer for specifying a config directory, I would think the equivalent behavior would instead be to just flag this as an error during the "Verify requirements" step?

  4. I think the helper function install_create_config_directory() would really be cleaner as a separate task within install_tasks().
David_Rothstein’s picture

I think the helper function install_create_config_directory() would really be cleaner as a separate task within install_tasks().

Actually, I just noticed that install_system_module() is already really poorly named (it installs the user module too) so maybe better to just leave it in there and rename it in some other issue after all.

(Although it would be very easy to rename it in this issue too; I don't understand why that's considered an invasive change.)

ksenzee’s picture

I tried installing Drupal with this patch and a non-writable settings.php file

I didn't think of trying that. It occurs to me that if we're not going to have automated tests for the installer, we should at least have documented functional test cases somewhere, so every person who sets out to test installation doesn't have to scratch their head wondering what else they can try to break.

Although it would be very easy to rename it in this issue too; I don't understand why that's considered an invasive change.

I imagine it's only an invasive change to people (like me) to whom the installation system is total voodoo. I honestly have no idea what I would put in install_tasks() to get the result I want here.

heyrocker’s picture

#9: 1) I actually intentionally didn't address this, because I don't think there is a use case for it. Either you will be pre-entering all the details (for an automated/command line/read-only deployment) or none of them. Checking this makes the verification code a bit more complicated and I didn't think it was worth the bother. If others think differently we can do it thought.

David_Rothstein’s picture

Well, I agree no one would modify settings.php that way intentionally, but it could easily happen by mistake, right? (For example, someone who is used to setting up sites the Drupal 7 way, or maybe they just forgot.) In that case I think it is worth preventing the install from going forward, because if you let it go through things will be kind of broken.

I started working on a rerolled version of this patch (to address the issues above) and I think it might not be hard to do all this validation correctly; I realized that we do have some file directory validation code that runs during install already, and if we add the config directory to the list of directories that are validated, then it helps keep things simpler and avoids dealing with some of the edge cases because they're already dealt with. I'll try to finish up that patch sometime later today.

It occurs to me that if we're not going to have automated tests for the installer, we should at least have documented functional test cases somewhere, so every person who sets out to test installation doesn't have to scratch their head wondering what else they can try to break.

There's #630446: Allow SimpleTest to test the non-interactive installer which may be possible in Drupal 8, although I suppose even then, testing things like file permissions of settings.php might be impossible. I like the idea of compiling a manual list and documenting it. The list of tests that have been done in this issue would be a good starting point for that.

I imagine it's only an invasive change to people (like me) to whom the installation system is total voodoo. I honestly have no idea what I would put in install_tasks() to get the result I want here.

There is pretty detailed documentation of hook_install_tasks(); but even easier, in the end we were just talking about renaming a function, "install_system_module" => "something_else" (maybe "install_base_system"?), which is literally just find-and-replace. That's why I didn't think it was invasive. Guess we'll see if it makes sense to do that as part of this issue or not.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
9.44 KB
PASSED: [[SimpleTest]]: [MySQL] 35,900 pass(es). View

The attached patch needs a bit more testing but I think it will work. It uses this basic strategy:

  1. Add the config directory to the existing list of directories in system_requirements(), so that if it's already defined in settings.php the installer will check it on the requirements page. (This also has the side effect of checking it on the status report page of a running site, which probably makes sense because if the config directory ever becomes unwritable, you want to know about it.)
  2. Use similar logic to check this directory when verifying settings.php, so that anyone without a valid config directory will be forced to go through the database settings form.
  3. Use similar logic in the submit handler of the database settings form, so that a correct config directory is created when settings.php is rewritten.
  4. For the .htaccess file, I realized that we don't actually need to special-case the config directory here. We actually want .htaccess created for all relevant directories, and we can call file_ensure_htaccess() in the installer to do that. The nice thing about doing it that way is that I think it (mostly) fixes #1354994: files/.htaccess not created for all sites - D6, D7, D8 as well.

This patch should address all issues raised above except for the empty $config_signature_key issue. I figured it's not worth bothering yet since #1444620: Remove file signing from configuration system may remove it, but if this issue gets fixed before that one we should track it somewhere.

xjm’s picture

I added a note to #1464944 #630446: Allow SimpleTest to test the non-interactive installer about the various installation scenarios we have in this issue; feel free to correct/clarify there.

Edited to reference the correct issue (as opposed to this one).

heyrocker’s picture

FileSize
9.39 KB
PASSED: [[SimpleTest]]: [MySQL] 36,587 pass(es). View

Here is a new rerolled patch to head. #1444620: Remove file signing from configuration system did in fact land, so I replaced the reference to config_signature_key with

'value' => $config_directory_name ? $config_directory_name : 'config_' . drupal_hmac_base64('', session_id() . drupal_hash_base64(drupal_random_bytes(55)) . $settings['drupal_hash_salt']['value']),

I have no idea if this is a valid approach or not, it just uses the same key generation mechanism as drupal_get_token() does. We don't actually need to save this key for anything, since we never have to reverse or verify the generated directory name - we just make it and save it.

I still need to do a proper review and testing, but I wanted to get this up while I'm going through all that.

heyrocker’s picture

I just did some testing of this patch and it works as advertised in all the scenarios listed above. Additionally, the code makes a lot more sense now. Thanks David. The only slight irritation I can see someone complaining about is that they still have to go through the database form if they have entered database settings but not a config directory, but I also consider this a total edge case so I'm not really going to worry about it.

The only question then is if the code I changed to generate the config dir above is acceptable. If so, I consider this RTBC, but someone else should really do it. Will be nice to get a critical out of the way.

alexpott’s picture

I think there might be an issue with the patch...

Steps to reproduce:

  1. Create empty db
  2. Create settings.php with valid database config and no configuration directory
  3. Goto /install.php
  4. Click through the forms... and do not enter a password into the database settings form
  5. Now you're stuck - the database has drupal tables but settings.php has an invalid database configuration
alexpott’s picture

FileSize
728 bytes
9.93 KB
PASSED: [[SimpleTest]]: [MySQL] 36,591 pass(es). View

Patch attached resolves issue described in #18

sun’s picture

Status: Needs review » Needs work
+++ b/core/includes/install.core.inc
@@ -135,6 +135,9 @@ function install_state_defaults() {
     // This becomes TRUE only when Drupal's system module is installed.
     'database_tables_exist' => FALSE,
+    // This becomes TRUE only when a valid database connection can be
+    // established.
+    'database_verified' => FALSE,

Chronologically and logically, the second comes before the first, so let's move it before that.

+++ b/core/includes/install.core.inc
@@ -297,9 +301,10 @@ function install_begin_request(&$install_state) {
   // Check existing settings.php.
-  $install_state['settings_verified'] = install_verify_settings();
+  $install_state['database_verified'] = install_verify_database_settings();
+  $install_state['settings_verified'] = install_ensure_config_directory() && $install_state['database_verified'];

After reviewing this patch in detail, I believe it's a good idea to split these flags, but we really should go the extra mile and also add a separate 'config_verified' flag.

Especially the install_ensure_config_directory() happens way too early and will thus create a config directory in a location where no config directory should be.

+++ b/core/includes/install.core.inc
@@ -778,15 +786,22 @@ function install_verify_requirements(&$install_state) {
+function install_base_system(&$install_state) {
...
+  // Ensure that all directories created by the installer have appropriate
+  // .htaccess files. This is done here because the function relies on
+  // system.module in order to work, and because at this point in the installer
+  // all the necessary directories (including the config directory) have been
+  // created.
+  file_ensure_htaccess();

I'm no longer able to distill which exact directories are being referenced here. According to install_tasks(), only settings.php should have been written at this point.

file_ensure_htaccess(), however, attempts to verify and write .htaccess files in the public, private, and temporary (and config) file directories. How can these exist or be configured already?

+++ b/core/includes/install.core.inc
@@ -829,12 +844,10 @@ function install_verify_completed_task() {
+function install_verify_database_settings() {

@@ -860,6 +873,23 @@ function install_verify_pdo() {
+function install_ensure_config_directory() {

1) The names are inconsistent. "verify" sounds more appropriate in both cases.

2) I'd prefer if the "verify" (check) and "ensure" (change) operations would be separated.

+++ b/core/includes/install.core.inc
@@ -860,6 +873,23 @@ function install_verify_pdo() {
+  // The config directory must be defined in settings.php.
+  global $config_directory_name;
+  if (empty($config_directory_name)) {
+    return FALSE;
+  }
+  // The logic here is similar to that used by system_requirements() for other
+  // directories that the installer creates.
+  else {
+    $config_directory = config_get_config_directory();
+    return file_prepare_directory($config_directory, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);
+  }

Infinite recursion warning.

config_get_config_directory() also relies on global $config_directory_name.

Hence:

$config_directory == conf_path() . '/files/'

In turn this makes me relatively confident that this patch does not fix the case of when there is a settings.php already, but it does not define a $config_directory_name.

That case not only happens on my local D8 dev site currently (which leads to all config being stored in the public files directory), but is also going to happen in D7->D8 upgrades.

+++ b/core/includes/install.core.inc
@@ -1002,35 +1032,32 @@ function install_settings_form_submit($form, &$form_state) {
+  // Create a default config directory, unless someone already defined one in
+  // settings.php.
   $settings['config_directory_name'] = array(

The comment does not map to the directly following code.

+++ b/core/includes/install.core.inc
@@ -1002,35 +1032,32 @@ function install_settings_form_submit($form, &$form_state) {
+    // This duplicates drupal_get_token() because that function can't work yet.
+    'value'     => $config_directory_name ? $config_directory_name : 'config_' . drupal_hmac_base64('', session_id() . drupal_hash_base64(drupal_random_bytes(55)) . $settings['drupal_hash_salt']['value']),

Can we move the latter part into a proper install_get_token() helper function?

+++ b/core/includes/install.core.inc
@@ -1002,35 +1032,32 @@ function install_settings_form_submit($form, &$form_state) {
+  // Ensure that the new config directory can be created and made writable.
+  if (!install_ensure_config_directory()) {

The verify/validation part of this should happen in install_settings_form_validate() already, so all other submitted form values are not entirely lost in case of an error.

+++ b/core/includes/install.core.inc
@@ -1002,35 +1032,32 @@ function install_settings_form_submit($form, &$form_state) {
+    throw new Exception(st('The directory %directory could not be created or could not be made writable. To proceed with the installation, either create the directory and modify its permissions manually or ensure that the installer has the permissions to create it automatically. For more information, see the <a href="@handbook_url">online handbook</a>.', array('%directory' => config_get_config_directory(), '@handbook_url' => 'http://drupal.org/server-permissions')));

We don't have to throw an exception here. The installer outputs other error messages as simple pages already. (e.g., install_no_profile_error())

Since we're in a form submit handler, you need to set $form_state['no_redirect'] = TRUE, or alternatively $form_state['redirect'] = FALSE to prevent drupal_redirect_form() from invoking drupal_goto(), which in turn renders any returned output as usual.

+++ b/core/modules/system/system.install
@@ -316,6 +316,12 @@ function system_requirements($phase) {
+  // Check the config directory, unless it is not yet defined in settings.php.
+  // (In that case, the installer will create a valid config directory later.)
+  if ($phase != 'install' || !empty($GLOBALS['config_directory_name'])) {
+    $directories[] = config_get_config_directory();
+  }

The comment on this confused me.

It can happen that a Drupal site runs on a settings.php without a global $config_directory_name defined. My current dev site serves as prime example. ;) But you can equally forget or lose the setting when settings.php is edited.

We should add a separate runtime requirement, which ensures that $config_directory_name is non-empty.

+++ b/core/modules/system/system.install
@@ -327,7 +333,7 @@ function system_requirements($phase) {
     if ($phase == 'install') {
-      file_prepare_directory($directory, FILE_CREATE_DIRECTORY);
+      file_prepare_directory($directory, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);
     }

This looks like a fix that could/should be backported.

alexpott’s picture

FileSize
1.4 KB
11.14 KB
PASSED: [[SimpleTest]]: [MySQL] 36,590 pass(es). View

Another issue is that if you have any other customisation in settings.php this will be lost.

So if you settings.php has valid database settings and the drupal_fast_404() line uncommented then this will reverted when the settings.php is saved with the new config_directory_name.

I have also tested the situation where you start an install with a settings.php without database configuration but with config directory configuration. In this instance everything works as expected in that the new settings.php contains the initial value to config_directory_name and it will create it. But again any other customisations will be lost.

Possible solution is to put a warning message in install_settings_form() and tell the user what is going to happen and what is missing from their settings.php

alexpott’s picture

Status: Needs work » Needs review
David_Rothstein’s picture

I can spend some time tomorrow working on this patch, although if someone else wants to jump in beforehand that's OK too. I think we can make many of the changes above (although probably not all of them).

@alexpott, are the two issues you identified definitely issues with this patch, or are they preexisting bugs? Note that entering invalid database configuration information is pretty broken in Drupal 8 right now (due to #1506630: Namedspaced code throws and catches "Exceptions" which don't exist (causing the DB settings form validation to break on install)) and the second one sounds like it's probably always been an issue whenever Drupal needs to overwrite your settings.php, although I'm not sure.

alexpott’s picture

The issue in #18 is definitely introduced by this patch - at the moment if you have a settings file with valid database settings it'd never be rewritten. And due to this patch we can rewrite the file with invalid information but as submit handler will still have valid information so the installer will do the first run of the install batch and then break.

The issue in #21 is similar... we now take people to the database settings form even if their existing settings.php has valid database settings. This is behaviour introduced by this patch.

catch’s picture

Yeah we've essentially added a second condition for people who want to set up settings.php themselves (or re-use the same settings.php for a fresh install etc.). I don't see a way around this, but it could do with it's own change notification I think. Not keen on a warning on the installer since most people re-using/pre-writing settings.php aren't running the web installer anyway, the only time I ever use it like that is if drush is broken for 8.x for a while.

David_Rothstein’s picture

FileSize
4.63 KB
10.25 KB
PASSED: [[SimpleTest]]: [MySQL] 36,599 pass(es). View

Here's a new patch. Some notes:

  1. #18 was caused by #1506630: Namedspaced code throws and catches "Exceptions" which don't exist (causing the DB settings form validation to break on install) and is resolved now that the patch in that issue has been committed. (If you enter invalid database credentials, you should never be able to get to the submit handler in the first place; that was the root cause of the bug.)
  2. #21 is a preexisting bug (reproducible even in Drupal 7). Can we create a new issue for that and move all discussion there? (Note that #445012: Prevent drupal_rewrite_settings() from overwriting customizations made to settings.php seems partially related.) This issue does change the definition of what it means for settings.php to be valid and therefore changes the conditions under which the bug can occur, but it's still the same bug (and as @catch said we can also document what the new procedure is for people who want to create settings.php themselves).

Therefore, I went back to the patch in #16 and made some changes in response to @sun's review. The comments below are only about things I didn't change:

  1. +++ b/core/includes/install.core.inc
    @@ -135,6 +135,9 @@ function install_state_defaults() {
         // This becomes TRUE only when Drupal's system module is installed.
         'database_tables_exist' => FALSE,
    +    // This becomes TRUE only when a valid database connection can be
    +    // established.
    +    'database_verified' => FALSE,
    

    Chronologically and logically, the second comes before the first, so let's move it before that.

    I agree, but currently install_state_defaults() is in alphabetical order. If we want to rearrange it to be in logical order instead, we'd probably want to do that all at once.

  2. @@ -297,9 +301,10 @@ function install_begin_request(&$install_state) {
       // Check existing settings.php.
    -  $install_state['settings_verified'] = install_verify_settings();
    +  $install_state['database_verified'] = install_verify_database_settings();
    +  $install_state['settings_verified'] = install_ensure_config_directory() && $install_state['database_verified'];
    

    After reviewing this patch in detail, I believe it's a good idea to split these flags, but we really should go the extra mile and also add a separate 'config_verified' flag.

    Especially the install_ensure_config_directory() happens way too early and will thus create a config directory in a location where no config directory should be.

    I've gone ahead and added 'config_verified'. I think calling install_ensure_config_directory() is safe here, though (it will only create the directory if it finds one in settings.php).

  3. +function install_base_system(&$install_state) {
    ...
    +  // Ensure that all directories created by the installer have appropriate
    +  // .htaccess files. This is done here because the function relies on
    +  // system.module in order to work, and because at this point in the installer
    +  // all the necessary directories (including the config directory) have been
    +  // created.
    +  file_ensure_htaccess();
    

    I'm no longer able to distill which exact directories are being referenced here. According to install_tasks(), only settings.php should have been written at this point.

    file_ensure_htaccess(), however, attempts to verify and write .htaccess files in the public, private, and temporary (and config) file directories. How can these exist or be configured already?

    I modified the code comment a bit. Drupal creates these directories in system_requirements(), and this happens when install_verify_requirements() runs (which is one of the earlier tasks). The exact ones it creates depend on what you have in settings.php, but the public files directory and the config directory (which are the ones we really care about here) should always have been created by this point, one way or another.

  4. +function install_verify_database_settings() {
    
    @@ -860,6 +873,23 @@ function install_verify_pdo() {
    +function install_ensure_config_directory() {
    

    1) The names are inconsistent. "verify" sounds more appropriate in both cases.

    2) I'd prefer if the "verify" (check) and "ensure" (change) operations would be separated.

    I think the functions are correctly named for what they do (one verifies and one changes). I see what you're saying that there should be an install_verify_config_directory() function also, but in general I think Drupal doesn't split these tasks when it comes to making sure files and directories exist. I'm not opposed to adding it, but I haven't done it for now because I'm not sure there's a good use case for it.

  5. Infinite recursion warning.

    config_get_config_directory() also relies on global $config_directory_name.
    ...
    In turn this makes me relatively confident that this patch does not fix the case of when there is a settings.php already, but it does not define a $config_directory_name.

    Could you clarify the concern here? If there is no $config_directory_name, install_ensure_config_directory() always returns FALSE and never calls config_get_config_directory() at all. This guarantees that the settings.php file won't be considered valid, and therefore you get forced through the settings form submit handler which will rewrite it to be valid. So, I think this is OK as is.

  6. That case not only happens on my local D8 dev site currently (which leads to all config being stored in the public files directory), but is also going to happen in D7->D8 upgrades.

    Is there an issue somewhere for adding this to settings.php during the D7->D8 upgrade?

  7. Can we move the latter part into a proper install_get_token() helper function?

    I was going to do that, but then I realized it would never work as a token (because it's based on a random number and therefore not verifiable later on like real tokens need to be). And actually, that's exactly what we want here anyway... a completely random string, which is based off something random and therefore can never be generated again by anyone else. Since all we care about is getting a random string, I don't think there's any reason we need to throw the session ID or hash salt in when generating it. So, I simplified it to simply use 'config_' . drupal_hash_base64(drupal_random_bytes(55)) instead, and we don't need a helper function for that.

  8. +++ b/core/includes/install.core.inc
    @@ -1002,35 +1032,32 @@ function install_settings_form_submit($form, &$form_state) {
    +  // Ensure that the new config directory can be created and made writable.
    +  if (!install_ensure_config_directory()) {
    

    The verify/validation part of this should happen in install_settings_form_validate() already, so all other submitted form values are not entirely lost in case of an error.

    Well, that validation would only work in the case where settings.php already had a config directory defined, right? In the case where we just created it in the submit handler (i.e. the more common case) we have to verify it here anyway.

    It might be possible to do something more about this, but as the code comment below it says, this is pretty much a fail-safe and it's hard to even imagine a scenario where the check can fail. So I guess I don't think it's worth expending too much effort to verify twice, because I think we'd be optimizing the user experience of a scenario that will never happen :)

  9. We don't have to throw an exception here. The installer outputs other error messages as simple pages already. (e.g., install_no_profile_error())

    Since we're in a form submit handler, you need to set $form_state['no_redirect'] = TRUE, or alternatively $form_state['redirect'] = FALSE to prevent drupal_redirect_form() from invoking drupal_goto(), which in turn renders any returned output as usual.

    This is different in the installer; we need to throw the exception so that it will work correctly during command-line installs. The interactive installer has code to catch those exceptions and render them nicely. That's how the installer handles install_no_profile_error() also; it's always called via throw new Exception(install_no_profile_error()).

David_Rothstein’s picture

Also, @heyrocker, is there a reason that between #14 and #16 you changed this:

   // Indicate that the settings file has been verified, and check the database
   // for the last completed task, now that we have a valid connection. This
   // last step is important since we want to trigger an error if the new
   // database already has Drupal installed.
   $install_state['settings_verified'] = TRUE;
+  $install_state['database_verified'] = TRUE;

to this:

   // Indicate that the settings file has been verified, and check the database
   // for the last completed task, now that we have a valid connection. This
   // last step is important since we want to trigger an error if the new
   // database already has Drupal installed.
-  $install_state['settings_verified'] = TRUE;
+  $install_state['database_verified'] = TRUE;

Or was it by mistake? For now I've changed it back in the above patch, since I think it makes sense for $install_state['settings_verified'] to always reflect the actual current state of settings.php.

David_Rothstein’s picture

Looking over the patch I posted, the new code comment I added to clarify the file_ensure_htaccess() call:

+  // Call file_ensure_htaccess() to ensure that all of Drupal's standard
+  // directories (e.g., the public files directory and config directory) have
+  // appropriate .htaccess files. These directories will have already been
+  // created by this point in the installer, since Drupal creates them during
+  // the install_verify_requirements() task. Note that we cannot call
+  // file_ensure_access() any earlier than this, since it relies on
+  // system.module in order to work.
+  file_ensure_htaccess();

Actually this isn't 100% correct; the config directory itself isn't always created during install_verify_requirements() because it is sometimes created in the settings form submit handler instead (as is part of the whole point of the present issue). I guess that's why my original code comment was more vague on this point :)

But either way, it is already created before this code is reached.

alexpott’s picture

FileSize
633 bytes
10.79 KB
PASSED: [[SimpleTest]]: [MySQL] 36,613 pass(es). View

Unfortunately the issue reported in #18 is not solved by #1506630: Namedspaced code throws and catches "Exceptions" which don't exist (causing the DB settings form validation to break on install) although the behaviour has changed.

Steps to recreate:

  1. Create a settings.php with valid db but no config directory
  2. Navigate to /install.php
  3. Go through forms and forget to enter your database password
  4. On submit you will not get an error... but you will return to the database settings form
  5. Press submit again you will get an error
  6. Complete the database credentials properly and press submit... you will go to the login screen! No site settings screen or anything... and the only modules enabled are user, system and stark!!! Truly minimal :)

Returning to the database settings page even if you have provided a settings.php with valid database settings is a new feature created by this patch. The attached patch adds the code from #19 that prevents this.

I'm also not 100% convinced that #21 is an existing bug as forcing people back to the database settings page is new but I also think that solving it here might not be necessary, for two reasons... creating your own settings.php and then running the installer is an edge case and this does not result in a broken drupal install.

David_Rothstein’s picture

Got it, thanks. Come to think of it, I may have seen this at one point too but then didn't see it again; the problem is that after you experience it once, your settings.php file gets rewritten in such a way that you won't experience it again unless you go back and re-edit it. Sorry about that :)

I suppose that fix makes sense - the issue is that install_database_errors() is getting called twice in the same request, once early in the bootstrap with the correct database credentials and a second time during form validation with the incorrect ones. That code change makes it possible to do that without the first call influencing the second. Still a really strange function (because it modifies the global $databases with whatever credentials you are passing in to test) but that doesn't affect us here.

tstoeckler’s picture

Just a short note: I tried this out and it works as expected. Nice one!

heyrocker’s picture

Status: Needs review » Reviewed & tested by the community

#26:

Is there an issue somewhere for adding this to settings.php during the D7->D8 upgrade?

We do now #1585844: Add config system settings to settings.php on upgrade from D7

#27: That was a mistake, thanks for catching it.

I've given this patch another once over, and it looks great. I'm giving this the go ahead. Thanks a lot David and Alex for your work on this.

catch’s picture

Status: Reviewed & tested by the community » Fixed

This looks good. Committed/pushed to 8.x.

David_Rothstein’s picture

Followups:

  1. I added a change notification at http://drupal.org/node/1591624
  2. I also posted a patch at #445012: Prevent drupal_rewrite_settings() from overwriting customizations made to settings.php for the issue @alexpott described in #21.

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

Anonymous’s picture

Issue summary: View changes

Added requirement for read-only settings.php