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
Comment | File | Size | Author |
---|---|---|---|
#53 | 2232605.53.patch | 123.21 KB | alexpott |
#53 | 50-53-interdiff.txt | 822 bytes | alexpott |
#50 | 2232605.47.patch | 123.21 KB | alexpott |
#42 | 2232605.42.patch | 123.52 KB | alexpott |
#39 | 2232605.39.patch | 123.49 KB | alexpott |
Comments
Comment #1
jessebeach CreditAttribution: jessebeach commentedComment #2
sunThis 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.Comment #3
sunComment #4
sunBaseline: API + tests.
Comment #5
sunI'd like to split this issue into two:
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).
Comment #6
jessebeach CreditAttribution: jessebeach commentedCan 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.
Comment #7
Gábor HojtsyDisabled modules were removed as a concept exactly due to data model / hooks issues :) Looks like we don't want that happen again here?
Comment #8
sunSorry 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.
Comment #9
sunCreated:
#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.
Comment #10
jessebeach CreditAttribution: jessebeach commentedThe 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.
Comment #11
catchIf 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.
Comment #12
jessebeach CreditAttribution: jessebeach commentedIt'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.
Comment #13
sun@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.
Comment #14
jessebeach CreditAttribution: jessebeach commentedDo 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.
Comment #15
xjmThanks 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):
Based on this, @alexpott and I agreed to make this a beta target.
Comment #16
martin107 CreditAttribution: martin107 commentedPatch 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.
Comment #17
mtift@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"
Comment #18
martin107 CreditAttribution: martin107 commented@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
Comment #19
martin107 CreditAttribution: martin107 commentedComment #20
alexpottWe 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.
Comment #21
martin107 CreditAttribution: martin107 commentedStraight reroll.
Comment #22
RainbowArraySo it looks like what's needed here are additional tests? Plus some checks for dependencies when a theme is uninstalled?
Comment #23
martin107 CreditAttribution: martin107 commentedCurrently in core \Drupal\Core\Extension\ThemeHandler::disable()
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 ?
Comment #24
Lowell CreditAttribution: Lowell commentedchecked and the patch still applies against HEAD
Comment #25
dawehnerTest new test coverage looks nice!
Let's inject that properly. Otherwise this dependency is just kinda hidden.
It would be great if this documentation would explain what all is done during uninstalling a theme.
Comment #26
dawehnerDone
At least at the moment the disabled() method checks that you cannot uninstall them, and the uninstall method ensures you cannot uninstall disabled themes.
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.
Comment #28
dawehnerJust fixing the test failures to unblock much more important discussions.
Comment #29
dawehnerI 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?
Comment #30
alexpottre @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.
Comment #32
dawehnerAs discussed with alex:
Comment #33
alexpottNo idea how I managed to duplicate the
use Drupal\block\Entity\Block;
Comment #35
alexpottPatch replaces enable with install everywhere. Making themes exactly like modules they are either installed or uninstalled.
Comment #37
star-szrThis should fix the fail.
Comment #38
RainbowArrayI'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!
Comment #39
alexpottAdded issues for the @todo's #2338167: Update ProjectInfo class to reflect changes to extension system and #2338175: [meta] Update module terminology still refers to disabled extensions when it should be uninstalled
Afaik this issue has the required test coverage.
Comment #40
alexpottI 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.
Comment #41
catchAgreed 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.
Comment #42
alexpottNeed a reroll - git handled it automatically.
Comment #43
alexpottThis needs reviews - thanks :)
Comment #44
alexpottUpdated issue summary
Removed the follow since I'm not sure why it was there.
Comment #45
alexpottI've done an automated nits review on the patch - found nothing.
Comment #46
dawehnerI 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.
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?
In case you rename the route name and the controller you should do the same for the path itself.
probably merge problem.
Comment #47
alexpottre: #46
Comment #48
alexpottComment #49
alexpottComment #50
alexpottOkay d.org my file got removed whilst fixing the issue summary... uploading patch from #47
Comment #51
dawehnerThank you
Comment #52
catchLooks good overall. Couple of nits.
array_diff_key()
array_diff_key()
sub themes. to work.
Comment #53
alexpottarray_diff(array_keys($foo), array_keys($bar));
is the same asarray_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 coreConsidering this is just a comment fix and I discussed 1,2 with @catch - setting to rtbc again.
Comment #54
catchCommitted/pushed to 8.0.x, thanks!
Comment #56
csakiistvanI found a bug in classy theme, add it to related issue.