Problem

  1. Create a new theme by creating a new .info.yml file.
  2. Enable it.
  3. You're greeted by a fatal error:

    "mytheme.settings config file does not exist."

Cause

  1. theme_get_setting() expects that each theme has a $theme.settings default config file.
  2. That does not make sense, because $theme.settings contains custom, user-configured settings only.
  3. A theme does not have to provide any custom settings.

Proposed solution

  1. Remove the bogus requirement.
  2. While being there, also fix the bogus config schema declaration for theme settings:
    1. Core declares theme_settings_default, but that contains a schema for the optional Shortcut module only.
    2. System module declares system.theme.global that contains the actual schema for theme settings, but that is used by System module only.
    3. The individual themes are referencing theme_settings_default in their config schema; i.e., their config schema only knows about the aforementioned, bogus Shortcut module declaration.
Files: 
CommentFileSizeAuthor
#36 interdiff-25-36.txt1.71 KBeffulgentsia
#36 theme.settings.36.patch12.33 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,649 pass(es). View
#25 theme.settings.25.patch11.85 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,619 pass(es). View
#23 interdiff.txt704 bytessun
#23 theme.settings.23.patch11.85 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,554 pass(es). View
#20 interdiff.txt564 bytessun
#20 theme.settings.20.patch12.02 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,412 pass(es), 0 fail(s), and 1 exception(s). View
#19 interdiff.txt3.18 KBsun
#19 theme.settings.19.patch12.02 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,487 pass(es), 0 fail(s), and 1 exception(s). View
#13 drupal8.theme-settings.13.patch8.84 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,007 pass(es). View
distro.theme-settings.0.patch8.95 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch distro.theme-settings.0.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

sun’s picture

Issue tags: +Sunrise Sanity Cruise
sun’s picture

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

sun’s picture

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

sun’s picture

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

sun’s picture

sun’s picture

jibran’s picture

I don't agree with #2 in issue summary but other then that it is simple code moving form one place to other let's see what @alexpott thinks about it?
PS: Is "Sunrise Sanity Cruise" still a thing?

sun’s picture

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

jessebeach’s picture

Priority: Major » Critical
Issue tags: +beta blocker

Elevated status because it blocks #2232605: Themes cannot be uninstalled.

sun’s picture

Issue summary: View changes

Clarified issue summary.

effulgentsia’s picture

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

Status: Needs review » Needs work

The last submitted patch, distro.theme-settings.0.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
8.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,007 pass(es). View

Re-rolled against HEAD.

Status: Needs review » Needs work

The last submitted patch, 13: drupal8.theme-settings.13.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
jessebeach’s picture

I can't find any tests that cover theme settings. Like simply toggle the logo setting. sun, do you know if this exsits offhand? If not, I'll write the coverage.

Otherwise the code looks good and all existing tests pass. As long as the coverage for settings is in place and passes, then let's get this in.

sun’s picture

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

sun’s picture

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.

sun’s picture

FileSize
12.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,487 pass(es), 0 fail(s), and 1 exception(s). View
3.18 KB
  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

sun’s picture

FileSize
12.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,412 pass(es), 0 fail(s), and 1 exception(s). View
564 bytes

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

→ Updated config schema type to theme_settings. ;-)

Status: Needs review » Needs work

The last submitted patch, 20: theme.settings.20.patch, failed testing.

The last submitted patch, 19: theme.settings.19.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
11.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,554 pass(es). View
704 bytes

Ugh.

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

sun’s picture

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

effulgentsia’s picture

FileSize
11.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,619 pass(es). View

Reroll.

Status: Needs review » Needs work

The last submitted patch, 25: theme.settings.25.patch, failed testing.

effulgentsia’s picture

Patch is looking close to ready. My only questions are:

  1. +++ b/core/modules/system/tests/themes/test_basetheme/config/test_basetheme.settings.yml
    @@ -0,0 +1,4 @@
    +override: base
    

    Is this being used by anything?

  2. +++ b/core/themes/bartik/config/schema/bartik.schema.yml
    @@ -1,5 +1,9 @@
    +  mapping:
    +    shortcut_module_link:
    +      type: boolean
    +      label: 'Shortcut module link'
    

    Looks like we removed this from Stark and Seven, which makes sense as those shouldn't be configurable. But why are we keeping it in Bartik? There doesn't appear to be a UI for configuring it in that theme either.

effulgentsia’s picture

Status: Needs work » Needs review

25: theme.settings.25.patch queued for re-testing.

xjm’s picture

Issue tags: +Configuration system
jessebeach’s picture

FileSize
12.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,571 pass(es), 0 fail(s), and 1 exception(s). View
1.54 KB
+++ b/core/themes/bartik/config/schema/bartik.schema.yml
@@ -1,5 +1,9 @@
+  mapping:
+    shortcut_module_link:
+      type: boolean
+      label: 'Shortcut module link
'

Looks like we removed this from Stark and Seven, which makes sense as those shouldn't be configurable. But why are we keeping it in Bartik? There doesn't appear to be a UI for configuring it in that theme either.

The shortcut_module_link setting is only set to true in Seven, so we don't need mention of it in Bartik. I've sorted these various settings, see the interdiff.

effulgentsia, we should have a way to expose settings defined by a theme's schema. I've added an issue to track that. #2235387: Render schematized theme settings in the theme settings form.

I'm looking into the override: base comment.

jessebeach’s picture

As far as I can tell override: base is not used. It could be removed along with the corresponding scheme declaration. I'll wait for sun to respond, though, because I might not be grepping for the right thing here.

jessebeach’s picture

Also, the tests in ThemeSettingsTest.php satisfy my concerns from #16. Thanks sun!

If #30 comes back green and given a response to the override: base comment, I think we're ready to commit this.

Status: Needs review » Needs work

The last submitted patch, 30: theme-settings-2218655-30.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

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.

sun’s picture

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

effulgentsia’s picture

FileSize
12.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,649 pass(es). View
1.71 KB

Thanks for opening that issue. How about this then? Interdiff relative to #25 attached. I'm happy with this, so if you approve the interdiff, this can be RTBC.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Sure :)

jessebeach’s picture

Neat. Progress obtains.

effulgentsia’s picture

+++ b/core/config/schema/core.data_types.schema.yml
@@ -101,12 +101,69 @@ action_configuration_default:
+    features:
+      type: mapping
+      label: 'Shortcut icon settings'
...
+    logo:
+      type: mapping
+      label: 'Shortcut icon settings'

In case the core committer reviewing this wonders why those labels make no sense, it's because this is a pre-existing bug in HEAD, and this patch just moves it from one file to another. After this is committed, we can fix the labels in #2236089: theme_settings schema duplicates the label "Shortcut icon settings" where it doesn't make sense.

xjm’s picture

Assigned: sun » alexpott

Let's have @alexpott take a look at this one.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for opening the two followups. When we converted theme settings to config I remember thinking the current architecture is a bit of a mess.

Committed 7f00c59 and pushed to 8.x. Thanks!

  • Commit 7f00c59 on 8.x by alexpott:
    Issue #2218655 by sun, effulgentsia, jessebeach: Core expects $theme....
sun’s picture

Assigned: alexpott » sun

Thanks!

Status: Fixed » Closed (fixed)

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