Problem/Motivation

Followup to #1067408: Themes do not have an installation status. Themes now have an installation status but there is no way to uninstall them - we can only disable them. This puts a theme into limbo where the theme can not be used but configuration that is dependent on it is still in the active store. Like modules, themes must now clean up their installed configuration when they are uninstalled.

Proposed resolution

  • Make theme enabling and disabling the same as the module subsystem following #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed. In HEAD (and D7) themes are a two state system (disabled or enabled) - in some ways this patch just changes the system use the same language as the module system.
  • Remove associated configuration entities when a theme is uninstalled eg block configuration entities.
  • Replace enable/disable in the UI with install/uninstall. Since themes are a two state system this change does not really change any user interaction.

The effects of the consolidation of ThemeHandler and ModuleHandler operations can be see in the ConfigImporter - this patch has several simplifications to that class.

Remaining tasks

Review patch

User interface changes

  • UI text changed from enable/disable to install/uninstall
  • CSS classes changed from system-themes-list-enabled/disabled to system-themes-list-installed/uninstalled

API changes

  • ThemeHandler::enable() replaced with ThemeHandler::install()
  • ThemeHandler::disable() replaced with ThemeHandler::uninstall() and add call to ConfigManager to remove configuration and the theme's dependencies
  • Replace hook_themes_enabled() with hook_themes_installed()
  • Replace hook_themes_disabled() with hook_themes_uninstalled()
  • Remove disabled key from core.extension configuration
  • Route names are change to reflect to move to install/uninstall
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jessebeach’s picture

Title: Remove theme-specific configurations when a theme is uninstalled » [PP-2] Remove theme-specific configurations when a theme is uninstalled
sun’s picture

Title: [PP-2] Remove theme-specific configurations when a theme is uninstalled » [PP-2] Themes cannot be uninstalled
Category: Task » Bug report

This will actually have to introduce the uninstall() functionality itself.

The parent issue only introduces a system-wide notion of installed / enabled / disabled. It does not introduce an uninstallation facility.

In other words, once a theme has been installed, it stays installed. You can only disable it.

This issue needs to introduce a low-level ThemeHandler::uninstall() method + corresponding UI changes on the Appearance page.

sun’s picture

Title: [PP-2] Themes cannot be uninstalled » Themes cannot be uninstalled
Status: Postponed » Active
sun’s picture

Assigned: Unassigned » sun
Status: Active » Needs review
Issue tags: -D8MI, -D8 upgrade path
FileSize
4.14 KB

Baseline: API + tests.

sun’s picture

I'd like to split this issue into two:

  1. Adding the API + tests + agreeing on the low-level functionality. (this issue)
  2. Adding/changing the UI to add an "uninstall" operation. (new issue)

The patch in #4 is a ready + working implementation of ThemeHandler::uninstall(). Now, the interesting question is:

The implemented expectation is that you can only uninstall themes that are disabled. Will we run into hook invocation problems with that? (e.g., when uninstalling config)

The alternative expectation would be that you can only uninstall themes that are enabled (as opposed to disabled).

jessebeach’s picture

Can we simply get rid of the concept of "disabled" for themes? They are the aberrant extensions in this regard.

I'm happy to work on the UI bit for this.

Gábor Hojtsy’s picture

Disabled modules were removed as a concept exactly due to data model / hooks issues :) Looks like we don't want that happen again here?

sun’s picture

Sorry for the confusion; let me clarify:

Disabled modules were removed, because they involved data integrity problems — primarily because they have a database table schema, and in general, because they are storing data that may integrate with other modules. Horizontal extensibility.

Disabled themes do not share these aspects — at least not in any way that is problematic. They do not have an own database schema/storage, and they only support vertical extensibility/inheritance via parent/base themes, whereas parent theme(s) cannot be disabled if there are sub themes that depend on them.

I do not think we should or have to remove the concept of disabled themes.


At the same time, it would be worth to rethink what disabled themes are good for today, and how we can/want to resolve their use-cases, because the parent issue effectively destroyed all previous use-cases for disabled themes... e.g. not exposing (disabled) base themes in theme selection controls, not exposing the (disabled) admin theme as a theme, etc.pp.

sun’s picture

Created:

#2250153: Allow to uninstall a theme on the Appearance page
#2250155: Rethink how former use-cases for disabled themes can be supported in D8

The latter issue may or may not remove the concept of disabled themes, or re-implement it in a different way (note: disabled != losing your configuration), but that's off-topic here.

Unless there are objections to the current patch, I think we should simply move forward here.

jessebeach’s picture

Status: Needs review » Reviewed & tested by the community

The additional test coverage addresses the new cases. Tests pass and I have no issues with the code. Let's get this in so we can start working on the UI piece in #2250153: Allow to uninstall a theme on the Appearance page.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -333,6 +333,32 @@ public function disable(array $theme_list) {
+        throw new \InvalidArgumentException("Theme $key is still enabled.");

If you can't go straight from install to uninstall of a theme, how can you stage that as a configuration change? Either way I think we should add tests for that.

jessebeach’s picture

Title: Themes cannot be uninstalled » Themes do not clean up their associated configuration when uninstalled

It's a bit of a misnomer to say that themes cannot be uninstalled. They can be, it's just that any configuration associated with the theme is left in place.

sun’s picture

Title: Themes do not clean up their associated configuration when uninstalled » Themes cannot be uninstalled
Component: theme system » extension system

@jessebeach: There is no way to remove a theme entirely from the core.extension configuration currently. This means that there is no way to uninstall a theme extension. → "...when uninstalled" is a condition that does not exist in HEAD.

jessebeach’s picture

There is no way to remove a theme entirely from the core.extension configuration currently

Do you think we can just do this piece in isolation?

Also, what is the danger of stale configuration lying about? How often does one really install and uninstall a theme? I'm wondering if this issue is really a beta blocker or just a nice to have.

xjm’s picture

Issue tags: -beta blocker +beta target

Thanks for the issue retitle; that makes the nature of the problem clearer.

We discussed this issue this morning with @alexpott, @jessebeach, and myself, and it sounds like this issue (and/or a possible followup):

  • Changes the user interaction for disabling/uninstalling a theme (in that it makes it at all possible)
  • Does not actually change the data model for themes' configuration system implementation.
  • Is an API addition (relative to HEAD) for the extension system.
  • Won't break existing sites once implemented.

Based on this, @alexpott and I agreed to make this a beta target.

martin107’s picture

Issue tags: +Needs reroll

Patch no longer applies.

Now that d.o has started rounded up dates I can't use the "commented 2 months ago" in #4 to find a date to start the reroll.

mtift’s picture

@martin107 If you click twice on words like "commented about a month ago" they will change to something like "commented May 2, 2014 at 2:07pm"

martin107’s picture

@mtift thanks for the tips .. much appreciated.

Reroll of #4

Surprising a straightforward reroll no conflicts - locally the following tests pass for me

./core/modules/system/src/Tests/Extension/ThemeHandlerTest.php
./core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php

Although I am still waiting for surprises given how long 2 months is in patch years

martin107’s picture

Status: Needs work » Needs review
alexpott’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll +Needs tests

We should be testing that dependent config entities are cleaned up - for example blocks.

There is also a question about whether or not we should only allow installed themes to be uninstalled - I think this is more logical. Disabled themes are a special state - if we go from installed to uninstalled then theme code is loaded during the uninstallation which means sense and might mean we have less problems - that means we'll need to add all the checks that the theme is not the admin theme or default theme.

martin107’s picture

Status: Needs work » Needs review
FileSize
4.08 KB

Straight reroll.

RainbowArray’s picture

So it looks like what's needed here are additional tests? Plus some checks for dependencies when a theme is uninstalled?

martin107’s picture

that means we'll need to add all the checks that the theme is not the admin theme or default theme.

Currently in core \Drupal\Core\Extension\ThemeHandler::disable()

      if ($key === $theme_config->get('admin')) {
        throw new \InvalidArgumentException("The current admin theme $key cannot be disabled.");

So the admin theme cannot be moved into the disabled state. Given that uninstall, to quote from the comments - "Uninstalls a given list of disabled themes."
Is this existing check considered sufficient ?

Lowell’s picture

checked and the patch still applies against HEAD

dawehner’s picture

Test new test coverage looks nice!

  1. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -332,6 +332,32 @@ public function disable(array $theme_list) {
    +      \Drupal::service('config.manager')->uninstall('theme', $key);
    

    Let's inject that properly. Otherwise this dependency is just kinda hidden.

  2. +++ b/core/lib/Drupal/Core/Extension/ThemeHandlerInterface.php
    @@ -42,6 +42,16 @@ public function enable(array $theme_list, $enable_dependencies = TRUE);
       /**
    +   * Uninstalls a given list of disabled themes.
    +   *
    +   * @param array $theme_list
    +   *   The themes to uninstall.
    +   *
    +   * @see hook_themes_uninstalled()
    +   */
    +  public function uninstall(array $theme_list);
    

    It would be great if this documentation would explain what all is done during uninstalling a theme.

dawehner’s picture

FileSize
11.52 KB
8.8 KB

We should be testing that dependent config entities are cleaned up - for example blocks.

Done

So the admin theme cannot be moved into the disabled state. Given that uninstall, to quote from the comments - "Uninstalls a given list of disabled themes."

At least at the moment the disabled() method checks that you cannot uninstall them, and the uninstall method ensures you cannot uninstall disabled themes.

There is also a question about whether or not we should only allow installed themes to be uninstalled - I think this is more logical. Disabled themes are a special state - if we go from installed to uninstalled then theme code is loaded during the uninstallation which means sense and might mean we have less problems - that means we'll need to add all the checks that the theme is not the admin theme or default theme.

It is indeed quite confusing that even we do have installed and enabled separated under the hood, it is not installed really. Once you do that
it becomes natural that you can uninstall an installed (but not yet disabled) theme.

On top of that here are some documentation improvements.

Status: Needs review » Needs work

The last submitted patch, 26: 2232605-theme-26.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.5 KB
1.99 KB

Just fixing the test failures to unblock much more important discussions.

dawehner’s picture

I personally think we should drop the enable/disable state and just have installed / uninstalled.

@sun Do you really think we have a usecase for disabled themes?

alexpott’s picture

FileSize
44.78 KB
51.52 KB

re @dawehner #29 - I really don't think we have a use case for disabled themes - the UI as always treating disabled and uninstalled themes the same. I propose that we simply replace the ability to disable a theme with the ability to uninstall a theme. Maintaining the tri-state of uninstalled, enabled and disabled for themes feels like a overhead that we don't need. AFAICS the is not not show a theme in a theme switcher block or a user theme preference field - this affect can easily be achieved without maintaining such a complex system.

We do have some of the exact same problems with disabled themes as we have for disabled modules - what is the status of a block configuration entity for a theme that is disabled? Is it active configuration?

Patch attached completely removes disabled themes.

Status: Needs review » Needs work

The last submitted patch, 30: 2232605.30.patch, failed testing.

dawehner’s picture

As discussed with alex:

  • Let's replace enable with install in this issue
  • Ensure that we don't loose test coverage
  • Let's have fun in fixing the failures (there might be some)
alexpott’s picture

Status: Needs work » Needs review
FileSize
416 bytes
51.34 KB

No idea how I managed to duplicate the use Drupal\block\Entity\Block;

Status: Needs review » Needs work

The last submitted patch, 33: 2232605.33.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
95.34 KB
123.2 KB

Patch replaces enable with install everywhere. Making themes exactly like modules they are either installed or uninstalled.

Status: Needs review » Needs work

The last submitted patch, 35: 2232605.35.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
FileSize
123.57 KB
786 bytes

This should fix the fail.

RainbowArray’s picture

I'm 1000% behind getting rid of the disabled state. The primary use case I can think of for that is if you're letting users select a theme, but I don't think you can do that in core anymore, and I don't think anyone real does that with their sites. So yes, let's get this in!

alexpott’s picture

alexpott’s picture

I think we should update the existing https://www.drupal.org/node/2232651 change record on commit to avoid a proliferation of change records covering extremely related topics.

catch’s picture

Agreed on just having two states, the user theme-choosing use case could be handled by an explicit list of themes to prevent in the select if necessary, it doesn't need to be crammed in here.

alexpott’s picture

FileSize
123.52 KB

Need a reroll - git handled it automatically.

alexpott’s picture

Assigned: sun » Unassigned

This needs reviews - thanks :)

alexpott’s picture

Issue summary: View changes

Updated issue summary

Removed the follow since I'm not sure why it was there.

  1. Theme settings are not rigorously declared. They require representative schemas.
  2. Modules can declare theme settings. These need to be tracked and removed when the theme is uninstalled.
alexpott’s picture

I've done an automated nits review on the patch - found nothing.

dawehner’s picture

Issue summary: View changes

I consider "assigned" as the worst thing you can do to an issue. People disagree here, though :)

95% of this patch is to replace enable(d) with install(ed) and disable(d) uninstall(ed).

Tested manualoly the UI and everything on there talks about installing/uninstalling.

Awesome patch, just a couple of small observations.

  1. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -12,12 +12,13 @@
    - * Default theme handler using the config system for enabled/disabled themes.
    + * Default theme handler using the config system to store installation statuses.
    

    Your language is broken. Status, coming from the old Latin language uses "status" for its plural form as well. German inherits that properly, but sadly english doesn't. Can you please open up a new issue in the english language queue?

  2. +++ b/core/modules/system/system.routing.yml
    @@ -253,18 +253,18 @@ system.modules_list_confirm:
    -system.theme_disable:
    +system.theme_uninstall:
       path: '/admin/appearance/disable'
    ...
    -system.theme_enable:
    +system.theme_install:
       path: '/admin/appearance/enable'
    

    In case you rename the route name and the controller you should do the same for the path itself.

  3. +++ b/core/modules/update/update.module
    diff --git a/core/modules/user/css/user.module.css b/core/modules/user/css/user.module.css
    index 658988d..8a03bdb 100644
    
    index 658988d..8a03bdb 100644
    --- a/core/modules/user/css/user.module.css
    
    --- a/core/modules/user/css/user.module.css
    +++ b/core/modules/user/css/user.module.css
    
    +++ b/core/modules/user/css/user.module.css
    +++ b/core/modules/user/css/user.module.css
    @@ -26,7 +26,8 @@
    
    @@ -26,7 +26,8 @@
     }
     .password-strength__meter {
       background-color: #ebeae4;
    -  height: 0.5em;
    +  height: 0.75em;
    +  margin-top: 0.5em;
     }
     .password-strength__indicator {
       height: 100%;
    

    probably merge problem.

alexpott’s picture

FileSize
1.15 KB
123.21 KB

re: #46

  1. English is not German we don't have a central bureaucracy to file an issue against - just start using the word that way and if it becomes common usage then the dictionaries will be updated :)
  2. Fixed
  3. Fixed
alexpott’s picture

alexpott’s picture

Issue summary: View changes
alexpott’s picture

FileSize
123.21 KB

Okay d.org my file got removed whilst fixing the issue summary... uploading patch from #47

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you

catch’s picture

Status: Reviewed & tested by the community » Needs work

Looks good overall. Couple of nits.

  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -395,9 +395,9 @@ protected function createExtensionChangelist() {
    +    $theme_install = array_diff(array_keys($new_extensions['theme']), array_keys($current_extensions['theme']));
    

    array_diff_key()

  2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -395,9 +395,9 @@ protected function createExtensionChangelist() {
    +    $theme_uninstall = array_diff(array_keys($current_extensions['theme']), array_keys($new_extensions['theme']));
    

    array_diff_key()

  3. +++ b/core/lib/Drupal/Core/Utility/ProjectInfo.php
    @@ -19,6 +19,10 @@ class ProjectInfo {
    +   *   to be installed for sub themes. to work.
    

    sub themes. to work.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
822 bytes
123.21 KB
  1. array_diff(array_keys($foo), array_keys($bar)); is the same as array_keys(array_diff_keys($foo, $bar)); - let;s do that in a followup since there are a few examples of this pattern in ConfigImporter that are not theme related - created #2340701: Use array_diff_key() more in core
  2. See #1
  3. Fixed

Considering this is just a comment fix and I discussed 1,2 with @catch - setting to rtbc again.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 2a0254f on 8.0.x
    Issue #2232605 by alexpott, dawehner, martin107, Cottser, sun: Fixed...
csakiistvan’s picture

I found a bug in classy theme, add it to related issue.

Status: Fixed » Closed (fixed)

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