Problem/Motivation

If you have an existing settings.php with database settings and a hash salt it is possible to install drupal without the install_profile setting being written to settings.php

Discovered in #2090115: Don't install a module when its default configuration has unmet dependencies

Proposed resolution

  • If the settings.php exists and has install_profile - it should be used and the form to select the profile should be skipped
  • If settings.php exists and is writable and does not have the install_profile - it should be selected using the form and written to settings.php

Remaining tasks

User interface changes

API changes

Files: 
CommentFileSizeAuthor
#19 11-16-interdiff.txt2.24 KBalexpott
#19 2451363.19.patch5.72 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,931 pass(es). View
#16 2451363.16.patch5.68 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,920 pass(es). View
#16 11-16-interdiff.txt2.24 KBalexpott
#11 2451363.11.patch5.46 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,738 pass(es). View
#11 10-11-interdiff.txt1.96 KBalexpott
#10 2451363.10.patch5.96 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,498 pass(es). View
#10 5-10-interdiff.txt696 bytesalexpott
#5 2451363.4.patch5.46 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,473 pass(es). View
#5 2451363.4.test-only.patch3.71 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,463 pass(es), 18 fail(s), and 1 exception(s). View

Comments

YesCT’s picture

alexpott’s picture

Title: Ensure the profile is written in settings.php or can be » Ensure install_profile is exists in settings.php after installation
Parent issue: #2090115: Don't install a module when its default configuration has unmet dependencies »
Related issues: -#2451369: It is possible to install a site without install_profile in settings.php +#2090115: Don't install a module when its default configuration has unmet dependencies

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

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes

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.

alexpott’s picture

Status: Active » Needs review
FileSize
3.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,463 pass(es), 18 fail(s), and 1 exception(s). View
5.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,473 pass(es). View

The last submitted patch, 5: 2451363.4.test-only.patch, failed testing.

pjcdawkins’s picture

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?

alexpott’s picture

@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

Berdir’s picture

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.

alexpott’s picture

FileSize
696 bytes
5.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,498 pass(es). View

@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.

alexpott’s picture

FileSize
1.96 KB
5.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,738 pass(es). View

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.

Status: Needs review » Needs work

The last submitted patch, 11: 2451363.11.patch, failed testing.

Status: Needs work » Needs review

Berdir queued 11: 2451363.11.patch for re-testing.

Berdir’s picture

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.

Berdir’s picture

Status: Needs review » Needs work
  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?

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.24 KB
5.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,920 pass(es). View

Addressed everything in @Berdir's review. Thanks!

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looks great now.

pjcdawkins’s picture

Status: Reviewed & tested by the community » Needs work

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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
5.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,931 pass(es). View
2.24 KB

@pjcdawkins nice catch :)

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

pjcdawkins’s picture

Status: Needs review » Reviewed & tested by the community

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

  • webchick committed a57bc04 on
    Issue #2451363 by alexpott, Berdir, pjcdawkins: Ensure install_profile...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Oops, my comment got eaten.

Committed and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

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