Settings file configuration override not respected when no $key is specified in Config::get()

[eric2pin]

Description of the bug

If a configuration value is loaded entirely by core / modules without a value for $key, the override system isn't properly implemented.

Steps To Reproduce

Install and enable the Maillog module on a Backdrop CMS install. Add $config['system.mail']['default-system'] = 'MaillogMailSystem'; to your settings file. Visit admin/config/system/mailsystem to see your override was not implemented.

Actual behavior

Existing default as defined in config appears on admin/config/system/mailsystem

Expected behavior

Override set in settings file is respected.

Additional information

Backdrop version 1.23.3.

This appears to happen when configuration objects are loaded in their entirety, without a $key specified. While $key = null, Config::getOverride does not return overridden values defined in settings files. Adding the following code in Config:getOverride just before "return $value;" resolves the problem in my particular case:

if (is_null($key) && isset($this->overrides)) {
  $value = $this->overrides;
}

[indigoxela]

@eric2pin many thanks for reporting, but I have two requests: can you provide an example without contrib module involved? And can you point to the code lines you're referring to in the additional infos?

As for the maillog module - I suspect, you mixed config with settings. Things added to the settings.php file are handled via settings_get. In contrast to that the config handles things defined in json files in the config directory.

Correct me, if I misunderstood.

[indigoxela]

Ah, wait, I have to correct myself 😉

Looking at Config::isOverriden, one clearly sees that the key is not optional.

  public function isOverridden($key) {
    return $this->getOverride($key) !== NULL;
  }

If there's no key, then it's not overridden.

Neither is the key optional in Config:getOverride

I suspect, the missing key has to get addressed in the contrib implementation, didn't look at the contrib code, though.

[indigoxela]

A quick question: Why using settings.php, anyway? The value is available via config - wouldn't it be easier to switch to config->set() instead? Then everything is feasible via UI, and no fiddling with settings.php is necessary.

UPDATE: here's a working example in the mimemail module. Of course, I don't know if that also works for the maillog module.

[eric2pin]

@indigoxela Thanks for the quick reply!

With my hosting setup (Pantheon) my config is not writeable on test or prod environments and has to be changed in the repo only, then deployed. It seems defensible that config should be static on production, but that's a big and different topic.

I am using a settings.local.php style implementation to provide some configuration overrides for distinct environments. I need dev & test environments to override json config (like to turn off mail for non-production sites, turn of google analytics, etc). It's a nice bonus that with overrides instead of config changes, on the dev environment I can export all config changes to reintegrate back into production, without having to worry about exporting the dev-only config that shouldn't make it back to production (the overridden values for mail, analytics, etc).

In the Config::get() function $key defaults to '', and the comments state that an empty key returns the whole configuration object / array -- so it seems like totally acceptable code to not pass a $key. Config::get() calls getOriginal and getOverride.

Here's a core example: In mail.inc line 267 there's $configuration = config_get('system.mail'); with no $key specified. If I add $config['system.mail']['default-system'] = 'MyDefaultClass'; in my settings.php file and then execute the same $configuration = config_get('system.mail'); code, I can inspect the resulting contents of the var to see my override is not respected.

You can look inside getOverride to see that the code is always evaluating the contents of $this->overrides like "if (isset($this->overrides[$assembled_key])) {" -- but $assembled_key is null if the $key passed on request is null. So if a specific $key is overridden but the request is for the entire config option, the override is ignored.

I hope that makes sense!

[indigoxela]

With my hosting setup (Pantheon) my config is not writeable on test or prod environments and has to be changed in the repo only, then deployed.

I've zero experience with Pantheon, but that seems very "business" and also very inconvenient to me. 😉
Does that mean, you can't even override the site name or add a date format without the deployment run?

I can export all config changes to reintegrate back into production

Sure, that's one of the cool features of Backdrop.

config_get() works nicely without key.

getOverride() without key can not work. You can only override something that's defined by a key.

Let's try to figure things out the other way round. Override via settings.php has been implemented in #2805.
Maybe @quicksketch (who provided the code) or @herbdool (who seemed to have reviewed it), can give some info if the current behavior is as expected or not.

I see a recent change from $key to (string) $key, which means that php 8+ nagged with $key being NULL at some point. So core probably also calls that function without a key provided. That would indicate that we have to consider empty keys (and get rid of the casting to string).

[herbdool]

I agree a fix is needed and @eric2pin's fix makes sense to me.

The only time $key would be NULL is when the entire configuration object is loaded, as far as I can tell. So it makes sense in that case to return all the overrides for that config object. However, I think it needs more. Unless you override all the items in the config object it will end up losing those other items.

So if in settings.php you've got $config['system.mail']['default-system'] = 'MaillogMailSystem'; and your config object looks like:

{
    "_config_name": "system.mail",
    "default-system": "DefaultMailSystem",
    "foo": "bar"
}

It will lose "foo": "bar" since it's not being overridden. We don't want that.

We may need to improve Config::get() and Config::getOriginal() to account for this. If the key is NULL then it'll have to merge overrides with the originals. So as not to get this too tangled I wonder if part of this should be split off into a new method.

[quicksketch]

Rephrasing this again to check if I understand the problem correctly.

If $config['system.mail']['default-system'] = 'MaillogMailSystem'; is in settings.php, then the following examples return different things:

// Gets overridden value "MaillogMailSystem":
$mail_system = config_get('system.mail', 'default-system');

// Gets overridden value "MaillogMailSystem":
$mail_config = config('system.mail');
$mail_system = $mail_config->get('default-system');

// Gets original value "DefaultMailSystem":
$mail_config = config_get('system.mail');
$mail_system = $mail_config['default-system'];

The expectation is that all 3 examples should all return the overridden value. Is that right?

[herbdool]

@quicksketch correct.

[herbdool]

We might end up with a similar issue with Config::getTranslated().

[quicksketch]

I checked the core code base for any places where we're calling config_get($config_name) with no specific key, and looks like we only have 7 instances where config is loaded in this way, including:

• Loading layouts
• Loading custom blocks
• Loading text formats
• Loading the mail system settings

Seems like in the short-term we should change the way the mail system is loaded from config_get('system.mail') to config('system.mail'). That would solve the only core use-case of config override not working for a site setting. It's also one of the things most likely to be overridden via settings.php.

[herbdool]

@quicksketch that crossed my mind too, just so long as it eventually gets done. I've added a PR.

[quicksketch]

The PR at backdrop/backdrop#4008 looks good to me in code review. @eric2pin, could you check if that PR will solve your immediate problem?

[eric2pin]

@herbdool @quicksketch Yes, although it does leave in some counterintuitive (less important?) interface stuff. With this code in place anything that calls backdrop_mail_system() will return an instantiated mail class that respects your $config override in settings, which would divert mail to that system which is great.

The mail system admin form at admin/config/system/mailsystem won't reflect your overrides, though, as you might expect based on similar override behavior with say JS / CSS compression. That's because mailsystem_admin_settings() pulls config from mailsystem_get(), which also uses the standard config_get() approach without a key. So your override wouldn't really be visible, although it's working.

[kswan]

I ran into this issue as well. @argiepiano pointed me to this issue. I tested the PR and it works correctly. With the PR applied the configuration in settings.php overrides the site configuration.

@eric2pin I don't have a page at admin/config/system/mailsystem. I believe that page is added by a module you have installed.

[eric2pin]

@kswan You're right! It's the mailsystem module. This works great for core.

So for the mailsystem case would it be best to post an issue on that queue? Or just hold tight while changes to config.inc are considered?

[herbdool]

Sounds like this has passed review and testing. Anyone want to update the labels and mark it RTBC? Or if you don't have access I can do it on your behalf (with your approval).

[kswan]

I would label it RTBC, but I don't have access.
Should it also get "Milestone candidate - bug"?

[herbdool]

thanks @kswan I've updated it.

[quicksketch]

I merged backdrop/backdrop#4008 by @herbdool into 1.x and 1.21.x. This solves the immediate problem with the mailsystem config not being overrideable. Thanks @herbdool, @eric2pin, @kswan, and @indigoxela for helping sort this out!

I am leaving this issue open because we still have the broader question of how to handle config_get() calls that load an entire file.

(@kswan: I've added you to the triage group so you can now add labels and modify issues. Thanks and welcome!)

[herbdool]

@eric2pin I've created a PR for mailsystem as well: backdrop-contrib/mailsystem#23

[klonos]

...sorry for (re)adding/(re)removing the issue labels - as I was reading the thread, it seemed to me that they were missing, but then realized what's going on when I finally reached the end of the thread.

The mail system admin form at admin/config/system/mailsystem won't reflect your overrides, though, as you might expect based on similar override behavior with say JS / CSS compression.

BTW, we have #4331 re indicating in the admin UI which settings in forms have been overridden via settings.php. There's no easy/elegant solution to it currently.

[indigoxela]

The immediate problem has been fixed. Currently we have no open PR for this issue, and the direction is still unclear.
Time to remove the milestone?