variable_get() has previously handled two classes of things that could be set in $conf:
1. Early bootstrap settings that would never end up in the database, which had to be set in settings.php instead
2. Variables which could live in the database, but might be overridden in $conf.
$conf is now specifically for configuration overrides, so we should find a new place to put things in the first group.
I'd suggest either $settings or $drupal_settings.
There are also some one-off globals, like $update_free_access which we could move into keys of this new global - just to reduce the number of global variables overall.
Major because there are already some CMI conversions which have put things into $conf and we'll need to move them out again.
Comment | File | Size | Author |
---|---|---|---|
#50 | 1833516_50.patch | 38.36 KB | chx |
#50 | interdiff.txt | 750 bytes | chx |
#48 | settings-1833516-47.patch | 38.49 KB | Berdir |
#48 | settings-1833516-47-interdiff.txt | 18.3 KB | Berdir |
#47 | 1833516_47.patch | 23.96 KB | chx |
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedThis seems very closely related to #1775842: [meta] Convert all variables to state and/or config systems, which is already a release blocker... Do we really need these as separate issues (both at major/critical priority or higher)?
I'm thinking it would make more sense to close this as a duplicate and just deal with all the "get rid of variables" stuff in one place, but leaving it to you to make that call.
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedEh, well I just realized that's a meta issue (duh) so closing this a duplicate doesn't make sense, but maybe lowering priority and linking it from there does... it still seems like one part of the same big cleanup task.
Comment #3
catchI'd like to keep this major since this is adding another place that variables can go to beyond the existing three (CMI, State and Cache) and it's going to block any conversion I see that tries to put things in $conf that shouldn't be.
Once we've got agreement here on adding this one (and whether things like $databases might go into it), then yeah it's just normal follow-up tasks to actually move things there that can be tracked by the existing critical.
Comment #4
catchHere's a patch, changes $class_loader (which just got moved to it's own variable) to $GLOBALS['settings']['classloader'].
So the idea would be to put two things in here:
- stuff that currently uses $conf but shouldn't.
- stuff that currently has it's own special global
We'd then be able to continue converting some variable_get() calls, and we could also get these lines down to just a couple of globals:
This would be good, because for example $db_url really shouldn't be in Drupal 8 anyway..
Comment #5
catchHmm the poll removal patch slipped into my working install. Here's one without.
Comment #7
catch#5: settings_1833516.patch queued for re-testing.
Comment #8
BerdirI'm a bit surprised that this doesn't cause a undefined array key notice? Or is it eaten because this is very early bootstrap?
Comment #9
BerdirWorked a bit on this.
Converted a bunch of low-level/environment specific variables/config to $settings:
- reverse_proxy*
- proxy*
- omit_vary_cookie (got converted to CMI already)
- session_inc, path_inc, menu_inc
- maximum_replication_lag
There are more that are related, maintanance theme for example. See also the list in #435580: Split up $conf and variables, which I'm going to close as a duplicate now.
And I also converted update_free_access and drupal_hash_salt (this breaks existing installations and will need an update function, the other things could be done as a change notice I guess)
Some of them have own issues currently, but their trivial to replace, creating an issue for each of them feels pointless.
I also added a settings_get() helper function, which makes default handling easier and avoids having to add $GLOBAL all over the place.
About the proxy configuration, I guess we will need to write a guzzle plugin to add support for configuring the proxy there as well. Not a problem of this issue :)
Comment #11
BerdirRemoved the hash salt part to avoid making this issue too complicated.
Comment #12
klausi"$settings.php" should be just "$settings".
Comment #14
BerdirFixed tests and the $settings.php thing.
Comment #16
BerdirHm, that obviously doesn't work, this needs to be persisted for the test to work.
Comment #17
catchThanks for taking this on and sorry for duplicating the three year old issue.
hook_boot() and hook_exit() are on their way out for cached pages (and possibly in general) so I think we could remove that test in this issue. Either that or split it out to a sub-issue so the pattern can be established here.
Comment #18
BerdirNo problem, I forgot about that issue myself too ;)
Yes, I will re-roll today evening and take out that part. I think it makes sense to convert as much as possible of the trivial things here but delay anything that requires non-trivial test changes to separate follow-up issues.
Comment #19
BerdirReverted that and the cache_backends variable, I think that one can just be deleted now anyway, instead, you need to make sure that the namespace of your module is loaded early enough (a drupal_classloader_register() call in settings.php works). But, not here :)
Comment #20
Lars Toomre CreditAttribution: Lars Toomre commentedIf this gets re-rolled, can type hints be included for settings_get() function? Thanks.
Comment #22
Berdir#19: settings-1833516-19.patch queued for re-testing.
Comment #23
aspilicious CreditAttribution: aspilicious commentedyes please...
This helps cmi a lot...
Comment #24
BerdirQuick re-roll with type hints.
Decided against converting more things as it is hard to stop once you start (keyvalue would be a nice example, but then why leave cache out, then you need to update tests, and make sure global settings is restored after a test like conf and so on...)
Comment #26
Berdirlock deadlock...
Comment #27
Berdir#24: settings-1833516-24.patch queued for re-testing.
Comment #28
aspilicious CreditAttribution: aspilicious commentedOk back to rtbc
Comment #29
webchickHm.
Here's a copy of default.settings.php in total with this patch applied, since reviewing the hunks don't really paint the whole picture.
Here's the extraction of the overrides in that file now:
This is frightfully confusing, for a few reasons:
1) $conf and $settings are more or less synonyms with each other. $runtime_config vs. $preboot_settings or something might be more clear.
2) How to explain to a site builder when they should use $conf and when they should use $settings for overrides? There's no documentation to this effect anywhere in the file. We do not want to force site builders to be familiar with the Drupal bootstrap process in order to make overrides.
3) Looking at the list, it all seems rather random. It's kinda like "low-level settings" but I would consider system.fast_404 and allow_authorize_operations to be fairly low-level. How do developers know when they should use settings_get() over config()->get()? The docs for settings_get() say "Settings should be used over configuration for read-only, possibly low bootstrap configuration that is environment specific." I'm not sure that's clear enough. Maybe a few explicit examples?
Comment #30
webchickAnd actually, as a DX thing, is there a reason for settings_get() over settings()->get() to match config()?
Comment #31
BerdirThanks for the feedback.
Yes, the current situation in default.settings.php is a mess. The goal of this issue is to get the new concept/helper function in, including a few example conversions, to see how it works, we can't convert everything yet, it's way too much.
As part of #1775842: [meta] Convert all variables to state and/or config systems, we are already tracking everything that still uses variabe_get() and needs to be moved to either of the three systems that we now have. Once that is done (or as part of actually removing variable_get()), we can update the documentation of the Variable Overrides block in that file to state Configuration Overrides and rename $conf to $config_overrides or similar (Search for "global $conf" to see why that is currently not an option :)). $settings IMHO makes sense, because it relates directly to settings.php.
What about adding a new documentation block for "Settings" to that file, explaining the concept and moving everything that is now settings below that? In the end, (almost) everything in settings.php will be either $settings or configuration overrides. Apart from maximum_replication_lag (which has an issue to add documentation to settings.php) and the *_inc settings (which are really just a hack until that stuff moves to a service, like it happened with password.inc for example), everything that is $settings is already documented in settings.php.
@catch made a joke yesterday about someone suggesting to make it a class :) It's a two line helper function and it won't match config() anyway as there is just get(). I don't think this needs to be made similar to config(), it will mostly be used in core (and possibly custom code for quick environment overrides)
Comment #32
webchickYes, I'm very aware of the never-ending quest to convert all the variable_* stuff to CMI/state/etc. ;)
Hm. So in the end, there will be no scrap of $conf left in settings.php, and all overrides will be $settings overrides? If so, that eliminates my first concern. But if there will still be two options to choose from, I think renaming them (or at least one of them) is important, because it's literally impossible to figure out what the difference is without reading docs of some kind.
New doc block for $settings to explain the concept and group the related settings together sounds good.
And yeah, I don't care about settings()->get() vs. settings_get() that much, was just curious if there was a reason, as I can see it tripping people up. "It's just a simple wrapper function" seems like a reason that makes sense for core devs, not sure about consumers of the code. But it could always be a follow-up.
Comment #33
webchickDuh. I need to actually read. Sorry, feeling under the weather tonight.
Hmmmm. I guess we could defer the $conf rename until after more conversions, but we have to make that at least a major if not critical task, because we definitely can't ship with D8's default settings.php file looking like this. :\
Comment #34
BerdirI think the $conf remove/rename is a part of #1775842: [meta] Convert all variables to state and/or config systems. We can either just rename it as part of the final variable_get() removal patch or we can introduce $config_overrides and corresponding documentation/settings.php updates in a separate issue first and then just remove $conf.
Comment #35
catchEvery single thing that is currently $conf in settings.php should either be moved to $settings or deleted from the file. We can have a hunk of documentation somewhere explaining that you can override config in this way, but that's about it I think.
Most of the stuff in there that shouldn't be moved to $settings is configuration for which there is no user interface, and hence no record of it existing except for the variable_get() call somewhere in the code base. However with CMI files that's enough documentation for people who need to find these hidden variables and using config()->set() is usually going to be the way to update them.
Moving back to CNR, seems like there's the following to do:
- open sub-issues to convert other settings.php-only stuff to $settings
- open another issue (or we could just do it here) to delete the pointless $conf overrides in settings.php that no longer make sense.
- add a docblock for $settings - seems like that should probably be done before commit here but I think the other two can be follow-ups.
Comment #36
webchickYeah, docs is a gate, so I think that needs to be done pre-commit. The rest can be follow-ups, I agree. I wouldn't remove e.g. the site name/default theme $conf overrides though, those are useful examples on how to do common tasks. (I use that a lot on testing/dev environments for example.)
I don't suppose it's possible to just use $settings['foo'] everywhere and have the system be automagically smart enough to know whether the value needs to be derived from settings_get() or config()->get()? :P Just really worried about the site builder experience here. :(
Comment #37
catchWell you could use $conf everywhere then expect developers to know whether to use config() or settings_get() to access it, which has been happening by stealth with some config conversions. But that'd be mixing two completely different and incompatible things together in a single global and has much, much more potential for confusion IMO.
Comment #38
BerdirEspecially because the format is now different.
To override config, you now need something like this:
$conf['mymodule.settings']['key']['subkey'] = 'override';
And for settings, it's just
$conf['setting_name'] = 'override';
So separating it completely makes more sense to me as well.
I'll try to work on a patch tonight or tomorrow for the documentation/settings.php part, maybe that will also help to explain it.
Comment #39
catchOne other thing with this, I'm hopeful we can eventually get this line:
Down to
By moving all those other globals into $settings (and removing the defunct ones like $db_url). There might be one or two exceptions, but most of them could go in there I think.
Comment #40
BerdirStarted to work in this.
Noticed that we partially converted allow_authorize_operations already to CMI but forgot to change settings.php (and a few variable_set()'s in tests that are obviously useless) but while re-organizing default.settings.php, I was wondering if this is actually CMI.
Everywhere where it's used in the code it is references as the "settings.php killswitch", making me wonder if it shouldn't be $settings.
Thoughts?
Comment #41
chx CreditAttribution: chx commentedNaked globals makes my skin crawl. They are very suspectible to change. Constants do not support arrays. What about this?
Comment #42
chx CreditAttribution: chx commentedNote: I know Berdir is working on this but I think my interdiff is small and self contained enough it wont break his work hopefully. If we decide we like it, I am happy to write the necessary doxygen.
Comment #44
tstoecklerHoly crap that is sweet. With that we could even do webchick's
settings()->get
, right?Comment #45
aspilicious CreditAttribution: aspilicious commentedwait...
We can't use a class because it's so low level we define the classloader in it. We can't load a class that needs to load the needed classloader. Correct?
Comment #46
BerdirThe patch works around that by including it directly but I'm not sure about it either..
Right now, it's possible to register the classloader and a path to it in settings.php (required if you want to use a module provided cache backend for low bootstrap caches, replacement for the cache_backends variable, not sure if there is a way around that). This change will break that.
As you can see in the failing tests, we actually have to change these settings at runtime sometimes, for example the tests. So we would have to keep that possibility anyway, IMHO resulting in the same problems as with a global variable...
Also, that singleton construct is not very intuitive :)
Comment #47
chx CreditAttribution: chx commentedNow we can do the coveted settings()->get() and calling drupal_classloader() from settings.php is supported and the test passes. For testing purposes I introduced a settingsSet. This truly should not be supported runtime but tests are... meh :)
Comment #48
BerdirOk, here is an attempt at improving default.settings.php, the order of things now is:
- Intro
- Remaining globals (will move to settings later)
- Settings
- PHP Settings (ini_set() stuff)
- Variable overrides (Will either move to settings or a new Configuration system overrides part once introduced)
Not yet complete but it's a start :)
Also changed the allow_authorize_operations to a setting.
Comment #49
aspilicious CreditAttribution: aspilicious commentedNeeds @file docblock.
Tries...
And the comment is not 100% understandable like it is written now.
@param can be type hinted and we could add a short description.
Should be a newline before @throws and this probably needs a description to.
"Nope." sounds a very usefull message when something fails ;)
I love this patch. Feels like the completion of the new cmi api regarding variable_get conversion
Comment #50
chx CreditAttribution: chx commentedEh, that __set is no longer necessary, I removed it. Added a @file too.
Comment #51
aspilicious CreditAttribution: aspilicious commentedI agree with the rtbc.
Comment #52
BerdirOpened #1881582: Change configuration overrides to use $config instead of $conf, will work on a patch once this is on.
Comment #53
catchFine with the wrapper class compared to raw global access, maybe we can remove that line instead of shortening it then.
I'm not sure about the wrapper function and the singleton, feels like we could do something else there, but it solves the issue of moving everything to one place, and any changes we can do in a find and replace so committed for now to unblock more conversions.
This likely needs two change notices - one for site administrators to tell them how to update settings.php, and one for developers to explain when/why to use this.
Comment #54
aspilicious CreditAttribution: aspilicious commentedChange notice should make a comparison between config() state() and settings(). Not that easy but it would be very very helpfull.
Comment #55
xjmComment #56
David_Rothstein CreditAttribution: David_Rothstein commentedSorry for the noise; I'm told that slashes in tags can break the autocomplete on drupal.org.
Comment #57
David_Rothstein CreditAttribution: David_Rothstein commentedOh.
Comment #58
BerdirCreated a single change record for now: http://drupal.org/node/1882698
The how to use it part for developers is just 1-2 sentences and fits in quite nicely there. Unless we want to provide details there about some hidden things like the ability to set settings for tests?
Comment #59
chx CreditAttribution: chx commentedAssigning to catch because he asked for two change notices and there's only one (which I have corrected a little) but I agree it's OK to have only one, so catch: if you agree please set this to fixed.
Comment #60
catchOh one change notice is fine if it's only a couple of sentences for developers, was only an idea. Moving to fixed.
Comment #62
plachComment #63
nategasser CreditAttribution: nategasser commentedIssue summary updated @ DrupalCon PortlandoopsComment #64
xjmUntagging. Please remove the "Needs change notification" tag when the change notice task is complete.
Comment #65
andyceo CreditAttribution: andyceo commentedFiled a followup: #2199795: Make the Settings class prevent serialization of actual settings