[DX] Allow `config_get()` to return a default value - like `settings_get()` and `update_variable_get()` do

[klonos]

This is a sibling issue to #6107. The same request, but for the config_get() function instead.

Details coming soon...

[klonos]

@docwilmot mentioned the following in https://github.com/backdrop/backdrop-issues/issues/6107#issuecomment-1537560288:

I think those two functions don't have a default parameter because their defaults are set in files. config_get() should have a default in the config file, ...

@klonos

Right. That default would be useful in the cases when the module that this config file belongs to is being installed, or when reverting the config to defaults. I would still like to be able to specify a default config value programmatically, to act as a fallback if the config value has been deleted (for whatever reason). But I agree that that would be a nice-to-have. Lets discuss that further in #6108.

[docwilmot]

Right. That default would be useful in the cases when the module that this config file belongs to is being installed, or when reverting the config to defaults. I would still like to be able to specify a default config value programmatically, to act as a fallback if the config value has been deleted (for whatever reason). But I agree that that would be a nice-to-have. Lets discuss that further in https://github.com/backdrop/backdrop-issues/issues/6108.

I didnt quite understand what you mean here @klonos

[klonos]

Sorry that wasn't clear. I was referring to this:

config_get() should have a default in the config file

So for modules, we have a /config folder, which in its simplest implementation holds a single MYMODULE.settings.json file, which holds the defaults for various settings. These defaults are being imported when the module is installed, and they are also used if we provide a "revert to default" functionality. Right?

Now, we also have cases where individual configuration entries are saved to config files that don't necessarily "belong" to the module that includes the code to do that (think #config property in forms, but also other custom code/logic). Similarly, we might have code that removes config entries outside of any "standard" uninstallation or revert routine. Correct?

If the above are correct, then we may have code (in core/contrib or custom) that incorrectly/accidentally deletes config entries that it shouldn't, in which case trying to retrieve these settings with config_get() would return NULL. So it would be nice to have the ability to specify a "safe" default value in that case, as defensive coding. No?

[avpaderno]

I understand that the default value returned from config_get() is the value set in the configuration file, but when the required key is not found, config_get() returns NULL. See Config::getOriginal().

$parts = explode('.', $key);
if (count($parts) == 1) {
  return isset($this->data[$key]) ? $this->data[$key] : NULL;
}
else {
  $value = $this->data;
  $key_exists = FALSE;
  foreach ($parts as $part) {
    if (is_array($value) && array_key_exists($part, $value)) {
      $value = $value[$part];
      $key_exists = TRUE;
    }
    else {
      $key_exists = FALSE;
      break;
    }
  }
  return $key_exists ? $value : NULL;
}

In some cases, getting a value that is different from NULL could simplify the code. As it is, the code that retrieves a configuration value needs to verify the returned value is NULL and use a value that is more useful as default value (for that specific code).

[avpaderno]

Changing config_get($config_file, $option = NULL) to config_get($config_file, $option = NULL, $default = NULL) would not change the returned value for the existing code. Only the code that passes three arguments to config_get() would get a value that is different from NULL when the required value is not found.

[avpaderno]

That said, if a module needs to use a different default value for the same configuration value, for example depending on the function calling config_get(), that module could just not set a value in the .settings.json file, verify that config_get() returns NULL and use the default value for that specific function. It would work without adding a new parameter to config_get().

Given that what I described is probably a not-very-common case, I would rather not add a new parameter to config_get(). Multiple default values for a configuration value seems rather confusing and cause of possible errors.

[klonos]

I'd forgotten that I filed this issue, and then went and filed a duplicate: #6605 ...sorry 🤦🏼 😞

@klonos: See:

• state_get(state_get($name, $default = NULL))

  $default: The default value to use if this state has never been set.

• settings_get($name, $default = NULL)

  mixed $default: (optional) The default value to use if this setting is not set.

We should have the same for config_get(). This should be a small API addition, but it would help reduce a lot of similar code in both contrib and core (all the conditionals that check if the config value was set and provide a fallback if not).

@docwilmot:

Config already has defaults; thats what the config file is for, and thats why config_get() didn't include it. Can't define defaults in two places.

@klonos:

@docwilmot that's based on the assumption that default config is provided + that that config includes an entry for the config value one is trying to retrieve with config_get(). This feature here would be more like in-built, OOTB, standardized defensive coding.

@docwilmot:

But I think this is a reasonable assumption, that if you develop a module that uses config, you should create a config file to define your defaults. I don't see a need for a duplicate process.

@argiepiano:

Config already has defaults; thats what the config file is for, and thats why config_get() didn't include it. Can't define defaults in two places.

I have to agree with this. Configs provide all defaults in the file , and it's reasonable to assume that, if you are using config values, you need to provide at least an empty value for each needed key

[klonos]

Perhaps if I gave a recent example where this came up again. Please refer to this PR in contrib: https://github.com/backdrop-contrib/css_injector/pull/14/files#diff-9f7666b5aa4ac2586752a1b79ae711816352a46da0400c3ee8339bd85f87fb3cL159

So people are trying to do things like this, where they need to set the value to something other than NULL if the config value does not exist (for whatever reason):

'#default_value' => config_get('css_injector.settings', 'css_injector_use_ace') ?: 1,

Whereas what I am proposing is to allow them to do this instead:

'#default_value' => config_get('css_injector.settings', 'css_injector_use_ace', 1),

I hope that it makes a bit better sense now.

[argiepiano]

I hope that it makes a bit better sense now.

The problem you are trying to solve by changing a core api function can be fixed by providing a default value for css_injector_use_ace in the file css_injector.settings.json. This is what config files are for, and this is the correct way to use the api.

As tempting as this change sounds, as has been mentioned above, it's risky to provide defaults in two ways. From a coding point of view, this default duplication opens the door for unintended "bugs".

[klonos]

This is what config files are for ...

The same can be said for settings*.php files though, yet settings_get() provides an option to set a default to fall back to in case the setting was not added to a settings*.php file. So the proper way would be to add a value either in the settings.php file that comes with core (for core-related code), or a custom settings file for custom things.

..., and this is the correct way to use the api

That is the only way to use the API currently (in the context of this request here). What I am suggesting would also be a "correct" way once it is in the API. Right?

If we employ the same thinking, there would be no need for the $default parameter in state_get() nor in settings_get() - people would need to write custom conditionals, to make sure that a value is set if these return NULL/FALSE. So doing things like this would also work, even if $default was not already provided as a parameter in these functions: $something = state_get('something') ?: $default;

By the way, I'm not saying that there aren't other ways to do this - what I'm saying is that as a developer, I'd expect all these *_get() functions to behave somewhat consistently. config_get() feels like being "left out".

[avpaderno]

There is a substantial difference between config_get(), state_get(), settings_get(): What config_get() returns is what modules put in their config/*.json files or what has been set with config_set(); what state_get() returns is what has been set with state_set(); what settings_get() returns is what set in the settings.php file.

While for state_get() and settings_get() makes sense to have a $default parameter, it does not make sense for config_get(), since the default value is already stored in the modules' config/*.json files.

If a module asks to config_get() a value that has never been set, it will get NULL, which states No value has been set; use your own default value. I would not return a different value, in this case, since the chances to hide a bug in a module are probably high. (It could be the module calling config_get() which uses the wrong parameters, a module that has a typo in its config/*.json file, or a module that does not have any config/*.json file.)

[avpaderno]

As a side note, using code similar to config_get('css_injector.settings', 'css_injector_use_ace') ?: 1 is wrong, when 0 is an acceptable value for css_injector_use_ace. In fact, in that case, the value of the config_get('css_injector.settings', 'css_injector_use_ace') ?: 1 expression is 1.

[klonos]

Thanks @kiamlaluno ...and yes, the specific example with css_injector was a bug indeed - not arguing that. I only used that case as an example because it is not the first time I see people trying to set a default value with config_get() in-code.

And yes, I agree that under ideal circumstances, the default will have been set in the .json config file, so there would not be any need to specify a default. But the same can be said for settings_get(): that if you are trying to get a setting from the file, then there should be no need for settings_get() to provide an option for a default - whatever the default needs to be would need to be set in settings.php. Right? ...yet, we allow specifying a default in that function ...for the case where the default has not been set in the settings.php file (for whatever reason).

Anyway, if nobody else sees the need for this, then I'm happy to close this as "works as designed".

[avpaderno]

settings_get() reads its values from the settings.php file, which is not controlled by Backdrop core nor modules. Modules control what config_get() returns. They control also what state_get() returns (with a call to state_set()), but they do not have a .json file for state values, which means they cannot be sure state_get() returns the value they asked for.

I already commented saying that adding another parameter to config_get() (with a default value) would not cause any issue to modules which do not use it. I still have to find a use case for which that parameter is necessary, or makes easier for developers to write code, considering that PHP now has the null coalescing operator and the Elvis operator.