Remove custom theme settings from *.info.yml

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.

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.

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.

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.

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.

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.

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

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

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.

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.

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?

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.

@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.

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.

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.

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. :(

@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.

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.

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.

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.

@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.

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.

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.

#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.

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.

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.

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.

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)?

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.

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

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.

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

Can we defer #44 to a follow-up?

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.

Patch without automatic schema.

Created #2382671: Automatically define schema for themes

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/

Doh!

Committed to 8.x. Thank you!