I realize I suggested this grand hack late in the Drupal release cycle at http://drupal.org/node/192779#comment-661039 and committed these special cases in #192779: Don't show disabled themes on blocks settings page and #201577: show admin theme on page admin/build/themes/settings, but this was one very bad idea.

The original issue as I understand it is that if the admin theme is *enabled*, it can be selected by those users who have the permission to select themes. And this is not desired. So the D6 "solution" was to let the admin theme "work" while being disabled (so you can configure blocks and its settings even though it is disabled). So what's an enabled theme really boiled down to who you ask. If the end user, it was the themes you've set enabled. If Drupal's blocks and themes subsystem, it was the enabled themes plus the admin theme. But what is dangerous is that other subsystems of Drupal like update_status do not have this special casing for admin themes, and when disabled, they are just considered disabled. So if you use a contributed admin theme, update status will never tell you if you have a security update to it, your site will keep being vulnerable (and having your *admin* theme vulnerable can be quite an issue).

So we can continue sprinkling Drupal with special casing of disabled admin themes (more hacks), but in reality we should just fix our mental model. We should raise the question: what does enabling a theme means?

- You can configure its own settings
- You can configure blocks for it
- You can pick it as an admin theme
- You get update status reports about it

Drupal 6 also used to say "- Your users (with appropriate permissions) can pick this theme", but due to #292253: Remove the per-user themes selection from core this is not present in Drupal 7 anymore. However, if we'd consider user theme selection in core, the solution is still not to let the admin theme be disabled, since all the above and other theme related features need ugly special casing. Instead, we should fix the user theme selector then to not include the admin theme. Or even better, just have a configuration screen to pick which themes are available to users, so the above list would be extended with:

- You can pick the theme as one of the user selectable themes

This would map to sites where modules like themekey are used to switch themes based on sections. Maybe not all of those themes would the site owner expose to users. While we can exclude the admin theme, the more generic use case of themekey + user specific theming is impossible to infer programatically.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

I'm working on implementing #491214: implement the top level Appearance / Choose Theme admin page, and we really need a definition of what enabled means, so therefore the overall thinking on its issues and the attempt to properly solve them finally. In other words, fixing this would be a good kick to the Appearance page in the D7UX implementation.

Status: Needs review » Needs work

The last submitted patch failed testing.

Pasqualle’s picture

subscribe

hass’s picture

+

xmacinfo’s picture

The original issue as I understand it is that if the admin theme is *enabled*, it can be selected by those users who have the permission to select themes. And this is not desired.

This is not an issue. A user should have the right to use Seven as the default theme. Even if Seven does not have all the regions as one might expect, it still is a good theme for regular pages. :-)

Gábor Hojtsy’s picture

@xmacinfo: ok, I'll try to be more clear. The original issue **in Drupal 6** was that there was a feature called user theme selection where all enabled themes showed up without consideration. So therefore if you wanted to offer theme selection to users, but exclude the admin theme, you'd need to disable your admin theme so it does not show up in that list. Therefore the hacks which IMHO should be removed now that we have no user theme selection. If we are going to have user theme selection or some other similar feature, we should have a selection of the themes supported on the UI with a user interface to pick which themes to list, so hacks like disabling some themes will not be required.

xmacinfo’s picture

@Gábor: Those theming issues can become quite complex and hard to follow. Yes, now the end user cannot change theme. In my message I should have written the Admin user can use any theme and mark it as admin, whereas any theme, including Seven, can be used as a regular theme. So, yes, we must not special case the admin theme, disabled or not. Forgive me if I miss some points. :-)

Can you reroll this patch?

seutje’s picture

subscribing, and reading a couple more times

Pasqualle’s picture

Status: Needs work » Needs review
FileSize
5.99 KB

reroll, with changes:
1. made sure that the admin theme is enabled after submitting the theme settings form
2. the wrong block test removed
3. one wrong and useless message removed

Status: Needs review » Needs work

The last submitted patch failed testing.

Pasqualle’s picture

Status: Needs work » Needs review
FileSize
5.15 KB

rerolled without the useless message, see: #556714: Remove the administration theme status message

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Looks great. I'd only add some parenthesis in the .. || .. == .. || .. == .. sequence to make it more clear :)

Pasqualle’s picture

Status: Needs work » Needs review
FileSize
5.15 KB

added parenthesis

Status: Needs review » Needs work

The last submitted patch failed testing.

Pasqualle’s picture

Status: Needs work » Needs review

retest?

Status: Needs review » Needs work

The last submitted patch failed testing.

Pasqualle’s picture

Status: Needs work » Needs review
FileSize
5.28 KB

reroll

Status: Needs review » Needs work

The last submitted patch failed testing.

marcingy’s picture

Version: 7.x-dev » 8.x-dev
Assigned: Unassigned » marcingy
marcingy’s picture

Status: Needs work » Needs review
FileSize
7.22 KB

Updating patch for d8

Status: Needs review » Needs work

The last submitted patch, end_special_casing_admin_theme.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
11.28 KB

Ok hopefully tests are fixed up.

sun’s picture

Component: base system » theme system
Issue tags: +Framework Initiative, +API clean-up
FileSize
10.98 KB

OMG! This is totally in line with my thinking...

#1067408: Themes do not have an installation status
#1293550-54: Setting a currently disabled theme as admin puts block admin in a broken state

Re-rolled against HEAD and cleaned up the code a bit. Also reverted the removal/renaming of the test case.

Status: Needs review » Needs work

The last submitted patch, drupal8.admin-theme.23.patch, failed testing.

sun’s picture

Assigned: marcingy » sun
Status: Needs work » Needs review
FileSize
11.01 KB

Fixed that test.

sun’s picture

Assigned: sun » Unassigned
FileSize
11.05 KB

Added a 'theme-admin' CSS class to the admin theme to make the revised admin theme output truly equal to the default theme output on the Appearance page.

This actually looks ready to fly for me.

sun’s picture

Assigned: Unassigned » sun

Unintentionally unassigned me, sorry.

afeijo’s picture

Status: Needs review » Reviewed & tested by the community

Patch tested, now only the enabled themes show in the Admin Theme select
box
and I can only disable the themes that are not under use by the site
pages nor the admin pages
to be able to disable the Seven theme, I had to set Bartik to be the
Admin theme
Good change, mates!

Dries’s picture

Would love to see Gabor review this patch.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

I carefully looked through this patch. All it does is it removes the special casing in access callbacks and only lets you disable a theme, if its not the default and not the admin theme. It also fixes tests to actually enable themes when it sets different themes for admin.

I only found this minor issue, otherwise looks all good:

+++ b/core/modules/system/system.admin.incundefined
@@ -296,7 +303,7 @@ function system_theme_disable() {
-      if ($theme == variable_get('theme_default', 'stark')) {
+      if ($theme == variable_get('theme_default', 'stark') || $theme == variable_get('admin_theme', 0)) {
         // Don't disable the default theme.
         drupal_set_message(t('%theme is the default theme and cannot be disabled.', array('%theme' => $themes[$theme]->info['name'])), 'error');

The code comment is now outdated.

sun’s picture

Status: Needs work » Needs review
FileSize
674 bytes
11.12 KB

Fixed that comment. :)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Should be good to go then. The tests should still pass :)

xjm’s picture

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockAdminThemeTest.phpundefined
@@ -35,12 +35,13 @@ class BlockAdminThemeTest extends WebTestBase {
-    $this->assertResponse(403, t('The block admin page for a disabled theme can not be accessed'));
+    $this->assertResponse(403);
...
-    $this->assertResponse(200, t('The block admin page for the admin theme can be accessed'));
+    $this->assertResponse(200);

Should we be updating these assertion messages rather than removing them?

Alternately, maybe a followup issue that adds the request path to assertResponse()?

sun’s picture

My stance on that is this:

- If the test fails, it will fail.
- Running the test locally will reveal all information that is necessary to fix any kind of test failure.
- We are using way too many custom assertion messages anyway, in almost all cases unhelpful in case a test actually fails.
- Better get rid of most of them and back to sanity.

Dries’s picture

Asking for a re-test. Patch no longer applies on my localhost.

Dries’s picture

#31: drupal8.admin-theme.31.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Framework Initiative, +API clean-up

The last submitted patch, drupal8.admin-theme.31.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
11.08 KB

Re-rolled against HEAD. No difference. (Test $modules property changes merely conflicted.)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for the re-roll. Committed to 8.x.

Status: Fixed » Closed (fixed)
Issue tags: -Framework Initiative, -API clean-up

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