Problem

Currently, you can set variable overrides by using the $conf array, which is setup in settings.php. This has not yet been implemented in the configuration system.

Goal

Implement this functionality.

Proposed resolution

When a DrupalConfig object is constructed, it reads the current configuration from the active store and loads it into a $data array in the object. At this time, $conf should be checked to see if it contains any overrides of this data. If so, the overrides should be merged into the $data array. Once this is done, any subsequent calls to $config->get() for those keys will return the overridden data.

Notes

  • I had originally envisioned a system in which the data from $conf gets merged into the active store at reload, and if you changed $conf you would need to do another reload. This has the advantage of the active store always being canonical. However this won't work, as a lot of systems like Domain Access dynamically set these values at runtime. So we need a runtime solution as well.
  • We will have to change the format of $conf a little to accomodate our new dual-key system (filename and key). I figure it will be like this:
    $conf['system.performance']['cache'] = 0;
    
Files: 
CommentFileSizeAuthor
#26 1484690-26.patch3.43 KBjhedstrom
PASSED: [[SimpleTest]]: [MySQL] 35,702 pass(es). View
#17 1484690-17.patch3.39 KBjhedstrom
PASSED: [[SimpleTest]]: [MySQL] 35,690 pass(es). View
#12 1484690-12-conf.patch3.37 KBPol
PASSED: [[SimpleTest]]: [MySQL] 35,690 pass(es). View
#9 1484690-9-conf.patch3.4 KBPol
PASSED: [[SimpleTest]]: [MySQL] 35,685 pass(es). View
#8 1484690-8-conf.patch3.52 KBPol
PASSED: [[SimpleTest]]: [MySQL] 35,681 pass(es). View
#1 1484690-1-conf.patch2.22 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 35,648 pass(es). View

Comments

beejeebus’s picture

Status: Active » Needs review
FileSize
2.22 KB
PASSED: [[SimpleTest]]: [MySQL] 35,648 pass(es). View

ok, first cut patch attached.

Pol’s picture

Status: Needs review » Reviewed & tested by the community

Hello,

I've just tested your patch and it works.

I've tested with :

$conf['system.performance']['cache'] = 1;
$conf['site_name'] = "This site_name should be set by settings.php.";
beejeebus’s picture

Status: Reviewed & tested by the community » Needs review

thanks for testing the patch. we probably need some more review before we can set to RTBC.

jhedstrom’s picture

I tested #1 and it properly picked up $conf['site_name']. Do any of the existing $conf settings in default.settings.php need to be updated as part of this patch?

# $conf['site_name'] = 'My Drupal site';
# $conf['theme_default'] = 'stark';
# $conf['anonymous'] = 'Visitor';
# $conf['maintenance_theme'] = 'bartik';
# $conf['reverse_proxy'] = TRUE;
# $conf['reverse_proxy_addresses'] = array('a.b.c.d', ...);
# $conf['reverse_proxy_header'] = 'HTTP_X_CLUSTER_CLIENT_IP';
# $conf['omit_vary_cookie'] = TRUE;
# $conf['css_gzip_compression'] = FALSE;
# $conf['js_gzip_compression'] = FALSE;
# $conf['locale_custom_strings_en'][''] = array(
# $conf['blocked_ips'] = array(
$conf['404_fast_paths_exclude'] = '/\/(?:styles)\//';
$conf['404_fast_paths'] = '/\.(?:txt|png|gif|jpe?g|css|js|ico|swf|flv|cgi|bat|pl|dll|exe|asp)$/i';
$conf['404_fast_html'] = '<html xmlns="http://www.w3.org/1999/xhtml"><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>';
# $conf['allow_authorize_operations'] = FALSE;
beejeebus’s picture

re #4 - we'll need to change anything that has been ported to the config system already, which is not much.

each follow up patch that ports some part of drupal to the config system and has a default.settings.php snippet will need to update it.

Pol’s picture

I think I can take care of this, but I need more explanations.

We have to replace all variable_get() with :

$config = config('prefix.name');
echo $config->get('my.value');

How to do for $conf['site_name'] ?

Should we replace it with system.site_name and do:

$config = config('system');
echo $config->get('site_name');

And replace $conf['site_name'] with $conf['system']['site_name'] ?

Thanks.

beejeebus’s picture

we only need to update $conf settings in default.settings.php that have been converted to the new config system.

as more parts are ported, the patches will need to include snippets for default.settings.php.

the main thing this patch needs now is a test.

Pol’s picture

FileSize
3.52 KB
PASSED: [[SimpleTest]]: [MySQL] 35,681 pass(es). View

Patch updated with tests.

Pol’s picture

FileSize
3.4 KB
PASSED: [[SimpleTest]]: [MySQL] 35,685 pass(es). View

Updated patch, combined the two tests into one.

jhedstrom’s picture

+++ b/core/modules/config/config.testundefined
@@ -268,3 +268,32 @@ class ConfigFileContentTestCase extends DrupalWebTestCase {
+  protected $profile = 'testing';

Is this needed anymore? The default profile is now the testing profile.

beejeebus’s picture

Status: Needs review » Needs work

the patch at #9 looks ready to go to me, once we remove the $profile line as per #10.

nice work Pol!

Pol’s picture

FileSize
3.37 KB
PASSED: [[SimpleTest]]: [MySQL] 35,690 pass(es). View

Here we are.

beejeebus’s picture

Status: Needs work » Needs review

ok, 12 looks good, will RTBC it if the bot comes back green.

agentrickard’s picture

Is anyone else troubled by statements like this one?

$merged_data = drupal_array_merge_deep($this->data, isset($conf[$name]) ? $conf[$name] : array());

If not, I'll let it go, but it's a ternary operation inside a function call. Ugly, IMO.

beejeebus’s picture

re #14 - i don't feel strongly about it, so i'm ok with changing it if it bothers others.

agentrickard’s picture

It's just the second patch I've reviewed today that looks to me like a violation of coding standards.

jhedstrom’s picture

FileSize
3.39 KB
PASSED: [[SimpleTest]]: [MySQL] 35,690 pass(es). View

I re-rolled #12 to split the ternary operation into a separate line.

agentrickard’s picture

Status: Needs review » Reviewed & tested by the community

Appeasement!

chx’s picture

Status: Reviewed & tested by the community » Needs review

The proper status is "needs discussion". When we discussed this last year in Denver our thought was a local XML file. Writing settings.php is really tricky and I would love to avoid that. Really. Also, a global? In an OOP system?

jhedstrom’s picture

Do we really want to force people to write out local configuration to an XML file? My understanding was that the XML component of CMI was to be mostly hidden to non-developers.

beejeebus’s picture

re #19: we're not providing a mechanism via CMI to write to settings.php - that would be hard to do safely.

what we're providing is the ability (which exists now) to override configuration that lives in persistent storage with values in settings.php.

it's really ugly, because we're just using the existing ugly, global $conf. i've created #1493468: create a bootstrap config object that the config system can depend on - which will hopefully see us get rid of global $conf and replace it with something more inline with the rest of the config system.

agentrickard’s picture

My understanding cd this patch is that it is a shim to prevent losing existing functionality. I would expect it will be changednas D8 matures. But without this patch, we break techniques that have worked since drupal 5.

Pol’s picture

I don't know if it's appropriate, but... maybe we should do a function who loads xml 'on the fly' and use it as configuration.

beejeebus’s picture

re. #23 - i think that's a separate issue.

personally, i'm not sure i like the feature, but either way, please create another issue if you want to get it in - i think it's out of scope here.

Dave Reid’s picture

+++ b/core/lib/Drupal/Core/Config/DrupalConfig.phpundefined
@@ -81,17 +81,22 @@ class DrupalConfig {
+    $name = $this->_verifiedStorage->getName();
+    $data = isset($conf[$name]) ? $conf[$name] : array();
+    $merged_data = drupal_array_merge_deep($this->data, $data);
     if (empty($key)) {

It would be nice if we could skip the call to drupal_array_merge_deep() if $data is an empty array.

+++ b/core/lib/Drupal/Core/Config/DrupalConfigVerifiedStorageInterface.phpundefined
@@ -81,4 +81,10 @@ interface DrupalConfigVerifiedStorageInterface {
+
+  /**
+   * Gets the name of this object.
+   */
+  function getName();
 }

Needs to also be declared as public in the interface?

Otherwise this is looking good to ensure we're not losing support for an existing feature keeping in mind we eventually want to get rid of it, but this is beneficial to implement for now.

jhedstrom’s picture

FileSize
3.43 KB
PASSED: [[SimpleTest]]: [MySQL] 35,702 pass(es). View

Re: #25

This patch only calls drupal_array_merge_deep() if it needs to, and declares getName() as public.

marcingy’s picture

if (isset($conf[$name])) {
   $merged_data = drupal_array_merge_deep($this->data, $conf[$name]);
}
else {
    $merged_data = $this->data;
}

Might be a bit cleaner using

$merged_data = isset($conf[$name]) ? drupal_array_merge_deep($this->data, $conf[$name]) : $this->data;
Dave Reid’s picture

Yeah that's what I thought as well.

agentrickard’s picture

When did the coding standards change to privilege ternary expressions?!?!?!

heyrocker’s picture

Status: Needs review » Reviewed & tested by the community

Ternaries are really a matter of taste, both are valid via coding standards. I actually am not a fan of ternaries so I'm going to give this the green light.

Dave Reid’s picture

+1 on the RTBC. It works.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

chx’s picture

Status: Fixed » Reviewed & tested by the community

It's OK.

chx’s picture

Status: Reviewed & tested by the community » Fixed

Opsie crosspost :D

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

Anonymous’s picture

Issue summary: View changes

Adding some notes