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;
CommentFileSizeAuthor
#26 1484690-26.patch3.43 KBjhedstrom
#17 1484690-17.patch3.39 KBjhedstrom
#12 1484690-12-conf.patch3.37 KBPol
#9 1484690-9-conf.patch3.4 KBPol
#8 1484690-8-conf.patch3.52 KBPol
#1 1484690-1-conf.patch2.22 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Active » Needs review
FileSize
1484690-1-conf.patch2.22 KB

ok, first cut patch attached.

Pol’s picture

Pol
the/that
Location Brussels
CreditAttribution: Pol commented
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.";
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
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

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented

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;
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

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

Pol
the/that
Location Brussels
CreditAttribution: Pol commented

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.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

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

Pol
the/that
Location Brussels
CreditAttribution: Pol commented
FileSize
1484690-8-conf.patch3.52 KB

Patch updated with tests.

Pol’s picture

Pol
the/that
Location Brussels
CreditAttribution: Pol commented
FileSize
1484690-9-conf.patch3.4 KB

Updated patch, combined the two tests into one.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented 
+++ 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.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
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

Pol
the/that
Location Brussels
CreditAttribution: Pol commented
FileSize
1484690-12-conf.patch3.37 KB

Here we are.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review

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

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

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.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

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

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

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

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
FileSize
1484690-17.patch3.39 KB

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

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented
Status: Needs review » Reviewed & tested by the community

Appeasement!

chx’s picture

chx CreditAttribution: chx commented
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

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented

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.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

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

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

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

Pol
the/that
Location Brussels
CreditAttribution: Pol commented

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.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

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

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented 
+++ 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

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
FileSize
1484690-26.patch3.43 KB

Re: #25

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

marcingy’s picture

marcingy CreditAttribution: marcingy commented 
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

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Yeah that's what I thought as well.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

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

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented
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

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

+1 on the RTBC. It works.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

chx’s picture

chx CreditAttribution: chx commented
Status: Fixed » Reviewed & tested by the community

It's OK.

chx’s picture

chx CreditAttribution: chx commented
Status: Reviewed & tested by the community » Fixed

Opsie crosspost :D

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Adding some notes