olivero.settings config schema is wrong

Surprised that removing that cause all those failures. Is it a dependency thing where those other schemas aren't getting loaded?

Ah good find!

What about existing sites? Do we need to clean up if Olivero is installed but Shortcut is not?

You are correct. Did a standard install, uninstalled shortcut, config export and still see that third_party_setting in the yml file.

_core:
  default_config_hash: 1TswGK46jyu77aIM7Z-0JVQs5bxHmo-gtgrvrQGMXxc
favicon:
  use_default: true
features:
  comment_user_picture: true
  comment_user_verification: true
  favicon: true
  node_user_picture: false
logo:
  use_default: false
third_party_settings:
  shortcut:
    module_link: true
mobile_menu_all_widths: 0
site_branding_bg_color: default
base_primary_color: '#1b9ae4'

Postponing this on #3395555: Add validation constraints to olivero.settings, which adds more validation.

This bug is a bit annoying.

Contrib (e.g. Gin) is now copying this bug into their own codebase, just to make Config inspector shut up. And this again breaks theme 3rd party settings for everybody else :(

Issue itr was postponed on was fixed, this can actually be worked on.

Added an update hook and test, also tested it locally on a clean install.

So tested this one with 2 separate sites, 1 I had shortcuts installed the other I did not

First site
> Shortcut remained

Second site
> Shortcut was removed

Problem though, on the site that remained the validation went down because the third_party_settings schema is gone.

What was the message there?

The validation should still be OK, since:

olivero.settings:
  type: theme_settings

Which means core.data_types.schema.yml

theme_settings:
  type: config_object
  mapping:
...
    third_party_settings:
      # Third party settings are always optional: they're an optional extension
      # point.
      requiredKey: false
      type: sequence
      label: 'Third party settings'
      sequence:
        type: theme_settings.third_party.[%key]

Which then should resolve into:

shortcut.schema.yml

# Schema for theme settings.
theme_settings.third_party.shortcut:
  type: mapping
  label: 'Shortcut settings'
  mapping:
    module_link:
      type: boolean
      label: 'Add shortcut link'

I also tried to proove the issue in the test, but seems fine. I'm confused.

Don't have that branch up right now but validation had gone down for me because third_party_settings was no longer validated

Can you help me reproduce locally?

So I apply the MR and run the update when I go to olivero.settings

I see that the percentage went down to 94% and

Ahh, there.

Hmm, ok, seems weird but can look into that. I'd assume that is validated since it is not overwritten for the theme_settings type.

Thanks, I cab work with this.

I should confess i first read your message as the validation crashed, but you mean it is not 100% :)

This goed a little deeper. I dont see any third_party_settings validated in other config, only the actualyl settings when set, so this seems expected. There is also;

#3087036: third_party_settings in THEME.settings.yml cannot be dependency managed
#3016895: 3rd party should be allowed to add config dependencies

Which expose some issues around third part settings. So i don't think we should block on the fact the parent key is not validated.

Alright that was the only issue I found

This is going to change what is set up when you install olivero with shortcuts already enabled. This must change what happens when you install standard.

I think we need to go a little further and introduce a little bit of cleverness into the theme installer. Something along the lines of:

diff --git a/core/lib/Drupal/Core/Extension/ThemeInstaller.php b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
index 364d672c12f..c99f6a07a30 100644
--- a/core/lib/Drupal/Core/Extension/ThemeInstaller.php
+++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
@@ -240,6 +240,21 @@ public function install(array $theme_list, $install_dependencies = TRUE) {
       if (!isset($installed_themes[$key])) {
         // Install default configuration of the theme.
         $this->configInstaller->installDefaultConfig('theme', $key);
+
+        // Remove any third party settings for modules that are not installed.
+        $theme_config = $this->configFactory->getEditable($key . '.settings');
+        if (!$theme_config->isNew()) {
+          $theme_config_changed = FALSE;
+          foreach ($theme_config->get('third_party_settings') ?? [] as $module => $third_party_settings) {
+            if (!$this->moduleHandler->moduleExists($module)) {
+              $theme_config->clear("third_party_settings.$module");
+              $theme_config_changed = TRUE;
+            }
+          }
+          if (!$theme_config_changed) {
+            $theme_config->save();
+          }
+        }
       }
 
       $themes_installed[] = $key;

Perhaps more elegant:

        // Remove any third party settings for modules that are not installed.
        $theme_config = $this->configFactory->getEditable($key . '.settings');
        if (!$theme_config->isNew()) {
          $third_party_settings = array_filter(
            $theme_config->get('third_party_settings') ?? [], 
            fn ($module) => !$this->moduleHandler->moduleExists($module), 
            ARRAY_FILTER_USE_KEY
          );
          if ($third_party_settings !== $theme_config->get('third_party_settings') ?? []) {
            $theme_config->set('third_party_settings', $third_party_settings)->save();
          }
        }

Seems like a good suggestion to add that.

Added the extra removal, and rebased while i was at is.

That isn't a random test failure. I verified the failure multiple times on my local environment. I didn't look into it, but it appears to have something to do with Olivero's third party settings. So it seems likely that it has something to do with the most recent changes. Because of that I didn't review those changes.

Fixed missing annotation.

Modifying a theme's configuration when it's installed seems like something we should have tests for. Otherwise I think the changes that resulted from the increase in scope from #25 look OK to me, with one minor bit of feedback.

We have OliveroPostUpdateTest which at least checks if removal is done properly.

What would you like to see tested? If other settings are kept? Not sure what you are asking more coverage for.

What would you like to see tested? If other settings are kept? Not sure what you are asking more coverage for.

Sorry, I was talking specifically about the change to ThemeInstaller. The update test looks good. But #25 caused the addition of new functionality. I think that a Kernel test that verifies third-party settings are removed/retained from a theme's settings when it's installed is necessary. I looked for an existing test for that, but didn't find one. I might have looked in the wrong places.

Remove 'Needs tests' tag. Coverage is in place.

Test-only output:

PHPUnit 11.5.39 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.4.12
Configuration: /builds/issue/drupal-3402885/core/phpunit.xml.dist
F.                                                                  2 / 2 (100%)
Time: 00:11.890, Memory: 8.00 MB
There was 1 failure:
1) Drupal\Tests\olivero\Functional\Update\OliveroPostUpdateTest::testOliveroThirdPartySettingsWithoutShortcut
Failed asserting that Array &0 [
    'shortcut' => Array &1 [
        'module_link' => true,
    ],
] is null.
/builds/issue/drupal-3402885/core/themes/olivero/tests/src/Functional/Update/OliveroPostUpdateTest.php:60
FAILURES!
Tests: 2, Assertions: 358, Failures: 1.
Exiting with EXIT_CODE=1

It seems like #32 has been addressed now? Setting to Needs review.

That was an update test. But there was functionality added to the ThemeInstaller that also needs to be tested.