Problem/Motivation

We currently show any theme that is added on the Appearance page. This results in a page that is highly convoluted and mixes base themes, admin themes and themes you actually wish to install.

In Drupalcon Barcelona conversations we expect this to become an even bigger problem out of the box. We want to disallow people choosing "Classy" as their theme and placing a block in it. This is completely non-sensical.

Proposed resolution

We propose to add a new YAML property:

+hidden: true

This allows us to hide the theme for selection in the Appearance page, for placing blocks in the Block UI and in Theme Settings. However if the hidden theme is the default theme then it is still display on the Block UI and Theme Settings page. This means we don't have to worry about sites which are currently using Stable or Classy as their default theme.

This issue blocks #2581443: Make Classy extend from the new Stable base theme

Remaining tasks

User interface changes

We don't show base themes on the Apperance page, Blocks UI, or Theme settings, unless the base theme is the default theme.

API changes

block_themes_installed() does not auto create block placements for hidden themes.

Data model changes

None

Files: 
CommentFileSizeAuthor
#51 2574975-new-51.patch26.77 KBalexpott
#51 50-51-interdiff.txt1.33 KBalexpott
#50 2574975-new-50.patch26.21 KBalexpott
#50 49-50-interdiff.txt1.74 KBalexpott
#49 2574975-new-49.patch25.34 KBalexpott
#49 46-49-interdiff.txt7.2 KBalexpott
#47 41-46-interdiff.txt6.03 KBalexpott
#46 2574975-new-46.patch23.66 KBalexpott
#46 41-46-interdiff.txt643 bytesalexpott
#42 2574975-new-41.patch19.17 KBalexpott
#42 38-41-interdiff.txt4.03 KBalexpott
#38 2574975-new-38.patch15.15 KBalexpott
#38 36-38-interdiff.txt54.84 KBalexpott
#36 2574975-36.patch63.51 KBalexpott
#36 32-36-interdiff.txt9.11 KBalexpott
#32 2574975-32.patch57.6 KBalexpott
#32 30-32-interdiff.txt1.28 KBalexpott
#30 2574975-30.patch58.02 KBalexpott
#30 27-30-interdiff.txt7.54 KBalexpott
#28 25-27-interdiff.txt9.21 KBalexpott
#27 2574975-27.patch50.48 KBalexpott
#27 25-27-interdiff.txt645 bytesalexpott
#25 2574975-25.patch41.27 KBalexpott
#25 22-25-interdiff.txt39.16 KBalexpott
#22 allow_base_themes_to-2574975-22.patch3.02 KBCottser
#22 allow_base_themes_to-2574975-22-no-test-changes.patch608 bytesCottser
#4 Drupal_core-exclude_themes_from_ui-2574975-4.patch12.86 KBizus
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,972 pass(es), 4 fail(s), and 1 exception(s). View

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

xjm’s picture

Title: Exclude themes from the UI » Allow themes to be excluded from the UI

The previous issue title really confused me. ;)

izus’s picture

Status: Active » Needs review
FileSize
12.86 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,972 pass(es), 4 fail(s), and 1 exception(s). View

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

lauriii’s picture

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

Status: Needs review » Needs work

The last submitted patch, 4: Drupal_core-exclude_themes_from_ui-2574975-4.patch, failed testing.

The last submitted patch, 4: Drupal_core-exclude_themes_from_ui-2574975-4.patch, failed testing.

joelpittet’s picture

@lauriii we also want to hide them from the block UI example but they need to be available to the the sub-themes.

lauriii’s picture

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

Bojhan’s picture

Title: Allow themes to be excluded from the UI » Allow (base-)themes to be excluded from the UI (e.g. blocks, Appearance).
Issue summary: View changes
LewisNyman’s picture

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?

Cottser’s picture

alexpott’s picture

Issue tags: +rc target

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.

Cottser’s picture

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?
webchick’s picture

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.

Cottser’s picture

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.

dawehner’s picture

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?

webchick’s picture

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?

Cottser’s picture

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

Bojhan’s picture

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.

webchick’s picture

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.

Cottser’s picture

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.

The last submitted patch, 22: allow_base_themes_to-2574975-22-no-test-changes.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 22: allow_base_themes_to-2574975-22.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
39.16 KB
41.27 KB

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.

Status: Needs review » Needs work

The last submitted patch, 25: 2574975-25.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
645 bytes
50.48 KB

Fixed all the test fails.

alexpott’s picture

FileSize
9.21 KB

Wrong interdiff

Status: Needs review » Needs work

The last submitted patch, 27: 2574975-27.patch, failed testing.

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
7.54 KB
58.02 KB

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

Cottser’s picture

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.

alexpott’s picture

Thanks @Cottser. Fixed both points.

Cottser’s picture

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.

alexpott’s picture

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

DuaelFr’s picture

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)?

alexpott’s picture

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.

alexpott’s picture

Issue summary: View changes

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

alexpott’s picture

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.

alexpott’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 38: 2574975-new-38.patch, failed testing.

Cottser’s picture

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?

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.03 KB
19.17 KB

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.

alexpott’s picture

@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.

alexpott’s picture

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...

Status: Needs review » Needs work

The last submitted patch, 42: 2574975-new-41.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
643 bytes
23.66 KB

Fixing the last test fail.

alexpott’s picture

FileSize
6.03 KB

The interdiff on #46 was skewiff...

Cottser’s picture

Status: Needs review » Needs work

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/

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.2 KB
25.34 KB
  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.

alexpott’s picture

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.

alexpott’s picture

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

The last submitted patch, 50: 2574975-new-50.patch, failed testing.

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

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.

catch’s picture

Status: Reviewed & tested by the community » Fixed
+++ 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!

  • catch committed a2988f1 on 8.0.x
    Issue #2574975 by alexpott, Cottser, izus: Allow (base-)themes to be...

  • catch committed a2988f1 on 8.1.x
    Issue #2574975 by alexpott, Cottser, izus: Allow (base-)themes to be...
Bojhan’s picture

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

Cottser’s picture

@Bojhan sorry which part do you mean?

Status: Fixed » Closed (fixed)

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