Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#42 | interdiff.txt | 660 bytes | sun |
#42 | config.conf_.42.patch | 28.82 KB | sun |
#37 | interdiff.txt | 2.14 KB | sun |
#37 | config.conf_.37.patch | 28.82 KB | sun |
#35 | interdiff.txt | 3.85 KB | sun |
Comments
Comment #1
sunComment #2
msonnabaum CreditAttribution: msonnabaum commentedI 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.
Comment #3
alexpott+1 for $config too... it'll read nice. For example,
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.
Comment #4
mtiftAnd 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
Comment #5
mtiftThis 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]
Comment #6
xjmIf 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?
Comment #7
sunI'd also go with
$config
(sans _override).However, at the same time, I wondered whether the entire approach of
$conf[ig]
withinsettings.php
is still the right architectural approach?$conf
overrides that you previously had insettings.php
are$settings
now.$config
insettings.php
are very limited, as you do not have access to a working service container or anything else yet.$config
overrides insettings.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:Thoughts?
Comment #8
BerdirThe 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.
Comment #9
sunOK, 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.
Comment #10
sunLet's just see how this goes. Patch will most likely fail, but let's talk about the architecture first.
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentednot a full review, but i like the idea of injecting the settings override instead of using a global.
Comment #13
catchDidn'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.
Comment #14
sunAdded new GlobalConfig class singleton, to see how that would look like. Unlike Settings, it actually ensures that it's read-only, except in tests.
Comment #16
sunHeh, 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.
Comment #17
BerdirThe patch is green :p
The fatal error detection still only works sometimes ;)
Comment #18
sunFixed 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.
Comment #20
sunFixed failing tests.
Comment #22
sunWhoopsie, sorry. Fixed invalid PHP syntax.
Comment #25
sunSince 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
Comment #26
swastik1608 CreditAttribution: swastik1608 commentedComment #27
swastik1608 CreditAttribution: swastik1608 commentedComment #28
sunBlocked on the following issues to get in:
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedhmm. we don't fire 'config.init' events any more.
also, not sure this is blocked on both of those issues.
Comment #30
catchYeah 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.
Comment #31
BerdirAgree 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.
Comment #32
sunAs 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.
Comment #33
BerdirYou replaced it in the comment but added it in the global, instead of replacing.
I think we need to add $config to global then as well here. Probably replace $conf.
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)
Doesn't this mean that it will be unset, or is the installer test different?
This specifically uses standard, because it was broken and the tests didn't cover it.
Comment #35
sunComment #37
sunFixed ConfigOverride tests.
Comment #38
catchLooks great to me.
Comment #39
xjmReminder: 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. :)
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous commentedsun++
Comment #41
sunNote: 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).
Comment #42
sunFixed small oversight in system_install().
Comment #43
catchThe 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.Comment #44
xjmComment #45
Gábor HojtsyUpdated the change notice: https://drupal.org/node/1882698/revisions/view/2549546/6898039 :)