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
Comment | File | Size | Author |
---|---|---|---|
#37 | 2975328-37--8.5.x-reroll.patch | 12.13 KB | smaz |
#27 | 2975328-2-27.patch | 22.74 KB | alexpott |
#27 | 24-27-interdiff.txt | 1.96 KB | alexpott |
#24 | 2975328-2-24.patch | 22.48 KB | alexpott |
#24 | 22-24-interdiff.txt | 2.04 KB | alexpott |
Comments
Comment #2
alexpottThis 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().Comment #3
alexpottHere'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.
Comment #5
alexpottHere'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.
Comment #7
catchGiven 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.
Comment #8
alexpottPart 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.
Comment #9
alexpottI think the patch attached might get us a good compromise.
Settings::get('install_profile')
Comment #11
alexpottComment #13
alexpottI 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 isit 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.Comment #15
alexpottFixing the remaining fails.
Comment #16
borisson_Should we change this message to be more specific?
The profile $settings_profile in settings.php does not match %config_profile.
/s/can/should/?
Goes over 80 cols.
Comment #17
alexpottThanks for the review @borisson_
Patch attached addresses #16
Comment #18
alexpott.... and here is the patch.
Comment #19
borisson_The interdiff is not complete, but the patch looks great now.
Comment #20
alexpottOops - here's the correct interdiff.
Comment #21
catchWhat 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.
Comment #22
alexpottYep 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.
Comment #23
Gábor HojtsyI 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?
Comment #24
alexpottI 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.
Comment #25
Gábor HojtsyImagine 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.
Comment #26
catchI also think the warning is unnecessary. If we never read the value, it does very little harm that it's there.
Comment #27
alexpottI 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.
Comment #28
Gábor HojtsyLooks good to me. No further concerns.
Comment #31
alexpott@catch committed this - also I think @catch deserves credit for opening and reviewing this issue.
Comment #32
alexpottComment #33
andypostWould be great to have CR about it if no warning showed
Comment #34
alexpottThis 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.
Comment #35
Gábor HojtsyAdded this issue to that original CR as well.
Comment #36
catchWe 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.
Comment #37
smazHere'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.
Comment #38
Gábor HojtsyThanks, 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.
Comment #39
xjmThanks for working on this issue.
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.
Hm, this also seems disruptive.
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.Comment #40
alexpottI've filed #2982634: Fix docs in install_write_profile() to address #39.
Comment #41
bobbygryzyngerThanks all for working on this. I am however noticing one problem seemingly related to this change.
After removing my
install_profile
value fromsettings.php
, if I run a site installation from existing configuration using theconfig_installer
profile, theinstall_profile
is re-inserted intosettings.php
.Is this insertion coming from core's handling of the installation? Or, is
config_installer
responsible for this? Ifconfig_installer
is the culprit, I can move this issue over there.Comment #42
alexpott@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.
Comment #43
bobbygryzynger@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.Comment #44
wiifmHey all,
On a site I am involved with, there is code that reads the variable
install_profile
fromsettings.php
and essentially switches on the value and does certain actions. Think a large multisite with a sharedsettings.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).
Comment #45
alexpott@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.
Comment #46
alexpottAnother 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?
Comment #47
wiifmThanks, 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.