Allow themes to have config folders

[docwilmot]

Description of the need

Currently CMI does not scan themes for a config folder. Not sure if there is any major disadvantage to doing so, but it may be helpful in solving Use config for color module palettes

Themes do save data in config, but only by syncing from the info file during enabling, and/or saving the settings form.

This may also allow deprecating info file settings such as settings[]=xxx etc.

Proposed solution

Allow themes to have config folders

[klonos]

This is part of the #5317 meta, but also sounds like it could be a duplicate (or subset) of what we are trying to achieve in #4829.

[docwilmot]

I dont think its quite the same as #4829. I've pushed my code.
(edited)

[stpaultim]

Interesting issue. This sounds like it could be really useful, but I've not thought a lot about this. Seems like this could benefit contrib theme developers.

What do others think about this?

[avpaderno]

I would like to see this implemented because theme_get_setting() loads the theme settings with config($theme . '.settings')->load(); the values set in the .info files are used as fallback values.

[yorkshire-pudding]

This makes a lot of sense. A lot of themes have settings, and this would make the config consistent with modules.

[docwilmot]

Added a brief test to this to show that themes without a settings[] array in the info file will still show a settings page with correct settings.

[quicksketch]

Thanks @docwilmot! Test coverage and the general feature look solid to me! I added a commit to add @since 1.26.0 lines to the docblocks. What's the best place to document this new ability? It's API backwards-compatible, so a change record isn't strictly necessary.

[jenlampton]

a change record isn't strictly necessary.

I still think it would be a good idea, since people will probably still be looking for it on the API site.

[quicksketch]

I merged backdrop/backdrop#4440 into 1.x. This looks great! Thanks @docwilmot for your work on this!

[klonos]

Reopening this, in order to keep it within our radar for a change record.

[argiepiano]

I'm not sure if this fact was considered when creating these changes, @docwilmot. I haven't done any testing to see if this will create regressions, BUT:

Color module already creates <theme_name>.settings.json when it rebuilds the customized colors for a theme. This is done by color_rebuild_settings(). This means that even a theme that doesn't provide a default <theme_name>.settings.json, WILL get one when :

1. Color is enabled
2. You have at any point in time gone to the theme settings path (e.g. admin/appearance/settings/basis) and clicked Save

BTW, this settings file created by Color gets rebuilt every time you clear caches.

This came up as I was investigating an issue with a subtheme (Thesis) and its puzzling handling of overridden colors. Then I saw what this patch did to core (basically checking for the existence of <theme_name>.settings.json in theme_has_settings()) and a red flag came up in my mind: basically, two potential issues: (1) a theme that DOESN'T provide <theme_name>.settings.json WILL have such a file created by Color, and (2) more seriously, that file WILL get rewritten by Color every time you clear the caches.

As a side note: The use of a config file that gets rebuilt by a module on cache clears (by Color) seems wrong to me.

[argiepiano]

Did a bit of testing with a custom test theme that provides a settings config file with a dummy key. Color does add its usual stuff to that file under key "color", but it doesn't delete any other information that theme provides in that file - so we are safe, as my point (2) above doesn't actually happen. However point (1) may still be a concern.

[docwilmot]

Yeah, that could be an issue. We'll need to check if the json file has only a "color" key in there, and ignore that conditional if so. Good catch.

[docwilmot]

Well, I was going to create an issue, but now I'm not so sure. Point 1 in @argiepiano's post may not be entirely valid AFAICT: the json file is still only created if you save the settings page. So since theme_has_settings() as is now is checking that the theme has a settings file, or an entry settings entry in the info file, which should in fact return true if the theme only has color support without other settings, I think this is OK.

[avpaderno]

I left a comment on the PR, but I now noticed the PR has been already merged. (Yes, the purple sign was impossible to miss, but I was able to miss it. 🤦🏻‍♂️)

[avpaderno]

Should I open a new issue for that change, if it is indeed the right change?

[stpaultim]

@kiamlaluno

If you are suggesting a change to this PR, you should create a new issue. This issue is closed for work and merged, @klonos opened it only for purpose of creating a Change Record (as I understand it).

If there is a problem with this PR, it should be a new issue and a new PR.

[avpaderno]

@stpaultim I noticed the changes have been committed after I commented on the PR. I did not open an issue because I forgot about my comment on this PR, but I am considering that.

[herbdool]

I made a stab at a change record for this: https://docs.backdropcms.org/change-records/backdrop-will-now-discover-settingsjson-configuration-files-in-a-theme-config-folder. From my understanding, it's not just that a theme can have settings in a config folder, but that it should be .settings.json and there needs to be a hook_config_info() in template.php. Is that correct?

[laryn]

I haven't tried it yet but that looks correct from what I can tell. We should link to/mention that change record here as well:
https://docs.backdropcms.org/documentation/adding-theme-settings-advanced

EDIT: I took a stab at that.

[herbdool]

@laryn I see it. Looks good to me

[avpaderno]

On https://docs.backdropcms.org/documentation/adding-theme-settings-advanced, there are some <br /> tags that are rendered as code.

[bugfolder]

there are some <br /> tags that are rendered as code.

Fixed.