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

[klonos]

When trying to set up a settings form for a theme (using HOOK_form_system_theme_settings_alter() in a theme-settings.php file), I very often find myself doing something like this:

function MYTHEME_form_system_theme_settings_alter(&$form, &$form_state, $form_id = NULL) {
  $my_setting = theme_get_setting('my_setting');
  $form['my_setting'] = array(
    '#type' => 'checkbox',
    '#title' => t('Some setting'),
    '#default_value' => isset($my_setting) ? $my_setting : TRUE,
  );
}

I would like to be able to do the following instead:

function MYTHEME_form_system_theme_settings_alter(&$form, &$form_state, $form_id = NULL) {
  $form['my_setting'] = array(
    '#type' => 'checkbox',
    '#title' => t('Some setting'),
    '#default_value' => theme_get_setting('my_setting', 'MYTHEME', TRUE),
  );
}

In other words, change the function to include a 3rd, optional $default parameter so that it is theme_get_setting($setting_name, $theme = NULL, $default = NULL).

Similarly, I would like to be able to do that with config_get(), so change it to this:
config_get($config_file, $option = NULL, $default = NULL).

[klonos]

...actually, I would argue that for consistency, I would prefer it if we introduced a new function called theme_settings_get() (notice the pural "settings"), for the following reasons:

• consistency with the file that is called theme-settings.php
• consistency with all the other *_get() functions, where the verb "get" is at the end of their names.
• I believe that the need to set a default return value is much more frequent then to specify a $theme (because it defaults to the current theme). Since changing the order of function parameters is a backwards-compatibility-breaking API change, we are left with two options:
  • add the new $default parameter at the end of the current theme_get_setting() function, which forces us to do this in order to get the setting value for whatever the current theme is:
    '#default_value' => theme_get_setting('my_setting', NULL, TRUE), (needs that NULL specified as the 2nd parameter)
  • introduce a new function, where the $theme parameter is last, so that it can be ommitted completely, like so:
    '#default_value' => theme_settings_get('my_setting', TRUE),

[klonos]

I've just filed the first of what I expect to be a series of 3 2 PRs: backdrop/backdrop#4435

1. one that introduces a new theme_settings_get() function to replace/deprecate theme_get_settings()
2. an alternative to the first one, that simply adds a 3rd optional parameter to theme_get_settings() for specifying a default value.
3. another to add a 3rd parameter to config_get() for specifying a default value. moved to [DX] Allow config_get() to return a default value - like settings_get() and update_variable_get() do #6108

[klonos]

...actually, I think I better slit the 3rd item into its own separate issue 🤔

[docwilmot]

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, and theme_get_setting has defaults set already in two places: in the theme info file as settings[] = 'whatever' or the theme-settings.php file.

As for renaming theme_get_setting to conform, we alreaady have many functions where the "get_" bit comes before whats being gotten, like backdrop_get_path, backdrop_get_schema etc etc.

I'd say this PR would complicate rather than help TBH.

[klonos]

config_get() should have a default in the config file

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.

[klonos]

...theme_get_setting has defaults set already in two places: in the theme info file as settings[] = 'whatever' or the theme-settings.php file.

And that is what I don't like and would like to see changed. I still want to preserve the ability to specify defaults via the .info file (for backwards compatibility too), but I don't want that to be an absolute requirement. Perhaps I should have filed a separate META before raising this, but here's the DX I had as I was trying to do something simple as adding a few settings to an existing theme:

1. I created a theme-settings.php file, and started adding my form array in a hook_form_system_theme_settings_alter() implementation. When I finished with that, everything seemed right in the code, and I checked that against other core and contrib themes, I couldn't however get the settings form nor the "settings" link for that theme to show up (cleared caches, double- and triple-checked things etc.). No joy 👎🏼
2. I then started searching the core issue queue, and found that I had stumbled on this issue a few years back: [DX] Document the usage of theme-settings.php #3391 ...so I thought "OK, so perhaps that is the reason why", and I tried removing the wraping (obvious optional in Backdrop) MYTHEME_form_system_theme_settings_alter() implementation, and leave the form array "bare" in the theme-settings.php file. Cleared caches, checked things again. Still no joy 👎🏼
3. At this point, my code included defaults for the theme config, but only programatically, via #default_values in the form array. So, digging into the code of other themes that already had working settings forms, I tried adding settings[config] = value entries in the .info file. After clearing caches, the form finally showed up. So that was the point when I had both a 🎉 as well as a WTF! moment ...why the hell do I need to specify settings in the .info file, when I have specified defaults programatically? Why doesn't that work like the rest of core? 👎🏼
4. Further trials revealed that I do not even need specificsettings[config] = value entries in the .info file - simply adding an empty settings[] entry would do. However, if my .info file includes no settings[] entries, the settings form will NOT show up no matter what I try. Another WTF moment 👎🏼

So what seemd like a simple and relatively straight-forward task, ended being quite a few wasteful minutes of my life, some serious head-scratching, and the realization that we have unnecessary Backdrop-isms in place that make for a worse DX. I just expected things to "simply work"!

So this issue here is the begining of an attempt to fix a few of things that as a developer I expect:

• Consistency and expected behavior: if I am calling a settings_get() function to get a setting from a file called settings.php, then when trying to retrieve a setting from a file called theme-settings.php I expect the function that does that to be called theme_settings_get(). If the function that allows retrieving settings from settings.php allows for a defaul/fallback value to be specified, then I expect the equivalent function that retrieves settings from theme-settings.php to also allow the same.
• if I configure defaults in my code programatically, I do not want to be "forced" to declare them in another file (the .info). It's still nice if that was an option, but certainly not a hard requirement. For the task at hand (creating a form) and the workflow that I am used to from working with the Form API, I expect things to work in a similar manner as with settings forms provided by modules.
• I don't want to keep declaring variables for each form element I add + then also the same isset() conditional for those variables in the #default_value properties of each elemet - instead, I would want to use a single function in #default_value that does that for me, and falls back to a default if the setting is not found in config or any .info file. Saves me at least one variable per element, which may be several lines of code + why should I be declaring variables when there's absolutely no other logic in the form that uses them - I just want values retrieved from and saved in config - that's it.

I hope that the above helps clarify the drive behind this request @docwilmot ...it wasn't simply me going through code and wanting to change/rename things - I actually tried to do something relatively simple that turned out to be super-tedious and annoying.

[klonos]

...for some better context, here's the simple settings form I was trying to build:

...and here's the entire code that's now in my theme-settings.php file:

$form['help_text_position'] = array(
  '#type' => 'fieldset',
  '#title' => t('Help text position'),
  '#description' => t('The help/description text in forms is by default positioned after all elements, except for fieldsets, where it is positioned right after the legend. The settings below allow you to change that for the Seven admin theme.'),
  '#collapsible' => FALSE,
);

$position_options = array(
  'after' => t('After the element'),
  'before' => t('Before the element'),
  'invisible' => t('Invisible'),
);

$respect_explicit_description_display = theme_get_setting('respect_explicit_description_display');
$form['help_text_position']['respect_explicit_description_display'] = array(
  '#type' => 'checkbox',
  '#title' => t('Respect the <code>#description_display</code> property'),
  '#options' => $position_options,
  '#default_value' => isset($respect_explicit_description_display) ? $respect_explicit_description_display : TRUE,
  '#description' => t('This setting controls whether this property will be respected if it is explicitly set for an element, or if it will be overridden by the settings configured below regardless.'),
);

$fieldsets = theme_get_setting('fieldsets');
$form['help_text_position']['fieldsets'] = array(
  '#type' => 'select',
  '#title' => t('Fieldsets'),
  '#options' => $position_options,
  '#default_value' => isset($fieldsets) ? $fieldsets : 'before',
);
$radios_and_checkboxes = theme_get_setting('radios_and_checkboxes');
$form['help_text_position']['radios_and_checkboxes'] = array(
  '#type' => 'select',
  '#title' => t('Radios and checkboxes'),
  '#options' => $position_options,
  '#default_value' => isset($radios_and_checkboxes) ? $radios_and_checkboxes : 'after',
);
$default = theme_get_setting('default');
$form['help_text_position']['default'] = array(
  '#type' => 'select',
  '#title' => t('All other elements'),
  '#options' => $position_options,
  '#default_value' => isset($default) ? $default : 'after',
);

...and here's what's in my .info file:

settings[respect_explicit_description_display] = true
settings[fieldsets] = before
settings[radios_and_checkboxes] = after
settings[default] = after

As you can see:

• the $position_options variable is being reused by all select elements, so it has a purpose 👍🏼
• the $respect_explicit_description_display, $fieldsets, $radios_and_checkboxes, and $default variables have absolutely no reason to be there, other than specifically for the isset()s in the '#default_value' of each element. That's wasteful and completely unnecessary 👎🏼
• the exact same things that I have declared variables for in php need to also be placed as settings[] in the .info (otherwise the form won't show up) - also unnecessary and wasteful 👎🏼

In fact, I don't need all those settings[whatever] = default_value in the .info file - this empty settings array w/o any value also works:

settings[]

...but the crazy thing is that if I don't add it in the .info file, the form won't work (trying to access admin/appearance/settings/seven redirects me back to the themes listing page, where the "settings" link is missing for the theme to which I have added the form)

[kiamlaluno]

theme_get_setting() can return NULL when the requested value is not found. It would be much helpful if the code that calls theme_get_setting() could tell the function which value to use instead of NULL.

(I mean I would like to see this implemented.)

[docwilmot]

Then all functions that could return NULL be allowed to have a default!! menu_tree_get_path($menu_name, '<front>'), country_get_list('USA')...

@klonos said: And that is what I don't like and would like to see changed.

I'm being silly above but I think the whole point of putting config defaults in files was to make them portable, moving them back to code would be a grand step backwards, I'm sure we'd agree. So if these configs already have defaults on files, doesnt really make sense to me to create an opportunity to confuse things by having two possible defaults.

If the function returns NULL it means you forgot to add a default file, and you should fix that. Not change the whole function.

Maybe just me though.

[bugfolder]

Someday the minimum PHP will be 7.4 and then we can just use ??.

[argiepiano]

the whole point of putting config defaults in code was to make them portable, moving them back to code would be a grand step backwards, I'm sure we'd agree. So if these configs already have defaults in code, doesnt really make sense to me to create an opportunity to confuse things by having two possible defaults.

I have to say that I agree with @docwilmot on this whole discussion.

[kiamlaluno]

The NULL value returned from theme_get_setting() is not a default value; it is rather a fallback value. If it is considered a default value, then it is a default value set from code for which the we should not set default values in code objection would apply too.

That said, without the proposed change, the code to use is not much more complex.