In the Drupal 8.x cycle we've endeavoured to remove all global variables from Drupal to reduce side effects and make unit testing easy. However we've introduced a few during the development cycle. One of them is $config. It is used in settings.php to override config values.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because globals work
Issue priority Major because it is a stated aim of Drupal to avoid using global state is possible. This was not achievable when global config overrides were first implement because we didn't yet have the kernel and settings class implemented.
Prioritized changes The main goal of this issue is to reduce fragility by preventing side effects in the configuration system.
Disruption Not disruptive as the method of declaring configuration overrides is exactly the same. There are no changes to settings.php. Some contrib tests might need to change if they are just setting global state but they can simply use WebTestBase::writeSettings() or create a new Settings() object.

Why should this be an rc target

Because global state is problematic for having a predictable system and testing. It would be great to be able to remove the configuration globals during the Drupal 8 cycle. Therefore we should make them internal and provide getters so that code can rely on setting them in settings.php and having a way to access them that will not change.

Files: 
CommentFileSizeAuthor
#24 2183591-new-24.patch4.5 KBalexpott
#12 interdiff.txt678 bytesdawehner
#12 2183591-12.patch18.09 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,668 pass(es), 2 fail(s), and 0 exception(s). View
#10 2183591.10.patch18.23 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,349 pass(es). View
#10 4-10-interdiff.txt7.86 KBalexpott
#4 2183591.4.patch18.29 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,570 pass(es). View

Comments

sun’s picture

A proper singleton that can't be futzed with at regular/non-test runtime (as opposed to current Settings) requires #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead

The parent issue contained an initial prototype, but I advanced that into a 100% secure singleton in #1757536: Move settings.php to /settings directory, fold sites.php into settings.php

andypost’s picture

mgifford’s picture

Status: Postponed » Active
alexpott’s picture

Status: Active » Needs review
Issue tags: +Needs change record
FileSize
18.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,570 pass(es). View

Here's a patch that removes the global $config by adding it to the Settings object which I think makes sense since this object corresponds to settings.php and is a singleton already.

alexpott’s picture

Issue summary: View changes
Issue tags: -Needs change record

Actually I don't think this needs a change record. Existing contrib should not be setting the global. I'd be surprised if even using this in tests was common.

alexpott’s picture

Assigned: sun » Unassigned
alexpott’s picture

Title: Replace global $config with GlobalConfig singleton » Replace global $config with a property on the Settings singleton
Berdir’s picture

+++ b/core/core.services.yml
@@ -229,7 +229,7 @@ services:
       - { name: event_subscriber }
       - { name: service_collector, tag: 'config.factory.override', call: addOverride }
-    arguments: ['@config.storage', '@event_dispatcher', '@config.typed']
+    arguments: ['@config.storage', '@event_dispatcher', '@config.typed', '@settings']
   config.installer:

That's actually the opposite of what we're doing in #2443351: Ensure that settings can't be serialized by not injecting them / replace stuff with container parameters..

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Site/Settings.php
    @@ -86,6 +94,23 @@ public static function get($name, $default = NULL) {
    +
    +  public static function hasConfigOverride($name) {
    

    Needs some quick docs, but the name describes things really well already.

  2. +++ b/core/lib/Drupal/Core/Site/Settings.php
    @@ -86,6 +94,23 @@ public static function get($name, $default = NULL) {
    +    return array_key_exists($name, self::$instance->configOverrides);
    

    Is there a reason you used ake() instead of the isset() calls in other current code?

  3. +++ b/core/modules/config/src/Tests/ConfigEventsTest.php
    @@ -64,8 +65,10 @@ function testConfigEvents() {
    +    $overrides = Settings::getConfigOverrides();
    ...
    +    new Settings(Settings::getAll(), $overrides);
    

    Does that mean for consisteny we should have Settings::getAllConfigOverrides() ?

alexpott’s picture

FileSize
7.86 KB
18.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,349 pass(es). View
  1. Fixed see 2
  2. I was overcomplicating this by thinking about being able to NULL config values - but that does not apply here since we working on the config object name level
  3. Fixed

Re #8 I'm not sure that that issue will pass our beta evaluation.

bill richardson’s picture

Status: Needs review » Needs work

Patch requires re- roll -- setting to needs work.

dawehner’s picture

Status: Needs work » Needs review
FileSize
18.09 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,668 pass(es), 2 fail(s), and 0 exception(s). View
678 bytes

Just some minor adjustment

Status: Needs review » Needs work

The last submitted patch, 12: 2183591-12.patch, failed testing.

joshi.rohit100’s picture

I am wondering if I change username/password of my db in settings.php, I see "The website encountered an unexpected error. Please try again later." error not "PDOException".

dawehner’s picture

Well, we hide exceptions by default. It is as simple as that, but we provide tools (see settings.local.php) to show the entire exception.

xjm’s picture

Version: 8.0.x-dev » 9.x-dev
Status: Needs work » Postponed

Unfortunately we can no longer change this safely in 8.x, so postponing to 9.x per discussion with cilefen and alexpott.

dawehner’s picture

What about a BC layer?

alexpott’s picture

I'm not sure a BC layer is possible because with a global anything can add to it at any point - yes it is fragile but if you set a config override before that specific configuration object has been loaded then it'd work. :( globals--

catch’s picture

Version: 9.x-dev » 8.1.x-dev

I don't think #18 constitutes an API break, so moving this back to a minor version for now.

catch’s picture

We could also add support for a new key, that's not a global, and deprecate $config.

alexpott’s picture

I was thinking that we could make global access to $config and $config_directories @internal and document that the only supported way of setting these is in settings.php.

catch’s picture

alexpott’s picture

I think that that means we need a getter for global $config so that as long as code is using the getter and setting them in settings.php then their code will not break.

alexpott’s picture

Issue summary: View changes
Status: Postponed » Needs review
Issue tags: +rc target triage
FileSize
4.5 KB

Something like this. I think it is important that the new getter is used somewhere to (a) prove it works and (b) prove we don't break it. Hence I've used in the ConfigFactory - the most important place. But instead of forcing the injection of Settings I've made it optional - which i think is okay because it is a global singleton itself. This buys us testability and the ability remove the global $config in the Drupal 8 cycle. I've also made the other config global @internal because it already has a getter.

dawehner’s picture

Mh, so does that mean we can use the global at some point just for setting and unset it later?

alexpott’s picture

It means the only supported way of setting it is in settings.php where you all you have to do is $config['blah'] = ...

This way no one has to interact with the global at all.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Issue tags: -rc target triage

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.