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

  1. Install standard
  2. Override a theme setting in settings.php for example $config['system.theme.global']['favicon']['use_default'] = FALSE;
  3. Go to /admin/appearance/settings - the override will change the form value - which it should not.
  4. 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

  1. 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.
  2. 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.
  3. 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

Comments

catch’s picture

Is this still valid?

alexpott’s picture

yes

joelpittet’s picture

Status: Active » Postponed (maintainer needs more info)
Issue tags: +CMI

What is the proposed solution here?

joelpittet’s picture

Issue summary: View changes
Issue tags: +Needs steps to reproduce

This needs an example or steps to reproduce.

alexpott’s picture

Issue summary: View changes
Status: Postponed (maintainer needs more info) » Needs work
Issue tags: -Needs steps to reproduce

Added steps to reproduce.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

Issue tags: +Triaged D8 major

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Neograph734’s picture

Status: Needs work » Needs review
Related issues: +#2866194: Theme specific configurations always override the global configuration

Switching 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():

      $form['logo']['settings']['logo_path'] = [
        '#type' => 'textfield',
        '#title' => t('Path to custom logo'),
        '#default_value' => theme_get_setting('logo.path', $theme),
      ];

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?

function theme_get_setting($setting_name, $theme = NULL, $allow_overrides = TRUE) {

  ...

  if ($allow_overrides) {
    $cache[$theme]->setData(\Drupal::config('system.theme.global')->get());
  }
  else {
    $cache[$theme]->setData(\Drupal::configFactory()->getEditable('system.theme.global')->get());
  }
  
  ...
  
  // Retrieve configured theme-specific settings, if any.
  try {
    if ($allow_overrides) {
      if ($theme_settings = \Drupal::config($theme . '.settings')->get()) {
        $cache[$theme]->merge($theme_settings);
      }
    }
    else {
      if ($theme_settings = \Drupal::configFactory()->getEditable($theme . '.settings')->get()) {
        $cache[$theme]->merge($theme_settings);
      }
    }
  }

}

And then in ThemeSettingsForm do something like this:

      $form['logo']['settings']['logo_path'] = [
        '#type' => 'textfield',
        '#title' => t('Path to custom logo'),
        '#default_value' => theme_get_setting('logo.path', $theme, FALSE),
      ];

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.

joelpittet’s picture

I've got a couple of proposals:

  1. Leave the override value in the form, disable the field, add a description note about the value overridden, ensure the field doesn't get saved. (I think @alexpott was not liking this solution from other things I've read about this, correct me if I'm wrong)
  2. OR Along the lines of @Neograph734 in #10, extend the API of that 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.

Neograph734’s picture

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

Neograph734’s picture

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

Status: Needs review » Needs work

The last submitted patch, 13: prevent_theme_settings-bleed-2402467-13.patch, failed testing.

Neograph734’s picture

Ok, 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?

Neograph734’s picture

Let's see what happens if we supply some Bartik defaults.

Status: Needs review » Needs work

The last submitted patch, 16: prevent_theme_settings-bleed-2402467-16.patch, failed testing.

Neograph734’s picture

Hmm, 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.

The last submitted patch, 18: prevent_theme_settings-bleed-2402467-18-updated-test.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 18: prevent_theme_settings-bleed-2402467-18.patch, failed testing.

Neograph734’s picture

Issue summary: View changes

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