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.

Files: 
CommentFileSizeAuthor
#38 drupal8.admin-theme.38.patch11.08 KBsun
PASSED: [[SimpleTest]]: [MySQL] 39,979 pass(es).
[ View ]
#31 drupal8.admin-theme.31.patch11.12 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.admin-theme.31.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#31 interdiff.txt674 bytessun
#26 drupal8.admin-theme.26.patch11.05 KBsun
PASSED: [[SimpleTest]]: [MySQL] 39,745 pass(es).
[ View ]
#25 drupal8.admin-theme.25.patch11.01 KBsun
PASSED: [[SimpleTest]]: [MySQL] 39,741 pass(es).
[ View ]
#23 drupal8.admin-theme.23.patch10.98 KBsun
FAILED: [[SimpleTest]]: [MySQL] 39,744 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#22 end_special_casing_admin_theme.patch11.28 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 36,659 pass(es).
[ View ]
#20 end_special_casing_admin_theme.patch7.22 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 36,644 pass(es), 11 fail(s), and 0 exception(s).
[ View ]
#17 542828-17-no-more-disabled-admin-theme.patch5.28 KBPasqualle
Failed: 13648 passes, 1 fail, 0 exceptions
[ View ]
#13 542828-13-no-more-disabled-admin-theme.patch5.15 KBPasqualle
Failed: Failed to apply patch.
[ View ]
#11 542828-11-no-more-disabled-admin-theme.patch5.15 KBPasqualle
Failed: 13440 passes, 10 fails, 0 exceptions
[ View ]
#9 542828-9-no-more-disabled-admin-theme.patch5.99 KBPasqualle
Failed: Failed to apply patch.
[ View ]
no-more-disabled-admin-theme.patch2.94 KBGábor Hojtsy
Failed: 11822 passes, 3 fails, 0 exceptions
[ View ]

Comments

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.

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

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

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

subscribing, and reading a couple more times

Status:Needs work» Needs review
StatusFileSize
new5.99 KB
Failed: Failed to apply patch.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new5.15 KB
Failed: 13440 passes, 10 fails, 0 exceptions
[ View ]

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new5.15 KB
Failed: Failed to apply patch.
[ View ]

added parenthesis

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

retest?

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.28 KB
Failed: 13648 passes, 1 fail, 0 exceptions
[ View ]

reroll

Status:Needs review» Needs work

The last submitted patch failed testing.

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

Status:Needs work» Needs review
StatusFileSize
new7.22 KB
FAILED: [[SimpleTest]]: [MySQL] 36,644 pass(es), 11 fail(s), and 0 exception(s).
[ View ]

Updating patch for d8

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new11.28 KB
PASSED: [[SimpleTest]]: [MySQL] 36,659 pass(es).
[ View ]

Ok hopefully tests are fixed up.

Component:base system» theme system
Issue tags:+Framework Initiative, +API clean-up
StatusFileSize
new10.98 KB
FAILED: [[SimpleTest]]: [MySQL] 39,744 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

Assigned:marcingy» sun
Status:Needs work» Needs review
StatusFileSize
new11.01 KB
PASSED: [[SimpleTest]]: [MySQL] 39,741 pass(es).
[ View ]

Fixed that test.

Assigned:sun» Unassigned
StatusFileSize
new11.05 KB
PASSED: [[SimpleTest]]: [MySQL] 39,745 pass(es).
[ View ]

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.

Assigned:Unassigned» sun

Unintentionally unassigned me, sorry.

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!

Would love to see Gabor review this patch.

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.

Status:Needs work» Needs review
StatusFileSize
new674 bytes
new11.12 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.admin-theme.31.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixed that comment. :)

Status:Needs review» Reviewed & tested by the community

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

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

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.

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

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

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new11.08 KB
PASSED: [[SimpleTest]]: [MySQL] 39,979 pass(es).
[ View ]

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

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.