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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

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

David_Rothstein’s picture

Eh, 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.

catch’s picture

I'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.

catch’s picture

Status: Active » Needs review
FileSize
56.96 KB

Here'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:

function drupal_settings_initialize() {
  global $base_url, $base_path, $base_root, $script_path;

  // Export these settings.php variables to the global namespace.
  global $databases, $cookie_domain, $conf, $settings, $installed_profile, $update_free_access, $db_url, $db_prefix, $drupal_hash_salt, $is_https, $base_secure_url, $base_insecure_url, $config_directories;
  $conf = array();

This would be good, because for example $db_url really shouldn't be in Drupal 8 anyway..

catch’s picture

FileSize
1.83 KB

Hmm the poll removal patch slipped into my working install. Here's one without.

Status: Needs review » Needs work

The last submitted patch, settings_1833516.patch, failed testing.

catch’s picture

Status: Needs work » Needs review

#5: settings_1833516.patch queued for re-testing.

Berdir’s picture

+++ b/core/includes/bootstrap.incundefined
@@ -3012,12 +3012,11 @@ function drupal_classloader() {
-    switch ($class_loader) {
+    switch ($GLOBALS['settings']['class_loader']) {

I'm a bit surprised that this doesn't cause a undefined array key notice? Or is it eaten because this is very early bootstrap?

Berdir’s picture

FileSize
25.68 KB

Worked 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 :)

Status: Needs review » Needs work

The last submitted patch, settings-1833516-9.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
20.27 KB

Removed the hash salt part to avoid making this issue too complicated.

klausi’s picture

+++ b/core/includes/bootstrap.inc
@@ -945,6 +945,27 @@ function drupal_get_filename($type, $name, $filename = NULL) {
+ * Settings can be set in settings.php in the $settings.php array and requested
+ * by this function. Settings should be used over configuration for read-only,

"$settings.php" should be just "$settings".

Status: Needs review » Needs work

The last submitted patch, settings-1833516-11.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
21.45 KB

Fixed tests and the $settings.php thing.

Status: Needs review » Needs work

The last submitted patch, settings-1833516-14.patch, failed testing.

Berdir’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Bootstrap/HookBootExitTest.phpundefined
@@ -52,7 +52,7 @@ function testHookBootExit() {
     // Boot and exit should not fire since the page is cached.
-    variable_set('page_cache_invoke_hooks', FALSE);
+    $GLOBALS['settings']['page_cache_invoke_hooks'] = FALSE;
     $this->assertTrue(cache('page')->get(url('', array('absolute' => TRUE))), 'Page has been cached.');

Hm, that obviously doesn't work, this needs to be persisted for the test to work.

catch’s picture

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

Berdir’s picture

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
19.25 KB

Reverted 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 :)

Lars Toomre’s picture

If this gets re-rolled, can type hints be included for settings_get() function? Thanks.

Status: Needs review » Needs work

The last submitted patch, settings-1833516-19.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#19: settings-1833516-19.patch queued for re-testing.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

yes please...

This helps cmi a lot...

Berdir’s picture

Quick 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...)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, settings-1833516-24.patch, failed testing.

Berdir’s picture

lock deadlock...

Berdir’s picture

Status: Needs work » Needs review

#24: settings-1833516-24.patch queued for re-testing.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Ok back to rtbc

webchick’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
22.71 KB

Hm.

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:

# $conf['system.site']['name'] = 'My Drupal site';
# $conf['theme_default'] = 'stark';
# $conf['anonymous'] = 'Visitor';
# $conf['maintenance_theme'] = 'bartik';
# $settings['reverse_proxy'] = TRUE;
# $settings['reverse_proxy_addresses'] = array('a.b.c.d', ...);
# $settings['reverse_proxy_header'] = 'HTTP_X_CLUSTER_CLIENT_IP';
# $settings['omit_vary_cookie'] = TRUE;
# $conf['system.performance']['css']['gzip'] = FALSE;
# $conf['system.performance']['js']['gzip'] = FALSE;
# $conf['locale_custom_strings_en'][''] = array(
#   'forum'      => 'Discussion board',
#   '@count min' => '@count minutes',
# );
#$conf['system.fast_404']['exclude_paths'] = '/\/(?:styles)\//';
#$conf['system.fast_404']['paths'] = '/\.(?:txt|png|gif|jpe?g|css|js|ico|swf|flv|cgi|bat|pl|dll|exe|asp)$/i';
#$conf['system.fast_404']['html'] = '<!DOCTYPE html><html><head><title>404 Not Found</title></head><body><h1>Not Found</h1><p>The requested URL "@path" was not found on this server.</p></body></html>';
# $settings['proxy_server'] = '';
# $settings['proxy_port'] = 8080;
# $settings['proxy_username'] = '';
# $settings['proxy_password'] = '';
# $settings['proxy_user_agent'] = '';
# $settings['proxy_exceptions'] = array('127.0.0.1', 'localhost');
# $conf['allow_authorize_operations'] = FALSE;

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?

webchick’s picture

And actually, as a DX thing, is there a reason for settings_get() over settings()->get() to match config()?

Berdir’s picture

Thanks 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)

webchick’s picture

Yes, 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.

webchick’s picture

Duh. 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. :\

Berdir’s picture

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

catch’s picture

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

webchick’s picture

Yeah, 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. :(

catch’s picture

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()? :

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

Berdir’s picture

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

catch’s picture

One other thing with this, I'm hopeful we can eventually get this line:

-  global $databases, $cookie_domain, $conf, $installed_profile, $update_free_access, $class_loader, $db_url, $db_prefix, $drupal_hash_salt, $is_https, $base_secure_url, $base_insecure_url, 

Down to

global $settings, $config_overrides;

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.

Berdir’s picture

Started 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?

chx’s picture

Status: Needs work » Needs review
FileSize
2.16 KB
20.57 KB

Naked globals makes my skin crawl. They are very suspectible to change. Constants do not support arrays. What about this?

chx’s picture

Note: 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.

Status: Needs review » Needs work

The last submitted patch, 1833516_41.patch, failed testing.

tstoeckler’s picture

Holy crap that is sweet. With that we could even do webchick's settings()->get, right?

aspilicious’s picture

wait...

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?

Berdir’s picture

The 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 :)

chx’s picture

Status: Needs work » Needs review
FileSize
14.85 KB
23.96 KB

Now 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 :)

Berdir’s picture

Ok, 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.

aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Component/Utility/Settings.phpundefined
@@ -0,0 +1,64 @@
+<?php
+

Needs @file docblock.

+++ b/core/lib/Drupal/Component/Utility/Settings.phpundefined
@@ -0,0 +1,64 @@
+  /**
+   * Trying to set a property is not allowed.
+   *
+   * @param $name
+   * @param $value
+   * @throws \Exception

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.

+++ b/core/lib/Drupal/Component/Utility/Settings.phpundefined
@@ -0,0 +1,64 @@
+    throw new \Exception('Nope.');

"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

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
750 bytes
38.36 KB

Eh, that __set is no longer necessary, I removed it. Added a @file too.

aspilicious’s picture

I agree with the rtbc.

Berdir’s picture

catch’s picture

Title: Add a new top-level global for settings.php - move things out of $conf » Change notice: Add a new top-level global for settings.php - move things out of $conf
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

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

aspilicious’s picture

Change notice should make a comparison between config() state() and settings(). Not that easy but it would be very very helpfull.

xjm’s picture

Issue tags: +Needs change record
David_Rothstein’s picture

Issue tags: -Needs change record +major and critical issue threshold sweep

Sorry for the noise; I'm told that slashes in tags can break the autocomplete on drupal.org.

David_Rothstein’s picture

Issue tags: +Needs change record

Oh.

Berdir’s picture

Status: Active » Needs review

Created 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?

chx’s picture

Assigned: Unassigned » catch
Status: Needs review » Reviewed & tested by the community

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

catch’s picture

Assigned: catch » Unassigned
Priority: Critical » Normal
Status: Reviewed & tested by the community » Fixed

Oh one change notice is fine if it's only a couple of sentences for developers, was only an idea. Moving to fixed.

Status: Fixed » Closed (fixed)

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

plach’s picture

Title: Change notice: Add a new top-level global for settings.php - move things out of $conf » Add a new top-level global for settings.php - move things out of $conf
nategasser’s picture

Issue summary updated @ DrupalCon Portland oops

xjm’s picture

Issue tags: -Needs change record

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.

andyceo’s picture