As discussed in #1833516: Add a new top-level global for settings.php - move things out of $conf, we want to use $config_override (or possibly something else, but not $conf) for config() overrides.

This should allow to better separate it from the new $settings array and just remove $confg once variable_*() functions are removed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Issue tags: +Configuration system
msonnabaum’s picture

I agree we should do this. The distinction between what $conf was in d7 and d8 is not very clear.

I'd go for $config though, since it's the name of the system. I could live with $config_overrides as well.

alexpott’s picture

+1 for $config too... it'll read nice. For example,

$config['system.logging']['error_level'] = ERROR_REPORTING_DISPLAY_ALL;

We don't need to name it override/s as the fact that we're overriding it is apparent from the fact it is in settings.php and hardcoded.

mtift’s picture

And if we do (can) change this, we still need to figure out the effect of those changes: #1934152: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects

mtift’s picture

This is not meant to sound condescending -- I just want to make sure I'm understanding the system as it is now:

Default error reporting is provided by the system module in:

core/modules/system/config/system.logging.yml

Currently, we have a UI where error reporting can be configured:

admin/config/development/logging

A change to this setting/configuration in the UI is reflected in

sites/d8.local/files/config_xxx/active/system.logging.yml

and written to config_cache (a database, memcache, redis, etc.). The active configuration can be changed by importing and exporting via the UI:

admin/config/development/export
admin/config/development/import

using the staging directory:

sites/d8.local/files/config_xxx/staging/

Additionally, this setting could also be overridden in settings.php with:

$conf['system.logging']['error_level'] = ERROR_REPORTING_DISPLAY_ALL;

This issue is about changing the name of $conf to $config_override or $config -- which is a variable that will co-exist with $settings in settings.php -- to reduce confusion. The $conf/$config_override/$config variable may or may not actually override this setting/configuration (see my comment #4 above). Furthermore, default.settings.php suggests to use "settings.local.php to override variables."

While I think I understand why many of these changes were put in place, this system as a whole is confusing to me, and I can't help but wonder if I am missing something.

[Edited for accuracy]

xjm’s picture

Priority: Normal » Major
Issue summary: View changes
Issue tags: +beta target

If we want to do this, I think we should do it before beta. Tagging accordingly. I'm on the fence regarding whether it's actually critical enough to be a blocker, though. Thoughts?

sun’s picture

Title: Change configuration overrides to use $config_override instead of $conf » Change configuration overrides to use $config instead of $conf
Related issues: +#2167105: Remove DRUPAL_BOOTSTRAP_VARIABLES, +#2167109: Remove Variable subsystem

I'd also go with $config (sans _override).

However, at the same time, I wondered whether the entire approach of $conf[ig] within settings.php is still the right architectural approach?

  1. Most $conf overrides that you previously had in settings.php are $settings now.
  2. Use-cases for overriding $config in settings.php are very limited, as you do not have access to a working service container or anything else yet.
  3. Unless I'm mistaken, the configuration system triggers a (single) event upon initialization, which you need to use for more sophisticated overrides anyway?
  4. Given the environmental bootstrap circumstances, $config overrides in settings.php are limited to (1) "dumb" PHP and perhaps more likely (2) static overrides?

As a consequence, wouldn't it make more sense to have $conf_path/config.yml containing:

system.logging:
  error_level: verbose

system.site:
  name: "My hardcoded site name"

# Or in other words:
{config_name}:
  {override declaration}

Thoughts?

Berdir’s picture

The advantage of settings.php is IMHO that you can easily have a global and a environment specific override by following the standard settings.php + local.settings.php approach, we use that a lot.

Also, as for the use cases, keep in mind that you can also override every. single. config entity with the same approach. For example, to force that a search api server configuration on staging always points to the staging solr server, that requires custom on-off solutions right now.

sun’s picture

Status: Active » Needs review
FileSize
4.51 KB

OK, I see, and I guess the data format in which $config is declared is a separate issue.

Perhaps a slightly more fundamental suggestion for this issue here is to (1) not declare $config as a global, but instead (2) inject the loaded data into the Config service (somehow).

Prototype patch attached to clarify what I mean.

sun’s picture

FileSize
31.28 KB

Let's just see how this goes. Patch will most likely fail, but let's talk about the architecture first.

Status: Needs review » Needs work

The last submitted patch, 10: drupal8.conf-config.10.patch, failed testing.

Anonymous’s picture

not a full review, but i like the idea of injecting the settings override instead of using a global.

catch’s picture

Priority: Major » Critical
Issue tags: -beta target +beta blocker

Didn't review sun's patch, but making this not a global seems like a good idea - we've already done it for $settings. I'm not sure if it's such a great idea to do that here - rename ought to be pretty quick, and that could be a quick follow-up.

The rename by itself seems critical to me - people are going to have to go through old settings.php to figure out what should go where, and having none of the new things being $conf makes that a lot easier.

sun’s picture

Status: Needs work » Needs review
FileSize
33.92 KB

Added new GlobalConfig class singleton, to see how that would look like. Unlike Settings, it actually ensures that it's read-only, except in tests.

Status: Needs review » Needs work

The last submitted patch, 14: config.conf_.14.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
36.54 KB
8.73 KB

Heh, hopefully I'm faster than other core commits this time ;)

Attached patch adjusts the Simpletest base classes to clean and set up GlobalConfig at the proper time. However, the global conf hacks in run-tests.sh are looking dubious to me, so testbot might still fail to run.

Berdir’s picture

The patch is green :p

The fatal error detection still only works sometimes ;)

sun’s picture

FileSize
36.55 KB
2.61 KB

Fixed run-tests.sh:

1. TestBase::$verbose can simply be made public, so as to allow run-tests.sh to pre-set it.

2. The simpletest.settings:clean_results configuration value is completely unnecessary and not used during a test run; it is only used in the parent site/script after a test has been executed, hence removed.

The last submitted patch, 18: config.conf_.18.patch, failed testing.

sun’s picture

FileSize
46.19 KB
10.03 KB

Fixed failing tests.

The last submitted patch, 20: config.conf_.20.patch, failed testing.

sun’s picture

FileSize
46.19 KB

Whoopsie, sorry. Fixed invalid PHP syntax.

Status: Needs review » Needs work

The last submitted patch, 22: config.conf_.22.patch, failed testing.

The last submitted patch, 22: config.conf_.22.patch, failed testing.

sun’s picture

Since I discovered that I need the exact same Simpletest changes for #1757536: Move settings.php to /settings directory, fold sites.php into settings.php, and because I advanced on them over there, I've separated them into a dedicated issue:

#2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead

swastik1608’s picture

Assigned: Unassigned » swastik1608
swastik1608’s picture

Assigned: swastik1608 » Unassigned
sun’s picture

Anonymous’s picture

hmm. we don't fire 'config.init' events any more.

also, not sure this is blocked on both of those issues.

catch’s picture

Status: Postponed » Needs work

Yeah I don't see why a simple rename of a variable needs to be blocked on those two issues.

If it's making it not a global, let's do the rename here, then the not-a-global refactor in a follow-up.

Berdir’s picture

Agree that it's probably better to split up the rename and not making it a global.

For the rename, it is probably easier to wait for #2167109: Remove Variable subsystem, as that should remove (almost) all $conf's that are not related to config overrides.

sun’s picture

Status: Needs work » Needs review
FileSize
27.06 KB
13.71 KB
  1. Removed all GlobalConfig singleton changes. → #2183591: Replace global $config with a property on the Settings singleton
  2. Updated for new Config override architecture.

As visible in the patch, I'm strictly using $GLOBALS['config'] instead of importing the global, because (1) many functions already have a local $config (object) variable and (2) to make it easier to grep for these instances in the singleton conversion later on.

Berdir’s picture

  1. +++ b/core/includes/bootstrap.inc
    @@ -2085,14 +2088,14 @@ function drupal_valid_test_ua($new_prefix = NULL) {
      */
     function _drupal_load_test_overrides($test_prefix) {
    -  global $conf, $config_directories;
    +  global $conf, $config_directories, $config;
     
       // Do not use the parent site's config directories. Use only the child site's.
       // @see \Drupal\simpletest\TestBase::prepareConfigDirectories()
    @@ -2107,7 +2110,7 @@ function _drupal_load_test_overrides($test_prefix) {
    
    @@ -2107,7 +2110,7 @@ function _drupal_load_test_overrides($test_prefix) {
       if (file_exists($filename)) {
         $settings = settings()->getAll();
         $conf_path = &drupal_static('conf_path');
    -    // This can override $conf, $conf_path, $settings, and $config_directories.
    +    // This can override $config, $conf_path, $settings, and $config_directories.
         include $filename;
         // Keep the overriden $conf_path alive across drupal_static_reset() calls.
         // @see conf_path()
    

    You replaced it in the comment but added it in the global, instead of replacing.

  2. +++ b/core/includes/bootstrap.inc
    +++ b/core/includes/bootstrap.inc
    @@ -539,14 +539,17 @@ function drupal_settings_initialize() {
    
    @@ -539,14 +539,17 @@ function drupal_settings_initialize() {
       // Export these settings.php variables to the global namespace.
       global $databases, $cookie_domain, $conf, $db_prefix, $drupal_hash_salt, $base_secure_url, $base_insecure_url, $config_directories;
       $conf = array();
    +  $settings = array();
    +  $config = array();
     
       // Make conf_path() available as local variable in settings.php.
       $conf_path = conf_path();
       if (is_readable(DRUPAL_ROOT . '/' . $conf_path . '/settings.php')) {
         include_once DRUPAL_ROOT . '/' . $conf_path . '/settings.php';
       }
    +  // Initialize Settings.
    +  new Settings($settings);
    

    I think we need to add $config to global then as well here. Probably replace $conf.

  3. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
    @@ -1052,6 +1052,9 @@ private function prepareEnvironment() {
         // Reset all variables to perform tests in a clean environment.
         $conf = array();
     
    

    I think this can be removed now, I just had to leave it because the testbot does set simpletest config overrides, but those are dead anyway now. (we will need to update it, because this will run with verbose = TRUE)

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Installer/InstallerLanguageTest.php
    @@ -24,11 +24,10 @@ public static function getInfo() {
     
       function setUp() {
    +    // Override the installation translations directory path.
    +    $GLOBALS['config']['locale.settings']['translation']['path'] = drupal_get_path('module', 'simpletest') . '/files/translations';
    +
         parent::setUp();
    -    // The database is not available during this part of install. Use global
    -    // $conf to override the installation translations directory path.
    -    global $conf;
    -    $conf['locale.settings']['translation.path'] =  drupal_get_path('module', 'simpletest') . '/files/translations';
       }
    

    Doesn't this mean that it will be unset, or is the installer test different?

  5. +++ b/core/modules/system/lib/Drupal/system/Tests/Installer/InstallerTranslationTest.php
    @@ -73,9 +81,13 @@ protected function setUp() {
     
    -    // Submit the standard profile installation.
    -    $this->drupalPostForm(NULL, array('profile' => 'standard'), $submit_value);
    +    // Submit the Minimal profile installation.
    +    $edit = array(
    +      'profile' => 'minimal',
    +    );
    +    $this->drupalPostForm(NULL, $edit, $submit_value);
    

    This specifically uses standard, because it was broken and the tests didn't cover it.

Status: Needs review » Needs work

The last submitted patch, 32: config.conf_.32.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
27.2 KB
3.85 KB
  1. Fixed drupal_settings_initialize() does not export $config into a global variable.
  2. Fixed InstallerLanguageTest::setUp() weirdness.
  3. Reverted InstallerTranslationTest installation profile back to Standard.
  4. Check what happens if global $conf is removed entirely.

Status: Needs review » Needs work

The last submitted patch, 35: config.conf_.35.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
28.82 KB
2.14 KB

Fixed ConfigOverride tests.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks great to me.

xjm’s picture

Reminder: you can now create a draft change record for API changes.
https://groups.drupal.org/node/402688
A change record will be required for an API change to be committed starting Feb. 14, but you're encouraged to create a draft for this issue now as well. :)

Anonymous’s picture

sun++

sun’s picture

Note: Some changes in this patch conflict with #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead and given how many other critical/major issues are currently blocked on that or will benefit from it, I'd vastly prefer to see the simpletest cleanup to land first.

This patch here can be merged/rebased much more easily anyway (and I'd be more than happy to do so).

sun’s picture

FileSize
28.82 KB
660 bytes

Fixed small oversight in system_install().

catch’s picture

Title: Change configuration overrides to use $config instead of $conf » Change notice: Change configuration overrides to use $config instead of $conf
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

The other issue is CNW at the moment, so I've gone ahead and committed this one. Committed/pushed to 8.x, thanks!

Needs a short change notice. We might want to re-purpose http://drupal.org/node/1882698 (the change notice for $settings) so it handles the two places where $conf overrides end up rather than adding a new one? Or they should cross-reference at least.

xjm’s picture

Issue tags: +Missing change record
Gábor Hojtsy’s picture

Title: Change notice: Change configuration overrides to use $config instead of $conf » Change configuration overrides to use $config instead of $conf
Priority: Major » Critical
Status: Active » Fixed
Issue tags: -Needs change record, -Missing change record

Status: Fixed » Closed (fixed)

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