Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
ThemeSettingsForm uses theme_get_setting()
to get form values - this uses configuration with overrides (as it has to since it is primarily used at runtime). But this means that if any theme setting is overridden using config overrides then they can end up stored in configuration without the user intending that.
Steps to Reproduce
- Install standard
- Override a theme setting in settings.php for example $config['system.theme.global']['favicon']['use_default'] = FALSE;
- Go to /admin/appearance/settings - the override will change the form value - which it should not.
- Press save and the override will now be saved to the stored configuration - which it should not.
Proposed resolution
Step away from theme_get_setting()
for retrieving the default form configurations and use an editable configuration instead (similar to all other core themes).
Remaining tasks
- Once themes start to use an editable configuration, there will no longer be an inheritance of default settings, thus each core theme should ship with its own
config/install/THEME.settings.yml
file with defaults. - Above step also means that the global configuration becomes useless. All theme specific configurations will always be fully defined. Due to the logic in theme_get_setting(), theme specific configs will always override the global configuration. This can be discussed in #2866194: Theme specific configurations always override the global configuration.
- Some tests assume that changing a global configuration setting will be enough, however depending on the outcome of #2866194: Theme specific configurations always override the global configuration this might be an incorrect assumption and these tests might need to be changed. See #16 and #18.
User interface changes
TBD
API changes
TBD
Data model changes
TBD
Comment | File | Size | Author |
---|---|---|---|
#28 | prevent_theme_settings-bleed-2402467-28.patch | 6.57 KB | Neograph734 |
Comments
Comment #1
catchIs this still valid?
Comment #2
alexpottyes
Comment #3
joelpittetWhat is the proposed solution here?
Comment #4
joelpittetThis needs an example or steps to reproduce.
Comment #5
alexpottAdded steps to reproduce.
Comment #7
catchWe discussed this on a call with core committers and theme system maintainers and agreed it's definitely a major bug since it's completely inconsistent with how other configuration forms work and could result in development settings being deployed to production inadvertently.
Depending on the fix this could end up only happening in 8.2.x but leaving at 8.1.x for now.
Comment #10
Neograph734Switching to 'needs review' to get feedback on my idea.
The problem here is that almost all forms use the configuration supplied by the ConfigFormBase, by doing something like
'#default_value' => $this->config('CONFIG')->get('VALUE')
.All theme configurations get their defaults as provided by theme_get_setting():
The problem here is that theme_get_setting() is also the only way for other systems to fetch the theme data. For instance SystemBrandingBlock uses it to load the site logo (which should respect overrides).
So the solution would be to add an extra parameter to theme_get_settings to load it without overrides?
And then in ThemeSettingsForm do something like this:
Marking #2866194: Theme specific configurations always override the global configuration as a related issue because the global configured logo can also leak into a theme specific form.
Comment #11
joelpittetI've got a couple of proposals:
theme_get_setting()
(which may be a good issue on it's own regardless of the outcome of this issue), thanks for digging into this!The second proposal way would get us along the same lines as the rest of the config forms but still leaves the problem of representing the state of those overrides.
We could maybe extend the FAPI with something like #description but maybe a #overridden state that could be themed (though may be difficult to represent all widgets generically I suppose other than a message like "This field has overriden config values, your changes may not be reflected on the site" or something.
Comment #12
Neograph734I suppose there would also be a 3rd option: replace the theme_get_settings in ThemeSettingsForm by an editableconfig value. The form already defines a config key (with a comment that it is useless). All that needs to be done is to replace the theme_get_settings with this editableconfig. (I am tempted to think it used to be there, but was replaced with theme_get_settings for some reason.)
With respect to the options of @joelpittet, 1 could be implemented independent of this issue (site name, slogan, etc can also be overridden) though for me it works quite well the way it does now. It would be especially confusing if the value would be conditionally overridden (and blocked from editing) from a config override service.
2. This 3rd suggestion would be less code than 2 and would make the form consistent with all other config forms.
Comment #13
Neograph734Patched option 3 against Drupal 8.4-dev. In the end I guess this would make the most sense as the form is now similar to other configuration forms.
Optionally the documentation for theme_get_setting could be altered to include that it is only suitable for display purposes and not for configurations.
Comment #15
Neograph734Ok, somebody please help me out here...
The tests are failing because there no default settings for the theme (all checkboxes unchecked so the logo does not show up).
But per this change record, themes should define defaults in
config/install/THEME.settings.yml
. Strangely, none of the core themes does...Anybody knows why? Has this simply been forgotten, or is it deliberately?
UPDATE #2218655: Core expects $theme.settings default config file to exist
Comment #16
Neograph734Let's see what happens if we supply some Bartik defaults.
Comment #18
Neograph734Hmm, obviously I should have copied the logo and favicon groups from the global theme as well.
The user test should not have failed as I did not change anything in the global configuration, but just to be sure I have a second patch with an updated test.
If this passes I suppose all core themes should be updated to also include a
config/install/THEME.settings.yml
file and it should be good.Comment #21
Neograph734Before proceeding here, we might first need to decide on the solution for #2866194: Theme specific configurations always override the global configuration. Once that is decided, the failing patches can be repaired.
Comment #28
Neograph734I think this should get us a long way. The problem with earlier patches was that there was no default for theme specific configurations, so values could be undefined. This is now solved by duplicating a bit of logic from theme_get_setting(), ensuring that the configuration falls back the the global configuration when there is nothing defined on theme level.
Comment #33
borisson_I think this issue should be postponed on the other one? The patch looks great and I only have a question and one personal preference to remark here.
Personal opinion: Not a big fan of having an assigment in an if statement for readability reasons.
Should the formatting here (80 cols) be fixed because we touch these lines as well?