Problem/Motivation

Values from custom theme settings set in *.info.yml are overridden by *.settings.yml in a theme’s config/install directory, which are in turn overridden by values set within the theme settings UI and stored in active config. Because the *.settings.yml file is in the install directory, it makes sense that those values are only relevant prior to a theme’s installation. When *.info.yml files are edited, though, the values generally have an effect once the cache is cleared. The fact that editing these values in *.info.yml sometimes doesn’t have an effect is not intuitive.

Proposed resolution

Remove custom theme settings from *.info.yml and rely solely upon *.settings.yml and active config.

User interface changes

None, assuming this works correctly.

API changes

Change notice will be needed to alert theme developers that all custom theme settings require a *.settings.yml in config/install, and these settings should not be duplicated in *.info.yml.

Original report by sun

  1. Hard-coded default theme settings are specified in a theme's .info.yml file.

    /core/themes/bartik/bartik.info.yml
    ---
    settings:
      features:
        favicon: true
      shortcut_module_link: true
    
  2. If a theme supplies a $theme.settings file, then it has to manually specify the (always identical) theme_settings config schema for it.

    /core/themes/bartik/config/schema/bartik.settings.yml
    ---
    # Required to validate default theme settings.
    # + satisfy DefaultConfigTest
    bartik.settings:
      type: theme_settings
      label: 'Bartik settings'
    
  3. If $theme.settings contains an additional configuration option of a module, then the theme's config schema has to manually specify (figure out) the config schema for that option (whereas the originating module does not specify a config schema at all).

    /core/themes/bartik/config/bartik.settings.yml
    ---
    # The config schema for this setting is not defined anywhere.
    # Additionally, because the key lives at the top-level, it is
    # inherently part of the 'theme_settings' schema itself.
    shortcut_module_link: true
    
  4. Anything in $theme.settings overrides the hard-coded default theme settings from .info.yml.

    bartik.info.yml:settingsconfig/bartik.settings.yml

  5. As soon as the theme is installed, the $theme.settings in the active config storage override the $theme.settings default config file AND the hard-coded default theme settings from .info.yml (for keys contained in $theme.settings).

    bartik.info.yml:settingsconfig/bartik.settings.ymlCONFIG_ACTIVE/bartik.settings

    1. The $theme.settings active config is only saved when theme settings are saved via the admin Appearance UI.
    2. If there is no $theme.settings config (yet), then a themer can simply adjust the settings in .info.yml, clear caches, and they become effective.
    3. Nothing happens at all if the $theme.settings active config exists.
  6. Additional module options in $theme.settings are not properly maintained and not cleaned up when e.g. the module is uninstalled.

    CONFIG_ACTIVE/bartik.settings.yml
    ---
    # Will never vanish, even if obsolete.
    shortcut_module_link: true
    
  7. The theme system itself has no Schema API and no update system; if a sub-theme contains config options of a base theme in $theme.settings, and in case the base theme config schema changes, there's no controlled way for either of both themes to update $basetheme.settings or $subtheme.settings (of all subthemes).

    Omega config schema 2014 Omega config schema 2015
    layout:
      type: integer
      label: 'Layout template ID'
    experimental:
      type: boolean
      label: 'Enable experimental fluff-huff'
    
    layout:
      type: string
      label: 'Layout template name'
    fancy_new:
      type: boolean
      label: 'Enable fancy new'
    
    Diff Diff Consequences
     layout:
    -  type: integer
    -  label: 'Layout template ID'
    +  type: string
    +  label: 'Layout template name'
    -experimental:
    -  type: boolean
    -  label: 'Enable experimental fluff-huff'
    +fancy_new:
    +  type: boolean
    +  label: 'Enable fancy new'
    
    …
    → (STORED) DATA TYPE/VALUE MISMATCH
    …
    …
    …
    → ORPHAN DATA (→ fancy_new?)
    …
    …
    → MISSING DEFAULT VALUE
    …
    …
    

    In words: New settings, data type changes, removed settings.

Files: 
CommentFileSizeAuthor
#50 2235901.50.patch14.36 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,717 pass(es). View
#50 47-50-interdiff.txt367 bytesalexpott
#47 2235901.47.patch14.39 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,717 pass(es). View
#47 40-47-interdiff.txt2.28 KBalexpott
#40 2235901.40.patch15.75 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,716 pass(es). View
#40 38-40-interdiff.txt2.98 KBalexpott
#38 2235901.38.patch15.71 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,744 pass(es). View
#38 33-38-interdiff.txt669 bytesalexpott
#33 2235901.32.patch15.29 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,738 pass(es). View
#33 30-test-fail-fix.txt1.28 KBalexpott
#33 30-32-interdiff.txt6.54 KBalexpott
#30 2235901.30.patch10.46 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,712 pass(es), 19 fail(s), and 5 exception(s). View
#17 custom-theme-settings-2235901-17.patch1.84 KBiMiksu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,101 pass(es), 13 fail(s), and 0 exception(s). View
#12 custom-theme-settings-2235901-12.patch1.57 KBmdrummond
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,096 pass(es), 16 fail(s), and 0 exception(s). View

Comments

sun’s picture

Issue summary: View changes
Jeff Burnz’s picture

I've been working with this "system" the past few months and here are some thoughts:

1) I'd like to see the idea of have theme setting defaults in info.yml file go away, to my mind this is what $theme.settings.yml is for. So #1, #2, #4 and #5 are not really flaws IMO, but rather one option too many (settings defaults in info files).

So, you set theme setting defaults in $theme.settings.yml and they're overridden by the active store. IMO this is fine, the defaults are there for when you first enable the theme, thereafter the user changes something/saves and those settings are saved in the active store, so it makes sense they're overridden in this fashion.

2) #3 I'm not sure that is a flaw, it's just the way it is afaict, you can get the config from the active storage and read it.

3) #6 and #7 are 100% flaws, agreed.

There is another serious flaw in the system - custom theme settings are not saved to the active store. Take a look at theme_settings_convert_to_config(), it's totally hard coded to convert only toggle settings (features) etc. For Adaptivetheme D8 I had to set these myself i.e.

  public function settingsConvertToConfig(array $values, Config $config) {
    $config = \Drupal::config($values['config_key']);
    foreach ($values as $key => $value) {
      if (substr($key, 0, 9) == 'settings_') {
        $config->set('settings.' . drupal_substr($key, 9), $value);
      }
    }
    return $config;
  }

call it from submit function etc:

  $config = \Drupal::config($theme . '.settings');
  $convertToConfig = new ThemeSettingsConfig();
  $convertToConfig->settingsConvertToConfig($values, $config)->save();

It's been a while since I looked at much internals in core around this, but it is/was totally flawed when I had to write this :/

On another point but somewhat related because of the "features" in particular the toggle settings - personally these are all from a bygone era and have no place in theme settings at all. The idea that you want to globally enable user pictures in posts is rather absurd. Logo, site name and slogan are a "branding" block now, as will be Main and Secondary menus etc. Logo and favicon upload are good, fine, the rest should be removed and farmed out to other parts of the system, for example user picture should be a setting in the content type alongside "Display author and date information" in the content type settings, not a theme setting.

sun’s picture

Priority: Major » Critical
Issue summary: View changes
sun’s picture

Issue summary: View changes
sun’s picture

Issue summary: View changes
mdrummond’s picture

If it reduces one of the conflicts, I think it makes a lot of sense to move the default theme settings to $theme.settings.yml. Because it's in the config install folder, it's intuitive that these are only default settings, so that people don't expect they can make changes to the setting in code.

#2271529: Move breakpoint settings to theme and module *.breakpoints.yml files at root level is looking to move breakpoint definitions out of config files and into *.info.yml files, but my understanding is that doesn't necessarily run into the same issues as theme settings.

catch’s picture

Issue tags: +beta target, +D8 upgrade path
mdrummond’s picture

It looks to me like #6 and #7 would likely be addressed in #2232605: Themes cannot be uninstalled. So probably the main thing to do here (or in a sub-issue) is to remove custom theme settings from *.info.yml.

For what it's worth, when I shared at a camp that settings would likely be removed from *.info.yml and changes could only be made via UI after a theme is installed, I heard a few grumbles from people that would prefer to be able to make those changes via code. But given the way configuration works, I don't see a better way that provides consistency and reliability.

hass’s picture

I have already filed an issue in D6/D7 days that theme settings required update hooks. I also have a lot of dynamic theme settings that require PHP code to build the config form. This worked all in D6/D7, but I have not yet tried porting to D8. I hope to not see any regressions.

mdrummond’s picture

Title: Theme settings (configuration) architecture is seriously flawed » Remove custom theme settings from *.info.yml
Issue summary: View changes

Rewriting the issue title and summary to reflect that part of the original issue report is handled by #2232605: Themes cannot be uninstalled, and this issue only needs a narrower scope.

mdrummond’s picture

Status: Active » Needs review
FileSize
1.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,096 pass(es), 16 fail(s), and 0 exception(s). View

Here's a patch that removes the custom theme settings from info.yml. The @todo indicates there is no UI for this setting, though, so that might need to be handled in another issue.

Status: Needs review » Needs work

The last submitted patch, 12: custom-theme-settings-2235901-12.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: custom-theme-settings-2235901-12.patch, failed testing.

iMiksu’s picture

Assigned: Unassigned » iMiksu

According to testing failures, it looks like there's problems with shortcut link and some missing schema.

iMiksu’s picture

FileSize
1.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,101 pass(es), 13 fail(s), and 0 exception(s). View

I've added schema entry of the shortcut_module_link setting to seven theme which caused "Missing schema" failures.

mdrummond’s picture

Status: Needs work » Needs review
sun’s picture

Remove custom theme settings from *.info.yml and rely solely upon *.settings.yml and active config.

This means that you can only change theme settings via the Appearance UI or Drush. Many themers won't be happy about that.

The abstract bottom line is that we're trying to cater for two vastly different audiences at the same time: (1) Authors of custom themes, operating to 99.9% in code, whose ideal DX is to just hit the "Reload" button, versus (2) End-users/consumers of ready-made, configurable themes, who might not have access to code at all.

For many Drupal sites, the use-case of (2) doesn't even exist, and if it was easily possible, they'd even rip out the entire Appearance UI, because their site's front-end ought to be controlled by code, not configuration.

Status: Needs review » Needs work

The last submitted patch, 17: custom-theme-settings-2235901-17.patch, failed testing.

iMiksu’s picture

Assigned: iMiksu » Unassigned

I think that use-case (1) is something that configuration management should cover. If you want to control over code you should use Configuration API for it.

I wonder do we actually need separate configuration interface for theme settings?

However, I got couple failures less in tests, but the proposed change would still need work in terms of making shortcut module detect the setting properly.

mdrummond’s picture

If you wanted control of custom settings to be solely in code, we could have an approach similar to #2271529: Move breakpoint settings to theme and module *.breakpoints.yml files at root level where some sort of plugin is used to handle the settings.

I think it's important not to underestimate the importance of UI for site builders. Themers or theme developers might prefer changes in code, but for a large number of site builders, they appreciate having a UI to change these settings.

So when UI is used to change these settings, the values of those settings go into the Active Store, which is the db. What would be helpful is if those changes were also written to code-based settings files in a theme, and if when cache was cleared, those code-based settings files were re-evaluated, and if their values were different from the active store, the code-based values would replace the db-based values.

That sounds to me like a rather large change, but something along those lines would greatly improve the experience of working with configuration, as everyone could win then, those who prefer using a UI and those who prefer code-based changes.

I have no idea how to build something like that though.

mdrummond’s picture

What I described above may seem like a longshot, but the possibilities just got far more interesting about why it would be great to have configuration for theme settings available and configurable both in a YML file within a theme as well as in the UI.

This article describes how YML can be used within Sass and Compass: http://fabbrucci.me/2014/08/integrate-yaml-in-your-sass-through-compass/

This means that if custom theme settings were stored within YML within a theme folder, as well as within the DB, the *.settings.yml could serve as a bridge between Drupal's UI and Sass-defined styles.

That opens up pretty amazing possibilities. You could then set up theme UI to allow site builders to control certain aspects of the design like fonts and colors or a wide range of design choices. You'd still need to work out a way to trigger a recompile of Sass when such a change was made, but it seems like a pretty exciting possibility.

Making that work is an entirely different matter. However, it does make me more interested in pursuing a solution where there could be *.settings.yml file at the root of a theme directory. If a change is made in the settings.yml file, it gets picked up when cache is cleared. If a change is made in the UI, it gets saved to the active store (DB) and to the settings.yml file.

Is something like that theoretically possible?

endorn’s picture

As a theme developer that has experience working in competing CMS platforms, allow me to give some feedback:

I'm absolutely flabbergasted that D8 was allowed to hit beta with this issue still lingering. There is no good way to develop a theme for D8 right now.

When I make a change to the info.yml file, I expect it to show up immediately. Just like when I change html, or css. Theme yml files are like 20 lines of code, so I don't understand why caching them is so important in the first place. That being said, at a MINIMUM reload the yml changes when I click the clear cache button, or at least give me another button I can push somewhere to reload info.yml files.

This is beyond a critical issue.

Jeff Burnz’s picture

@25 I'm not clear on what you are talking about here with regards to the Issue Summary and the proposed resolutions. For example you are not being clear about what you are changing in your info file, we already know settings won't update the active store, however Drupal provides a way to do this using the import/export system.

alexpott’s picture

re #24

That being said, at a MINIMUM reload the yml changes when I click the clear cache button

Afaik this is exactly what happens unless you've overridden the setting in the appearance UI.

alexpott’s picture

Theme settings come from (see theme_get_setting()):

  • The theme's info.yml
  • Any base theme's info.yml file
  • system.theme.global configuration
  • The theme's *.settings configuration

Complicating this further theme's can limit the features they support using a features key in their info.yml file. This is not inherited from any base themes. The default features are supplied by _system_default_theme_features(). Themes can also add new features by adding whatever they like to the features key in their info.yml file.

I think that the only practical way forward here is to remove the additional ability to get settings for a theme and its base themes info.yml file. Theme settings are configuration. Defaults should be supplied in config files and if people need command line access to change this they can use Drush's excellent config tools (eg. config-edit).

Less functionality is more! We have consistency - all install time defaults are supplied in config (for themes and modules) - we have less to do at runtime and theme_get_settings is simpler. All it does is merge system.theme.global and THEME.settings configurations.

webchick’s picture

That would have the unfortunate side-effect of making theme authors write config.schema.yml files for their themes (if they want their theme settings to be translatable like the rest of Drupal), is that correct?

Not saying necessarily that's a reason to NOT have consistency in the entire configuration system (which was rather the point of the configuration system in the first place :)).... just, ouch. :(

alexpott’s picture

@webchick #28 - so I'm not sure that writing this information into the theme's info.yml file is any easier. And with CMI you get an added bonus... configure everything through the appearance UI, export your configuration, and copy the theme's settings file to the theme's config/install folder.

alexpott’s picture

Status: Needs work » Needs review
FileSize
10.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,712 pass(es), 19 fail(s), and 5 exception(s). View

Here's a patch that removes the ability to define settings from the theme's info.yml file. Now all the theme settings are controlled by configuration which makes it much easier to work out what is going on. Basically the theme's setting key in their info.yml was default configuration before we have default configuration. Yes we lose the inheritability from base themes but in that instance - just copy the base theme's settings.yml and rename it.

This patch also addresses the schema situation by adding a third_party_settings key in the same that we have done for many configuration entities.

theme_get_setting() documentation is also updated to remove an erroneous claim that was not true before this patch.

Status: Needs review » Needs work

The last submitted patch, 30: 2235901.30.patch, failed testing.

dawehner’s picture

One thing we should/could recommend theme developers is the use of https://www.drupal.org/project/config_devel which already allows to
reimport config changes automatically. So consistency actually helps people again.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.54 KB
1.28 KB
15.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,738 pass(es). View

So the test fails were just to prove I was wrong about this...

theme_get_setting() documentation is also updated to remove an erroneous claim that was not true before this patch.

Included an interdiff for just the part that was necessary to fix the above fails.

The patch also adds an implementation of hook_config_schema_info_alter() to the system module so that theme that don't have any custom theme settings of their own don't have to have a config schema file.

Gábor Hojtsy’s picture

@alexpott: that is a neat trick so long as there are no translatable settings (ones that would need to be statically figured out). Looked and theme_settings does not seem to have any translatable strings, but it would get *bad* if it get one or more.

alexpott’s picture

So we could

  • Remove this and require themes to always have a schema - since they can always have settings
  • Add a comment to core.data_types.schema.yml next to theme_settings informing people to not add a translatable setting here
  • Or inform anyone building a statuic figurer outer that if it is looking at a theme's *.settings config object then to use theme_settings unless theme provides a schema

I think I feel like we should go for the last option but also add a comment to core.data_types.schema.yml to make this easier to discover.

Gábor Hojtsy’s picture

We don't have such magic elsewhere and one-off magic at one place may be confusing IMHO. This may set a dangerous precedent if we don't document it strongly (and even then). Anyway, we can move to special case this in the static parser (which is probably not trivial given it works with the assumption that everything can be described by the schema and *.xyz is not describable in schema), but I don't think there is a hurry there unless for some reason we want to keep this AND introduce a translatable setting.

Wim Leers’s picture

#27++

Only base themes and contrib themes do this; custom themes don't do this, they just hardcode the exact features they need, they don't make it configurable, because they don't need to. So the TX impact is lower than you'd think initially.

alexpott’s picture

FileSize
669 bytes
15.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,744 pass(es). View

Added comment to address #34 and #36. I think we could argue that theme creation is a different activity to most other activities with Drupal. Reducing the barrier to entry to theming is important for encouraging people to Drupal. Making new contributors are a boilerplate schema like:

my_first_theme.settings:
  type: theme_settings
  label: 'My first theme settings'

Could be off putting and seem pointless. Obviously if we provided a code generation tool then this would go away - but I don't think we'll have that for Drupal 8 (hopefully 8.1 or 8.2). OTOH making themers add this will make them aware of how this works. I could be convinced either way.

Wim Leers’s picture

  1. +++ b/core/config/schema/core.data_types.schema.yml
    @@ -120,6 +120,10 @@ action_configuration_default:
    +# Theme settings schema is automatically applied to a theme's *.settings
    +# configuration unless the theme provides its own schema. Static analysis of
    +# configuration objects using schema will have to duplicate the
    +# system_config_schema_info_alter() to determine a theme's *.settings schema.
    

    Nit: this reads to me as if we can have *.settings config files. But perhaps that's just me.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -90,8 +90,9 @@ public function __construct(ConfigFactoryInterface $config_factory, StorageInter
    +    // If the extension provides configuration schema or is a theme clear the
    

    Nit: could use a comma before "clear" for legibility.

  3. +++ b/core/modules/shortcut/config/schema/shortcut.schema.yml
    @@ -10,3 +10,12 @@ shortcut.set.*:
    +      label: 'Add link'
    

    Nit: s/'Add link'/'"Add shortcut" link'/

    ?

  4. +++ b/core/modules/system/system.module
    @@ -1344,3 +1344,20 @@ function system_path_insert() {
    +  // provide its own schema already..
    

    Nit: s/.././

  5. +++ b/core/themes/seven/config/install/seven.settings.yml
    @@ -0,0 +1,3 @@
    +third_party_settings:
    +  shortcut:
    +    module_link: true
    

    Suppose this wasn't Seven, but a custom theme. And suppose we'd forget to define this setting. Then where would the setting come from? I don't see a default being defined anywhere?

    I'd expect the default to be defined in shortcut.schema.yml, but I don't think it's possible to define defaults in schemas, so alas.

    EDIT: I guess this is the one remaining task as per the IS:

    - Make sure that custom settings still have defaults when they are only in *.settings.yml and not also in *.info.yml.

alexpott’s picture

Issue summary: View changes
FileSize
2.98 KB
15.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,716 pass(es). View

Thanks for the review @Wim Leers.

  1. Fixed
  2. Fixed
  3. Fixed
  4. Fixed
  5. The default value is always NULL see theme_get_setting(). I'm not sure this is important. Since there are no settings in info.yml the only place for defaults is in the settings.yml. Tentatively removing that part of the issue summary.
Wim Leers’s picture

Issue tags: +Needs change record

So this still needs a change record, but thanks to the IS update of #40, it looks like this is considered done? Is all that's still needed here just manual testing (and perhaps more code reviews)?

Gábor Hojtsy’s picture

I agree we would ideally make theme creation as easy as possible. I remain cautious and just wanted to make sure we understand the applicability of this "trick" only as long as a theme does not have translatable settings and/or the base theme system does not have translatable settings on its own. So I think this is an ok step for now because we want to make theme creation easier not because this sounds like a useful general approach.

Wim Leers’s picture

Status: Needs review » Needs work

NW for the CR, looks like #42 also has no remaining complains to delay RTBC any longer.

alexpott’s picture

I've written the CR https://www.drupal.org/node/2382645.

I think we have an issue with the theme settings form. See the following code in ThemeSettingsForm::buildForm()

      // Process the theme and all its base themes.
      foreach ($theme_keys as $theme) {
        // Include the theme-settings.php file.
        $filename = DRUPAL_ROOT . '/' . $themes[$theme]->getPath() . '/theme-settings.php';
        if (file_exists($filename)) {
          require_once $filename;
        }

        // Call theme-specific settings.
        $function = $theme . '_form_system_theme_settings_alter';
        if (function_exists($function)) {
          $function($form, $form_state);
        }
      }

I think that code means that we need to inherit the schema from base themes.

Wim Leers’s picture

Issue tags: -Needs change record

CR looks good. I just added some formatting to it.

Can we defer #44 to a follow-up?

alexpott’s picture

re #45 - not sure. We've removed the inheritability of the defaults from the base theme's info.yml file in this patch. Which is similar to this. I guess what we could do is remove the automatic schema from this patch - and have a new issue to discuss that and its implications.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.28 KB
14.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,717 pass(es). View

Patch without automatic schema.

alexpott’s picture

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Besides a tiny mistake which could be fixed on commit, I think this is RTBC then.

+++ b/core/themes/seven/config/schema/seven.schema.yml
@@ -0,0 +1,5 @@
+  label: 'Bartik settings'

s/Bartik/Seven/

alexpott’s picture

FileSize
367 bytes
14.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,717 pass(es). View

Doh!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thank you!

  • Dries committed dbde59f on
    Issue #2235901 by alexpott, mdrummond, iMiksu, sun, Wim Leers: Remove...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.