Ensure install_profile is exists in settings.php after installation

[Edit: was meant to be issue summary :)]

Settings.php has to be writable during the installer (see install_check_requirements()) so removing the requirement to deal with non-writable settings.php from issue summary.

This works with the debug script I made.

I can't really review it from a code perspective, though I suspect the mkdir() calls are now unnecessary.

I think the install_profile setting should be documented in default.settings.php. Particularly since it is now required when settings.php is not writable (it wasn't required before). Can that be part of this issue?

@pjcdawkins it was required if you were going to using a profile that provided modules. But yes I agree we should document it in default.settings.php

Hm.

So far, I've only noticed this problem when installing with drush, which *does* allow to install when your settings.php is not writable, as long as all the required information is already there.

The fix makes sense for the UI think, although it might be a bit confusing, not sure. Also not exactly sure how that will work in combination with a distribution, which skips that step too?

For drush, it's a bit confusing/unexpected too. Because you are required to provide the install profile upfront, but it will then actually not be used. To test, drush si -y standard, and then drush si -y minimal will install again in standard. Without ever telling you so. Maybe that's something that drush needs to take care of, not sure.

@Berdir all this patch does is say that if install_profile is set in settings.php then that is what is used which is consistent with what we for databases and config_directories. HEAD is currently very broken because if you had an exclusive distribution with a non writable settings.php which did not have the install_profile set and install using drush then modules from the profile are used during installation but once Drupal is installed they won't be.

Discussed this with @Berdir in IRC we decided to not force the profile according to what is in settings.php but to instead ensure that we rewrite if necessary. This means that drush si will crash if working with a write protected settings.php and a mismatched install profile.

Manually tested with drush si and a distribution install profile. Works well, I think this is the better approach. Will do another review and RTBC later.

1. +++ b/core/includes/install.core.inc
   @@ -2263,3 +2265,20 @@ function install_display_requirements($install_state, $requirements) {
   + * @param $install_state
   + *   An array of information about the current installation state.
   

   "array" missing.

2. +++ b/core/modules/system/src/Tests/Installer/InstallerExistingSettingsTest.php
   @@ -33,10 +33,8 @@ protected function setUp() {
        // Actually the install profile should be skipped to because it is written
        // to settings.php.
   -    // @todo https://www.drupal.org/node/2451369 Fix install_profile so that it
   -    //   is written to an existing settings.php if possible or if set used.
        $this->settings['settings']['install_profile'] = (object) array(
   -      'value' => 'testing',
   

   Comment is a bit strange.. why Actually, should "skipped to" be "skipped too", but too doesn't make sense to me either...

3. +++ b/sites/default/default.settings.php
   @@ -246,6 +246,16 @@
   + * If this is set prior to installation this value will used regardless of any
   + * parameters passed to install_drupal(). Changing this after installation is
   + * not recommended as it changes which directories are scanned during extension
   + * discovery.
   + */
   +# $install_profile = '';
   

   Not correct anymore I think?

Addressed everything in @Berdir's review. Thanks!

Looks great now.

The patch works but the documentation is slightly wrong.

+++ b/sites/default/default.settings.php
@@ -246,6 +246,18 @@
+# $install_profile = '';

This should be $settings['install_profile'], and it should be under the "Settings" section just below.

@pjcdawkins nice catch :)

The interdiff is more confusing than just reading the patch :)

Thanks. I tested with the same script I was using before, under a bunch of different cases, including writable and non-writable settings.php.

Oops, my comment got eaten.

Committed and pushed to 8.0.x. Thanks!