Unable to uninstall base theme and subtheme via config sync at the same time

8.4.x is unsupported.

I can still reproduce this. If you try to import config that uninstalls a base theme and sub theme at the same time, it throws an error.

But I'm not sure whether it's something to be fixed/supported or just need to update that comment to say they can't be uninstalled at the same time?

Just ran into this exact same problem that config sync cannot uninstall base theme and theme at same time, and looked to make sure an issue was already filed. This is clearly a pretty important cleanup of the configuration management system which is usually pretty solid, or at least works if you run it twice (that doesn't fix anything here).

Hi, Did you run drush updb or/update.php first? then try the drush cim or configuration import via UI.

The patch on #5 works for me, all it needs is a test for it to get merged in.

@iyyappan.govind, I think this isn't related to the cache and something fundamental in the implementation where it uninstalls the themes in alphabetical order rather than based on their dependency tree.

This also comes up for sites upgrading from Drupal 8 to Drupal 9 due to #3115223: Remove Stable as a base theme of core themes.

I wrote the following update hook to handle a site using Claro in Drupal 8, which had dependencies on Stable and Classy.

/**
 * Workaround for Drupal 9 uninstalling stable and classy.
 *
 * Claro in Drupal 8 depends on stable and classy, but no longer does in Drupal
 * 9. We import configuration before running post-update hooks, which means that
 * if we export the expected config after an update, it fails on deployments
 * because we can't remove stable and classy yet.
 *
 * @see https://www.drupal.org/project/drupal/issues/3001430
 */
function HOOK_update_8004() {
  /** @var \Drupal\Core\Extension\ThemeInstallerInterface $installer */
  $installer = \Drupal::service('theme_installer');
  // Install seven so we can change the default theme. It will be uninstalled
  // when configuration is next imported.
  $installer->install(['seven']);

  $config = \Drupal::configFactory()->getEditable('system.theme');
  $config->set('admin', 'seven');
  $config->save(TRUE);

  // Claro will be installed with the next configuration import.
  $installer->uninstall(['claro', 'classy', 'stable']);
}

The theme list actually has weights assigned by dependency - see \Drupal\Core\Extension\ThemeExtensionList::doList() - it calls into \Drupal\Core\Extension\ModuleHandlerInterface::buildModuleDependencies() and creates a graph ... so we can do the same as modules. I'm going to add a test to prove that we install and uninstall themes with base themes via config sync correctly.

Here's a test and a proper patch. #12 was the same as #5 oops...

So we now using the theme's sort property to sort them for installing and uninstalling. Plus I've got a test that proves the fix and the fact we have a problem.

The last 9.3.x bugfix release has gone out so moving to 9.4.x

Fixing the tests and injecting the theme extension list into the config importer.

Updating the issue summary.

Rerolled after #2392815: Module uninstall validators are not used to validate a config import

I removed the changes to \Drupal\Tests\config\Functional\ConfigImportUITest because they are no longer needed since the test is now using Olivero instead of Bartik.

Actually noticed this bug while prepping a site for D10

@trigger_error('Calling ' . __METHOD__ . ' without the $extension_list_theme argument is deprecated in drupal:9.4.0 and will be required in drupal:10.0.0

1. Should be updated for 10.1.x and 11 now I think.

Testing out the functionality by editing a contrib theme I'm using to base theme off stark.
Before installing I exported my config
Installed theme which installed stark
Ran config import and got the error

Applied fix
Ran config import and both uninstalled.

So this looks good just needs that 1 update.

Addressed the pointer in #23.
Please review.

Issue has been addressed

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -448,9 +463,24 @@ protected function createExtensionChangelist() {
+    // Set the actual module weights.

Should this say theme weights?

Also is there no API method for that anywhere?

Yes it should say theme weights... it's about setting them in the array we're creating to do the ordering... so it's not setting the theme weights in the system... will fix this to be clearer. It's a copy/pasta so fixing it above too.

Leaving at RTBC as this is only fixing a comment as @catch requested.

@alexpott found #2241855: Provide canonical extension sorting by dependencies in the extension system which covers my remaining concern here, which was apparently a remaining concern on a similar issue 9 years ago.

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!

I agree that we should fix this in 9.5 and 10.0 so here's a patch to remove the deprecation message as we should only be adding them in 10.1

Hmm fair enough, this is the tricky thing about best effort bc vs. backporting bugfixes with internal API changes. Committed/pushed to 10.0.x and 9.5.x, thanks! I don't think anyone's going to be overriding either class so hopefully this is moot.

Just a couple of micronits

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -166,6 +167,13 @@ class ConfigImporter {
   +  protected $themeExtensionList;
   
   +++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
   @@ -104,6 +105,13 @@ class ConfigSingleImportForm extends ConfirmFormBase {
   +  protected $themeExtensionList;
   
   +++ b/core/modules/config/src/Form/ConfigSync.php
   @@ -121,6 +122,13 @@ class ConfigSync extends FormBase {
   +  protected $themeExtensionList;
   

   Let's add a property type-hint here for the D10 version.

2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -448,9 +463,24 @@ protected function createExtensionChangelist() {
   +    // Set the actual module weights.
   

   %s/module/theme

3. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php
   @@ -687,6 +687,37 @@ public function testRequiredModuleValidation() {
   +   * Tests installing a base themes and sub themes during configuration import.
   

   %s/base themes/base theme

4. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php
   @@ -687,6 +687,37 @@ public function testRequiredModuleValidation() {
   +    $this->assertTrue($this->container->get('theme_handler')->themeExists('test_basetheme'));
   +    $this->assertTrue($this->container->get('theme_handler')->themeExists('test_subsubtheme'));
   +    $this->assertTrue($this->container->get('theme_handler')->themeExists('test_subtheme'));
   ...
   +    $this->assertFalse($this->container->get('theme_handler')->themeExists('test_basetheme'));
   +    $this->assertFalse($this->container->get('theme_handler')->themeExists('test_subsubtheme'));
   +    $this->assertFalse($this->container->get('theme_handler')->themeExists('test_subtheme'));
   

   micronit: is it worth putting the theme_handler service into a local variable ?

   would probably need to assign it twice to allow for any container rebuild as a result of updating core.extension

Ah crosspost

Published the change record.