Problem/Motivation

#2156401: Write install_profile value to configuration and only to settings.php if it is writeable changed from always writing the install profile to configuration, and only to settings.php if settings.php is writable.

Change record is here: https://www.drupal.org/node/2538996

However this leaves a relatively annoying issue when re-installing Drupal.

Steps to reproduce:

1. Install minimal profile
2. Drop the database, try to reinstall with Standard (doesn't matter if this is via drush or the UI)
3. You'll see an error message about the install profile in settings.php not matching the chosen profile.

Proposed resolution

Completely stop writing the install profile to settings.php regardless of whether it's writable or not.

Remaining tasks

User interface changes

API changes

We'e already deprecated getting the install profile from settings.php and accepted it won't always be available there for new sites where settings.php is read-only. Given this change will also only affect new sites, it should not in practice break bc that anyone is relying on at this point.

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

alexpott’s picture

This is not really a new problem caused by #2156401: Write install_profile value to configuration and only to settings.php if it is writeable - it was always the case with a non-writable settings.php - you couldn't change the profile. The problem with removing this is that we continue to write it as a BC layer. If we remove the BC layer code in contrib and custom that does Settings::get('install_profile') will break. I think maybe we could just add a BC layer to Settings to get around this - one that calls \Drupal::installProfile().

alexpott’s picture

Status: Active » Needs review
FileSize
10.13 KB

Here's a possible approach. One thing about Drupal 8 is that prior to #2156401: Write install_profile value to configuration and only to settings.php if it is writeable you could install with a different profile but once installation was done then it would magically swap back to the profile in settings.php. This could have site breaking consequences if the install profile you installed contained modules that were not available to the profile in defined in settings.php.

Status: Needs review » Needs work

The last submitted patch, 3: 2975328-3.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.71 KB

Here's another approach that I think is safer. We can just warn the user if there is a mismatch but still allow them to install.

Status: Needs review » Needs work

The last submitted patch, 5: 2975328-2-5.patch, failed testing. View results

catch’s picture

Given we're going to be telling people to reinstall their sites with a different install profile, I don't think we should be showing warnings as part of the process we've instructed them to take.

alexpott’s picture

Part of me feels uncomfortable with what we're telling people. The idea of maintaining a settings.php through a site re-install means that the hash salt is also being copied which doesn't feel like good practice. If I was telling people to reinstall I would be saying something like create a fresh settings.php as a copy from default.settings.php - not only for a fresh hash salt but also in-case there are any changes to default.settings.php.

alexpott’s picture

Status: Needs work » Needs review
FileSize
15.42 KB
16.51 KB

I think the patch attached might get us a good compromise.

  • We only write the install profile to settings.php if we can and there is a mismatch during the installer, however we do not warn during installation if we can't write.
  • If there is a mismatch during runtime we warn on the status report and tell people to remove from settings.php
  • We trigger deprecation errors for Settings::get('install_profile')

Status: Needs review » Needs work

The last submitted patch, 9: 2975328-2-9.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.28 KB
16.48 KB

Status: Needs review » Needs work

The last submitted patch, 11: 2975328-2-11.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
699 bytes
16.83 KB

I played around a bit with reading the install profile from the container in Settings::initialize() so that Settings::get('install_profile') would always work but then I got concerned about chickens and eggs because of how settings is available before container building and about the upgrade path before we've put the install profile in configuration so I stopped investigating that.

I think given that drupal_get_profile() was the old API to access this and the new API is

 *   Use the install_profile container parameter or \Drupal::installProfile()
 *   instead. If you are accessing the value before it is written to
 *   configuration during the installer use the $install_state global. If you
 *   need to access the value before container is available you can use
 *   BootstrapConfigStorageFactory to load the value directly from
 *   configuration.

it is okay to break Settings::get('install_profile') for new installs. By break I mean this will not return the install profile - it will return a NULL.

Status: Needs review » Needs work

The last submitted patch, 13: 2975328-2-13.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.21 KB
21.04 KB

Fixing the remaining fails.

borisson_’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/system.install
    @@ -1001,6 +1001,24 @@ function system_requirements($phase) {
    +        'value' => t("The profile %settings_profile does not match %config_profile.", [
    +          '%settings_profile' => $settings['install_profile'],
    +          '%config_profile' => $config_profile,
    +        ]),
    

    Should we change this message to be more specific? The profile $settings_profile in settings.php does not match %config_profile.

  2. +++ b/core/modules/system/system.install
    @@ -1001,6 +1001,24 @@ function system_requirements($phase) {
    +        'description' => t('Drupal 8 no longer uses the settings.php value and it can be removed.'),
    

    /s/can/should/?

  3. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerExistingSettingsMismatchProfileTest.php
    @@ -8,9 +8,10 @@
    - * Tests the installer with an existing settings file but no install profile.
    + * Tests the installer with an existing settings file and a mismatching install profile.
    

    Goes over 80 cols.

alexpott’s picture

Status: Needs work » Needs review

Thanks for the review @borisson_

Patch attached addresses #16

alexpott’s picture

.... and here is the patch.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

The interdiff is not complete, but the patch looks great now.

alexpott’s picture

FileSize
2.64 KB

Oops - here's the correct interdiff.

catch’s picture

+++ b/core/includes/install.core.inc
@@ -2284,16 +2283,15 @@ function install_display_requirements($install_state, $requirements) {
  */
 function install_write_profile($install_state) {
-  // Only need to write to settings.php if it is possible. The primary storage
-  // for the install profile is the core.extension configuration.
+  // Only need to write to settings.php if it is possible and already
+  // configured. The primary storage for the install profile is the
+  // core.extension configuration.

What does this mean? That we write to settings.php if we're doing a reinstall, and we've already got the install profile in settings.php?

If that's right, can we use a word other than 'configured', like 'already written to settings' - at the moment it's not clear if it means core.extension configuration, settings.php configuration, a form submission.

alexpott’s picture

Yep that sentence is not the clearest. Also in discussion with @catch on slack we agreed that the comments in default.settings.php are not necessary especially for new sites which is the main target for default.settings.php.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

I have mixed feelings about adding a runtime requirements check that is displayed as a warning but basically says "two values are not the same, although we don't care about one of them, so whatever" ;) Do we need that warning? Looks like we'd add warnings to existing sites that are not doing anything wrong per say and that will not have issues due to this mismatch either?

alexpott’s picture

I think we should be telling people to remove the value from settings.php. It is very confusing when a site has a setting that is completely ignored by the system. And it gets worse when the value a developer is seeing in settings.php is not being respected. The fix is very simple - remove it and it is just a warning.

@Gábor Hojtsy's question made me realise that we actually were still writing the install profile to settings.php on every fresh install with a writable settings.php file. So I've fixed that and changed the warning to always be there if settings.php has an install profile set. Yes this means all sites will get a warning BUT then their settings.php will be one step closer to D9 compatibility. If we don't want to stick a warning on everyone's existing site now we could downgrade this to an informational and then up it to a warning closer to the D9 time. But, at least for me, avoiding the potential confusion of why is this value being ignored makes this worth it.

Gábor Hojtsy’s picture

Imagine we'd not be able to remove variable table values in D7 for some reason. We would discontinue using some of them, and then start warning people like "The file_public_url setting is not used anymore, so please remove it". I think its confusing because if we don't use it, then what harm will it do? If all harm it does is it may get confusing to see the mismatch by a human later, it would be best to describe it. Otherwise the urgency to remove it seems strange if we don't use it. We should answer the question IMHO on why would the site user need to do all this work. This is especially true for hosted environments where the user may not have access to the setings.php to begin with and has no way to fix this. We'll keep telling them this is wrong, but there is nothing technically wrong until they could actually look at the file and get confused.

catch’s picture

I also think the warning is unnecessary. If we never read the value, it does very little harm that it's there.

alexpott’s picture

If we never read the value, it does very little harm that it's there.

I don't agree with "no harm". A setting that does not match the developers experience or previous expectations with what that setting would do is harmful.

However, the argument that not everyone can fix this based on their hosting provider is a good one. And so I think the best option is to go for an informational "requirement" that tells people they can remove the setting. So no warning that people can do nothing about but at least people can find out that something can be removed.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. No further concerns.

The last submitted patch, 24: 2975328-2-24.patch, failed testing. View results

  • catch committed 1672637 on 8.6.x
    Issue #2975328 by alexpott, Gábor Hojtsy, borisson_: Install profile in...
alexpott’s picture

@catch committed this - also I think @catch deserves credit for opening and reviewing this issue.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
andypost’s picture

Would be great to have CR about it if no warning showed

alexpott’s picture

This CR covers this I think - https://www.drupal.org/node/2538996 - this is really just an extension of that work to move install profile into configuration.

Gábor Hojtsy’s picture

Added this issue to that original CR as well.

catch’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Fixed » Needs review

We can probably backport this to 8.5.x, but it would need to remove the status message and the deprecations (at least). I had typed this up to post along with the commit, but the comment never made it.

smaz’s picture

Here's a stab at an 8.5.x version based on the patch from #27. I've left out any deprecations / warnings + the status report message as per #36.

It works locally for me (installing with minimal, dropping database, re-installing with standard).

I'm not sure on the tests though, as I need to do some work on my local environment to be able to run them.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, this seems to contain all changes that are not related to depreciation and keeps the boostrap.inc BC layer which we need for 8.5 as far as I understand. Looks good.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Thanks for working on this issue.

  1. +++ b/core/includes/install.core.inc
    @@ -2214,16 +2213,16 @@ function install_display_requirements($install_state, $requirements) {
    -  // Only need to write to settings.php if it is possible. The primary storage
    -  // for the install profile is the core.extension configuration.
    +  // Only write the install profile to settings.php if already exists. The value
    +  // from settings.php is never used but drupal_rewrite_settings() does not
    +  // support removing a setting. If the value in configuration and settings.php
    +  // don't match there will be a warning on the status report.
    

    I realize this was already committed, but this comment is not clear and has a grammatical error ("if already exists"). As someone reading this code, I would want to know why the value from settings.php was never used (because it's deprecated I guess) and when the unspecified missing subject of the clause would already exist.

  2. +++ b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
    @@ -225,11 +225,6 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    -    // Remember the profile which was used.
    -    $settings['settings']['install_profile'] = (object) [
    -      'value' => $install_state['parameters']['profile'],
    -      'required' => TRUE,
    -    ];
    

    Hm, this also seems disruptive.

  3. +++ b/sites/default/default.settings.php
    @@ -263,23 +263,6 @@
    -/**
    - * The active installation profile.
    - *
    - * Changing this after installation is not recommended as it changes which
    - * directories are scanned during extension discovery. If this is set prior to
    - * installation this value will be rewritten according to the profile selected
    - * by the user.
    - *
    - * @see install_select_profile()
    - *
    - * @deprecated in Drupal 8.3.0 and will be removed before Drupal 9.0.0. The
    - *   install profile is written to the core.extension configuration. If a
    - *   service requires the install profile use the 'install_profile' container
    - *   parameter. Functional code can use \Drupal::installProfile().
    - */
    -# $settings['install_profile'] = '';
    -
    

    I'm not super keen on backporting this change. Even though this should have no impact on existing sites, it's still a bit disruptive because it causes the default and the installed settings.php to diverge, and it might make site owners hesitate about the update. I'd prefer for this to be minor only.

alexpott’s picture

bobbygryzynger’s picture

Thanks all for working on this. I am however noticing one problem seemingly related to this change.

After removing my install_profile value from settings.php, if I run a site installation from existing configuration using the config_installer profile, the install_profile is re-inserted into settings.php.

Is this insertion coming from core's handling of the installation? Or, is config_installer responsible for this? If config_installer is the culprit, I can move this issue over there.

alexpott’s picture

@bobbygryzynger that is the config_installer profile's doing. Please don't move this issue - file a new one against that project. It could be a generic issue to make that ready for 8.6.x as there are a few other changes that need to take place. Depending on what gets into 8.6.x.

bobbygryzynger’s picture

@alexpott, thanks for clarifying and yeah, that's what I meant (not moving the issue, but creating a new one). I've opened up the issue over in config_installer: #2983111: install_profile reinserted into settings.php.

wiifm’s picture

Hey all,

On a site I am involved with, there is code that reads the variable install_profile from settings.php and essentially switches on the value and does certain actions. Think a large multisite with a shared settings.php file.

Now, after the upgrade to 8.5.5 I find that this value is no longer written to new sites (but still exists for old sites).

This to me seems like a break in backwards compatibility https://www.drupal.org/core/d8-bc-policy - code that used to work in Drupal 8.4.x no longer works in 8.5.x.

Does anyone have any suggestions on how to restore this behaviour? Or anyway to determine the installation profile when at reading settings.php (the database is not available at this stage).

alexpott’s picture

@wiifm how do you have a shared settings.php and different values for the install profile? I guess you're expecting the install process to write the value to settings.php.

Also I'm a little confused by the report of this being an issue with 8.5.5 as this patch was only committed to 8.6.x and in 8.5.x we do write the install profile to settings.php if it is writable - see install_write_profile(). But I guess you might have a problem with 8.6.0 when that is out.

alexpott’s picture

Status: Needs review » Fixed

Another thought this won't be put in 8.5.x so we should close this issue. @wiifm can you create a follow-up issue to discuss the problems you are facing?

wiifm’s picture

Thanks, seeing as this behaviour is going away (seemingly on purpose) (also unsure why this is not working now), we will be writing the install_profile value out to a file post install.

Status: Fixed » Closed (fixed)

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