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.)
Comment | File | Size | Author |
---|---|---|---|
#29 | 1464944-29.config-install.patch | 10.79 KB | alexpott |
#29 | config-install.interdiff.txt | 633 bytes | alexpott |
#26 | 1464944-26.config-install.patch | 10.25 KB | David_Rothstein |
#26 | interdiff-16-26.txt | 4.63 KB | David_Rothstein |
#21 | 146944-21.patch | 11.14 KB | alexpott |
Comments
Comment #1
gddHere'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.
Comment #2
cosmicdreams CreditAttribution: cosmicdreams commentedIn 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?
Comment #3
gddFirst, 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.
Comment #4
cosmicdreams CreditAttribution: cosmicdreams commentedManually tested this patch.
Setup steps
Results
Comment #5
catchCould 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.
Comment #6
gddIt 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?
Comment #7
gddtagging
Comment #8
ksenzeeI'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.
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedMainly 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?
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?
Comment #10
David_Rothstein CreditAttribution: David_Rothstein commentedActually, 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.)
Comment #11
ksenzeeI 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.
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.
Comment #12
gdd#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.
Comment #13
David_Rothstein CreditAttribution: David_Rothstein commentedWell, 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.
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.
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.
Comment #14
David_Rothstein CreditAttribution: David_Rothstein commentedThe attached patch needs a bit more testing but I think it will work. It uses this basic strategy:
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.
Comment #15
xjmI 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).
Comment #16
gddHere 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.
Comment #17
gddI 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.
Comment #18
alexpottI think there might be an issue with the patch...
Steps to reproduce:
Comment #19
alexpottPatch attached resolves issue described in #18
Comment #20
sunChronologically and logically, the second comes before the first, so let's move it before that.
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'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?
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.
Infinite recursion warning.
config_get_config_directory() also relies on global $config_directory_name.
Hence:
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.
The comment does not map to the directly following code.
Can we move the latter part into a proper install_get_token() helper function?
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.
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.
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.
This looks like a fix that could/should be backported.
Comment #21
alexpottAnother 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
Comment #22
alexpottComment #23
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #24
alexpottThe 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.
Comment #25
catchYeah 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.
Comment #26
David_Rothstein CreditAttribution: David_Rothstein commentedHere's a new patch. Some notes:
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:
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.
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).
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.
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.
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.
Is there an issue somewhere for adding this to settings.php during the D7->D8 upgrade?
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.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 :)
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())
.Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, @heyrocker, is there a reason that between #14 and #16 you changed this:
to this:
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.
Comment #28
David_Rothstein CreditAttribution: David_Rothstein commentedLooking over the patch I posted, the new code comment I added to clarify the file_ensure_htaccess() call:
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.
Comment #29
alexpottUnfortunately 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:
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.
Comment #30
David_Rothstein CreditAttribution: David_Rothstein commentedGot 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.
Comment #31
tstoecklerJust a short note: I tried this out and it works as expected. Nice one!
Comment #32
gdd#26:
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.
Comment #33
catchThis looks good. Committed/pushed to 8.x.
Comment #34
David_Rothstein CreditAttribution: David_Rothstein commentedFollowups:
Comment #35.0
(not verified) CreditAttribution: commentedAdded requirement for read-only settings.php