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.
Comment | File | Size | Author |
---|---|---|---|
#38 | drupal8.admin-theme.38.patch | 11.08 KB | sun |
#31 | drupal8.admin-theme.31.patch | 11.12 KB | sun |
#31 | interdiff.txt | 674 bytes | sun |
#26 | drupal8.admin-theme.26.patch | 11.05 KB | sun |
#25 | drupal8.admin-theme.25.patch | 11.01 KB | sun |
Comments
Comment #1
Gábor HojtsyI'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.
Comment #3
Pasquallesubscribe
Comment #4
hass CreditAttribution: hass commented+
Comment #5
xmacinfoThis 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. :-)
Comment #6
Gábor Hojtsy@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.
Comment #7
xmacinfo@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?
Comment #8
seutje CreditAttribution: seutje commentedsubscribing, and reading a couple more times
Comment #9
Pasquallereroll, 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
Comment #11
Pasquallererolled without the useless message, see: #556714: Remove the administration theme status message
Comment #12
Gábor HojtsyLooks great. I'd only add some parenthesis in the .. || .. == .. || .. == .. sequence to make it more clear :)
Comment #13
Pasqualleadded parenthesis
Comment #15
Pasqualleretest?
Comment #17
Pasquallereroll
Comment #19
marcingy CreditAttribution: marcingy commentedComment #20
marcingy CreditAttribution: marcingy commentedUpdating patch for d8
Comment #22
marcingy CreditAttribution: marcingy commentedOk hopefully tests are fixed up.
Comment #23
sunOMG! 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.
Comment #25
sunFixed that test.
Comment #26
sunAdded 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.
Comment #27
sunUnintentionally unassigned me, sorry.
Comment #28
afeijoPatch 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!
Comment #29
Dries CreditAttribution: Dries commentedWould love to see Gabor review this patch.
Comment #30
Gábor HojtsyI 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:
The code comment is now outdated.
Comment #31
sunFixed that comment. :)
Comment #32
Gábor HojtsyShould be good to go then. The tests should still pass :)
Comment #33
xjmShould we be updating these assertion messages rather than removing them?
Alternately, maybe a followup issue that adds the request path to
assertResponse()
?Comment #34
sunMy 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.
Comment #35
Dries CreditAttribution: Dries commentedAsking for a re-test. Patch no longer applies on my localhost.
Comment #36
Dries CreditAttribution: Dries commented#31: drupal8.admin-theme.31.patch queued for re-testing.
Comment #38
sunRe-rolled against HEAD. No difference. (Test $modules property changes merely conflicted.)
Comment #39
Dries CreditAttribution: Dries commentedThanks for the re-roll. Committed to 8.x.