Core expects $theme.settings default config file to exist

distro.theme-settings.0.patch queued for re-testing.

distro.theme-settings.0.patch queued for re-testing.

distro.theme-settings.0.patch queued for re-testing.

I guess this issue is essentially "blocked" on uncertainty/disagreement about the second change described in the issue summary...

To clarify, I'm not 100% sure on that myself — all I'm sure about is that the current config schema definitions do not make sense at all ;-) because:

1. theme_settings_default is declared by the core schema (next to other theme_ config schemata).

2. But that theme_settings_default declaration only contains a definition for the optional Shortcut module (in the core schema!)

3. The actual schema for $theme.settings config files is defined in system.theme.global of System module.

4. The config schema declarations of the actual themes are pointing to theme_settings_default instead of system.theme.global

   → the $theme.settings configurations are completely lacking a schema right now.

Now, this patch fixes the faulty config schema declarations and references.

The only part that I'm not sure about myself is whether the theme_settings config schema declaration belongs to core, or whether it belongs to System module...

This patch makes core the owner of the theme_settings config schema declaration, based on the idea that a theme is an independent extension and not tied to System module in any way. System module just happens to expose a UI to configure theme settings.

Does that make sense? :-)

Clarified issue summary.

Re-rolled against HEAD.

13: drupal8.theme-settings.13.patch queued for re-testing.

#1067408: Themes do not have an installation status introduces some very basic coverage, but technically that's even out of scope for the introduced DUTB test.

We have a web test that verifies different logo paths (focusing on file paths and stuff). But I don't think we have dedicated test coverage for just theme settings.

If we want to introduce test coverage for theme settings here, then (1) it should be a DUTB test (2) it should test nothing else than the expected functionality of theme_get_setting() (3) it should ideally not try to assert anything related to a theme's status.

That said, as soon as the parent issue lands, the installation status does play a key role with regard to theme settings, because a disabled theme should not have any settings... Without the parent issue, all themes are loaded at all times, so there is no difference between installed/enabled/disabled in HEAD right now.

So the only possible test coverage that I can see with HEAD right now is that theme_get_settings() (1) returns System module's default theme.settings.global configuration if you pass a non-existing theme name, and (2) that it merges the actual theme settings into the global default settings if you pass an existing theme name.

I'm suggesting DUTB instead of PHPUnit, because >95% of a PHPUnit test for this would consist of mocking only; barely testing the actual runtime behavior.

That said — strictly speaking, all of that is out of scope.

The only part that would be in scope for this issue would be a test that verifies that a $theme.settings file is not actually required.

This patch implicitly verifies that by removing various $theme.settings files.

Explicit test coverage for this would be a very narrow DUTB test — essentially:

  function testThemeSettingsDefaultConfigNotRequired() {
    $name = 'test_base_theme';
    $this->assertFalse(file_exists(drupal_get_path('theme', $name) . "/config/$name.settings.yml"));
    $this->container->get('theme_handler')->enable(array($name));
    $this->assertTrue(theme_get_setting('features.favicon', $name));
  }

The last line blows up without this patch.

1. Added ThemeSettingsTest.
2. Added counter-test for a theme with $theme.settings default config.

The default config for test_basetheme is copied/extracted literally from #1067408: Themes do not have an installation status

Oh, hah, obviously, taking it over literally doesn't account for the config schema type change here.

→ Updated config schema type to theme_settings. ;-)

Ugh.

Restored bartik.settings (required for recent changes to ConfigImportUITest, which I don't want to futz with here).

Alright, this patch includes test coverage for the actual bug now (and the new test class can be enhanced later on).

Back to the re-roll in #25.

1. override: base:

   Sorry, my fault, that's copied literally from the parent issue... The additional test coverage over there asserts that theme settings of a base theme can be overridden by a sub-theme. We can remove it here, but we'll add it anyway...?

2. bartik.settings.yml + shortcut_module_link:

   As mentioned in #23, the new/revised ConfigImportUITest needs that to assert that a theme has been correctly installed and uninstalled when performing a config import. I did not want to change that test, because the visual confirmation of Stark vs. Bartik in the test's verbose browser output helps to manually confirm that the functionality works as expected.

   Every configuration file needs a config schema - which is covered by DefaultConfigTest. Therefore, bartik.settings.yml cannot exist and have a shortcut_module_link key without a corresponding config schema.

   FWIW, the current theme settings design is seriously flawed in multiple ways:

   1. Hard-coded default theme settings are specified in a theme's .info.yml file.
   2. If a theme supplies a $theme.settings file, then it has to manually specify the (always identical) config schema for it.
   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).
   4. Anything in $theme.settings overrides the hard-coded default theme settings from .info.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).
   6. Additional module options in $theme.settings are not properly maintained and not cleaned up when e.g. the module is uninstalled.
   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 $theme.settings.

   In essence, this patch here only duct-tapes the situation for now. This entire design needs complete rethinking.

Created #2235901: Remove custom theme settings from *.info.yml for #34.2, since that is not to be addressed here.

Sure :)

Thanks!