Allow (base-)themes to be excluded from the UI (e.g. blocks, Appearance).

The previous issue title really confused me. ;)

hi,
there is also the 'hidden' key that can be used.
following patch uses it to hide test themes from the UI.

We don't want to hide themes but instead be able to not allow them being enabled as is e.g base themes

@joelpittet: How should the markup of these themes be tested?

I would prefer it if we had a theme_type key with the value base. It has a semantic meaning and opens the door to use adding more values like admin or non_admin so we can prevent people from selecting admin themes as frontend and vice-versa.

I'm not saying we should do that work here but the key we choose would affect this. What do you think?

Discussed with @catch and we agreed that this is an rc target and eligible for commit during the rc-phase. This is an key UI and with the addition of stable it will get even more unnecessarily complex. This was not an issue in D7 because base themes did not have to be installed.

I think we should try to determine the scope of what we're really talking about here. I think @izus is on to something and that we could probably use the hidden property to hide it from the Appearance UI.

But then we want to also:

• Hide these themes from the Block UI
• Prevent a "hidden" theme from being set as the default theme, main or admin
• Other things?

I love the idea of re-using hidden: true; making themes more like modules FTW.

I also think #14 sounds good; "hidden" = hidden from the UI, and that would include all UIs which list themes (which in core is only admin/appearance and admin/structure/block afaik). I think that hiding them from the UI therefore also prevents them from being enabled / default (except maybe by some fancy drush magic, but that'd be outside of core), by definition.

One problem I'm seeing is that if we hide these base themes from the UI it's really difficult to uninstall them if they're no longer being used.

One problem I'm seeing is that if we hide these base themes from the UI it's really difficult to uninstall them if they're no longer being used.

What about hiding it unless all their child themes are uninstalled?

Well, I'd rather handle this the way users would expect, which is if you uninstall a front-end/admin theme, and its parent base theme has no other active themes depending on it, Drupal uninstalls the base theme as well.

While that's very risky to do in the case of modules, given uninstall inherently = data loss, here we're deliberately removing the ability to add block configurations, etc. to base themes so I think it's safe?

Yeah I'm more in favour of handling it behind the scenes like that.

Alright, this makes most sense. This is in the end *magic* that users don't need to be aware of. Even for themers its not really relevant. When there theme depends on a base theme its always installed, when that theme is installed.

Ok cool, spun off a separate issue for that since it has nothing to do with UI hiding: #2581385: Automagically uninstall unused base themes when sub themes are uninstalled.

FWIW catch thinks it's not a huge deal and we don't need to block this patch on that one. There's a workaround (use Drush) and these "phantom" themes shouldn't represent a huge performance suck.

I was playing around with this a few days ago, uploading a patch to get things started. I can't remember how far I got with the tests so uploading two patches to see the delta in test fails.

No interdiff from the earlier patch since there is no commonality.

We shouldn't be listing hidden themes in the block ui either. And also this means we can't use classy as a theme for testing.

Fixed all the test fails.

Wrong interdiff

Fix more of the tests and add some for the Block UI and base themes not appearing.

Thanks very much @alexpott!

1. +++ b/core/modules/block/src/Tests/BlockUiTest.php
   @@ -83,13 +83,16 @@ protected function setUp() {
   +    $this->assertResponse(403, 'Hidden themes cannot are not supported by the block demo screen');
   

   Minor: Can probably just remove 'cannot' here.

2. +++ b/core/modules/system/src/Tests/System/ThemeTest.php
   @@ -126,6 +126,7 @@ function testThemeSettings() {
   +      $themes = \Drupal::service('theme_handler')->listInfo();
   

   This seems like it's not needed, if it is maybe it needs a comment.

Thanks @Cottser. Fixed both points.

I can't find any faults here and I really like how this cleans up Classy. I think test_classy is a very good idea.

Do we want to also hide these themes from /admin/appearance/settings (theme settings)? Just want to make sure we discuss that.

And I just want to mention that I tested it and it's still possible to uninstall Classy/Stable/other hidden base themes via Drush as long as themes depending on them are uninstalled first. If you're creative you can also do this via the UI.

Yes we do also want to hide these themes from admin/appearance/settings

The #32 patch works perfectly.

I think we should also hide these themes from admin/appearance/settings as #33 suggested.

We might also want a setting to allow to see these themes again. We already have $settings['extension_discovery_scan_tests'] for test themes and modules. Why not something for hidden themes (and modules)?

Postponed #2581443: Make Classy extend from the new Stable base theme on this issue.

Also I think 403's are the wrong responses from the BlockUI when trying to access Classy or Stable. It implies there is some permission that can be granted that would give the user access. There is no permission.

Patch in #36 fixes theme settings for the hidden base themes.

The only remaining question is what about sites that have blocks and theme settings for Classy and Stable on existing sites. It is possible that someone has set classy to be their default theme. The patch in #36 would break the functionality of their site - and I don't think we should do that. So... new approach that leads to a smaller patch.

So the one funky thing about the patch in #38 is that if you were to change the default away from Classy or Stable then it would just disappear. Not sure what we should do about that.

With that approach should we also be checking if it's set as the admin theme?

+++ b/core/modules/block/block.module
@@ -88,8 +88,13 @@ function block_page_top(array &$page_top) {
+    if (empty($theme_info[$theme]->info['hidden'])) {

Should this be checking hasUi now?

Okay so the problem I alluded to in #30 causes a few test fails. But they are all easy to fix and the fixes reduce our test dependency on classy so looking good imo.

@Cottser - excellent point yes!

The massive advantage of the new approach is that it means we don't have to try to stop people from setting these base themes as admin or default themes.

Ah @Cottser actually no... there is no point here because this only happens on theme install there so it can;t be default or admin at this point - unless it is the first theme in which case there are no blocks to copy so...

Fixing the last test fail.

The interdiff on #46 was skewiff...

Mostly docs nitpicks, it's definitely nice that this patch is less than half the size of the earlier version and also doesn't mess with folks who might be directly using these themes.

1. +++ b/core/modules/block/src/Controller/BlockController.php
   @@ -53,6 +54,11 @@ public static function create(ContainerInterface $container) {
   +    // Deny access if the theme is not installed or not found.
   
   +++ b/core/modules/block/src/Controller/BlockListController.php
   @@ -28,6 +57,11 @@ class BlockListController extends EntityListController {
   +    // Deny access if the theme is not installed or not found.
   
   +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
   @@ -108,13 +108,12 @@ public function buildForm(array $form, FormStateInterface $form_state, $theme =
   +      // Deny access if the theme is not installed or not found.
   

   I think this comment could be a bit more accurate if it's going to be used in a few places, maybe change "or not found" to "or is hidden from the UI".

2. +++ b/core/modules/block/src/Tests/BlockUiTest.php
   @@ -136,6 +140,29 @@ function testBlockAdminUiPage() {
   +    \Drupal::service('theme_installer')->install(['stable']);
   +    \Drupal::service('theme_installer')->install(['test_theme']);
   

   Curious why these are installed separately, maybe it merits a comment.

3. +++ b/core/modules/system/src/Tests/System/ThemeTest.php
   @@ -52,6 +52,9 @@ function testThemeSettings() {
   +    $this->assertResponse(404, 'The theme settings form URL for a hidden theme are unavailable.');
   

   Minor: I think this should be s/are unavailable/is unavailable/

4. +++ b/core/modules/system/src/Tests/System/ThemeTest.php
   @@ -190,6 +193,23 @@ function testThemeSettings() {
   +    // Ensure only vlaid themes are listed in the local tasks.
   

   Minor: s/vlaid/valid/

5. +++ b/core/modules/system/src/Tests/System/ThemeTest.php
   @@ -201,6 +201,15 @@
   +    $this->assertResponse(200, 'The theme settings form URL for a hidden theme that is the admin theme are avialable.');
   

   Minor: s/are avialable/is available/

6. +++ b/core/modules/system/src/Tests/System/ThemeTest.php
   @@ -255,8 +275,14 @@ function testAdministrationTheme() {
   +    // First, Install Stark and set it as the default theme programmatically.
   

   Very minor: s/Install/install/

1. TBH I think we can just remove this comment - the hasUi() method name is pretty descriptive and now that ThemeHandler::listInfo() only returns installed themes the whole installed thing is superfluous.
2. No good reason at all - changed test_theme to stark to be more real
3. Fixed
4. Fixed
5. Fixed
6. Fixed

I also removed the unneeded router rebuild setting in ThemeController::setDefaultTheme() cause the config event does that for us - as it should.

Added missing test coverage for the change to block_themes_installed(). Plus decided to use ThemeHandler::hasUi() because it is less reaching inside the theme's info array and plucking stuff out and it is nicely self documenting.

#50 is gonna fail because base_theme is a custom theme I have lying around. lolz.

Ship it! hasUi is much cleaner and this does everything it should now and has good test coverage. The config event thing is very nice too.

+++ b/core/modules/block/src/Controller/BlockListController.php
@@ -28,6 +57,10 @@ class BlockListController extends EntityListController {
+      throw new NotFoundHttpException();
+    }
+

I'd normally expect this to be done in the access callback, but there's something to be said for it being a 404 semantically.

No other comments really, so committed/pushed to 8.0.x, thanks!

eh, can we have a followup for the UI part?

@Bojhan sorry which part do you mean?