Merge administration theme with hook_admin_paths()

This would be awesome if it can be pulled off. The basic idea of merging them was discussed by @ksenzee during the overlay work, but it was thought it was too late for D7... Perhaps it might not be, though.

The challenge is this: Suppose I want my regular users to see node pages in the regular site theme, but as an administrator I still want to create content in the overlay - or do other things that depend on the return value of path_is_admin()?

Currently Drupal allows this, but with this patch as it currently stands, I don't believe that would be possible. Also, in the code:

+  $paths = array();
+  if (variable_get('node_admin_theme', 0)) {
+    $paths += array(
+      'node/*/edit' => TRUE,
+      'node/*/delete' => TRUE,
+      'node/*/revisions' => TRUE,
+      'node/*/revisions/*/revert' => TRUE,
+      'node/*/revisions/*/delete' => TRUE,
+      'node/add' => TRUE,
+      'node/add/*' => TRUE,
+    );
+  }
   return $paths;

This in general seems a little suspect because it's labeling the functionality of a page (admin or not) based on the theme that you want to display for it. And it doesn't get node-related paths defined by other modules.

I think there might be a way to solve all this though. What if we added a permission to core called "view the administrative theme"? Then the code would be something like this:

if (variable_get('admin_theme') && user_access('view the administrative theme') && path_is_admin($path)) {
  // Use the administrative theme.
}

This could probably happen dynamically on all page loads (depending on what happens with #553944: Define hook_menu_get_item_alter() as a reliable hook that runs before the page is doomed) and therefore not have to bother with the menu rebuilds and such.

The node module would I guess then have to add some trickery of its own that only applies when the path starts with 'node'..... but the basic idea is that if we keep the lists in hook_admin_paths() fixed, but use a permission to determine who actually sees the admin theme on those pages, then all you have to do is deny your regular site visitors the 'view the administrative theme' permission and the functionality described above would be preserved.

Also, there is this lovely code in system.module to deal with:

/**
 * Implements hook_init().
 */
function system_init() {
  // Add the CSS for this module.
  if (arg(0) == 'admin' || (variable_get('node_admin_theme', '0') && arg(0) == 'node' && (arg(1) == 'add' || arg(2) == 'edit' || arg(2) == 'delete'))) {
    drupal_add_css(drupal_get_path('module', 'system') . '/admin.css', array('weight' => CSS_SYSTEM));
  }
...
}

We might not need to deal with it in this issue (because I suspect it's a straight-up bug that this is conditionally loaded on node pages like that?) .... but whatever its purpose, the current code seems to be making a half-hearted attempt to add it on all pages that might potentially be considered administrative.

Stumbled across this while trying to fix #676058: Taxonomy term edit page should use admin theme. I think this is a good idea as I had to define a page as an admin path AND do some messing in hook_menu to get admin pages displayed correctly in the overlay using the admin theme. hook_admin_paths should do it all in one.

I'll do a reroll based on my very similar work over in #615138: Some pages display in the overlay in a non-adminstrative theme. Rather than using "theme callback" I think it would make MUCH more sense to simply use hook_custom_theme() and set the theme based on the return of path_is_admin(). We've got a couple of different mechanisms all bumping into each other here.

I really like Dave Reid's splitting up the node_admin_theme variable into a hook_form_alter(), and I'll make sure I maintain all of his changes.

Here's a patch that merges Dave Reid's approach from #5 with my patch in #615138: Some pages display in the overlay in a non-adminstrative theme, and fixes the issue David_Rothstein brought up in #9. Good gravy our custom theme setting is out of control. ;-)

Basically here's the run-down:
- If you set an admin-URL through hook_admin_paths(), system.module will automatically enable the admin theme for that path through hook_custom_theme(). No more monkeying around with "theme callback" in hook_menu().
- "theme callback" still exists, but it's only used on pages that absolutely demand a particular theme, such as the blocks preview pages. "theme callback" takes precedence over hook_custom_theme().
- Node module now form_alters the system themes form rather than having "node_admin_theme" hard-coded into system.module, which sets a good precedent for other modules that need to add similar settings for certain URLs.

Updates tests to reflect "theme callback" overrides hook_custom_theme().

Great patch.

Only thing I can come up with

-      'theme callback' => '_block_custom_theme',
-      'theme arguments' => array($key),
+      'theme callback' => 'arg',
+      'theme arguments' => array('4'),

'4' => 4

We can't use 4 (an integer) because then the 4th argument will be passed into arg() instead of the number 4. We want to call arg(4), not arg('garland').

Ow I get it. I think that needs a comment; nerdcode :)

Thanks for the review casey, here we are with a good comment. I couldn't explain it in 70 characters, so it's two lines. If it takes two lines to explain, we definitely needed it. :-)

Excellent. I'll take a more careful look tomorrow.

Hm, I think all the issues I mentioned in #8 still apply.

In short, because there are essentially two different "philosophies" in the existing code - do we show 'administrative' things based on the page you're visiting alone, or based on your user role also? - I'm having trouble seeing how we can combine them without getting rid of one or the other or losing some existing functionality. As mentioned in #8, I think per-role (i.e., the overlay module method) probably makes a lot more sense, but that would be a big change for Drupal 7 at this point. It would be really nice to get this in, though, so trying to think of ways around that...

***

Also, why does this patch switch things so hook_custom_theme() no longer takes precedence over the per-page theme? Based on some of the uses cases (e.g. at #553944: Define hook_menu_get_item_alter() as a reliable hook that runs before the page is doomed) that doesn't seem like it will work - modules need to be able to alter things dynamically at the last minute. Maybe we can come up with something else to make the block theme stuff work, even if it's a bit of a hack? For example, leave the 'theme_callback' on the 'admin' menu item but make it call 'path_is_admin', and then have a separate hook_custom_theme() which only acts on non 'admin/' paths, and takes care of calling path_is_admin() for them? (Ugh - this would be a lot easier if we had a more flexible hook weight system, and could then just guarantee that the block module's hook was called later...)

-      'theme callback' => '_block_custom_theme',
-      'theme arguments' => array($key),
+      'theme callback' => 'arg',
+      // We need arg(4) to be called, which be the name of the theme. The use of
+      // a string ensures the literal number 4 is passed, not the 4th argument.
+      'theme arguments' => array('4'),

Yikes :) So, what's wrong with leaving _block_custom_theme() like it is before this patch?

David, I'm not seeing why you couldn't accomplish all those things you've described through using hook_admin_paths_alter(). This would allow you to decide "admin paths" per-role or by permission or any other criteria. The big thing I want to avoid is having two mechanisms for deciding what has admin-behaviors (which currently includes displaying the admin theme and using the overlay). What scenario is not currently supported?

Also, why does this patch switch things so hook_custom_theme() no longer takes precedence over the per-page theme?

This is because admin/* is defined as an "admin path", which includes the preview page for displaying a theme's block regions. Because it's critically important that such a page be displayed in the theme that it's intending to preview, this would override something that is merely an "admin path". This is a shift where we'll discourage pretty much any use of "theme callback" unless it's absolutely, critically important that page be rendered in a particular theme. However if a module wants to force certain a certain theme on pages, they can use hook_menu_alter() and add these theme callbacks as needed.

So in short:
hook_admin_paths(): Things that should be opened in the overlay and use the admin theme.
"theme callback" in hook_menu(): Things that require a particular theme, regardless of whether it pertains to the overlay or not.

Yikes :) So, what's wrong with leaving _block_custom_theme() like it is before this patch?

Eh, a function that does nothing but return its only parameter seems ridiculous.

@David_Rothstein did you have some time thinking this over?

I haven't enough knowledge of this theme selection system to make up my mind, but for the sake of simplicity I agree with quicksketch one mechanism is better than two. Especially if you can accomplish the same.

This might be easier to illustrate with pictures. Currently, with Drupal 7 and @ksenzee's patch at #615138-3: Some pages display in the overlay in a non-adminstrative theme applied, it is possible to set up your site so that administrators see this when creating content:

But regular users see this:

Which would be a decent setup for a number of kinds of sites. With the patch in this issue, though, that would not be the case. It would be possible to have both admin users and regular users see something like the second screenshot, but if you want admin users to see the first screenshot, you are forced to make regular users see this:

(which any site that allows non-administrators to create content is not likely to want to do)

I'm not sure how to get around that with the current patch. Given that contrib modules might be checking the 'node_admin_theme' variable as well, I don't think there's a clean way for the overlay module to fix this in hook_admin_paths_alter() - it would not know which paths to alter.

It does occur to me now that it could perhaps work if the overlay always set $GLOBALS['conf']['node_admin_theme'] = TRUE early in the page request for any user who had permission to use the overlay.... that would be a hack, but it would probably work. But overall, I still find it strange that with this patch, a site administrator who clicks on a checkbox that says "Use the administration theme when editing or creating content" also affects any other module which might be using the results of path_is_admin() for any other purpose.

Also, I think I realized why we probably shouldn't use arg() in hook_menu.

Using it means that any code which runs on a different page but happens to call menu_get_item('admin/structure/block/....') to load this specific path will not get the correct result if they try to evaluate the theme callback. Granted that is a severe edge case :)... but I think in general we want to be using arg() as little as possible, since it tends to prevent code from being reusable on pages besides the page it was designed for.

A different approach to flipping paths back and forth between admin and not-admin is to inspect the user rather than the path. I would say that a path is either an administrative path or it is not, no matter what user you're talking about. If you've decided that a user is not an administrator for some reason, and they should not have access to any administrative paths, you don't need to reference path_is_admin() at all. In which case, Overlay module should determine it's behavior based on the user AND path, not just the path alone. The path shouldn't be flipping back and forth between admin and not-admin based on the user.

So, rather than depending solely on path_is_admin(), there should be a user_is_admin() in addition. The use of overlay should depend on both of these, or there should be another wrapper around both of them, like overlay_applies($path, $user);.

function overlay_applies($path = NULL, $account = NULL) {
  $path = isset($path) ? $path : current_path();
  $account = isset($account) ? $account : $GLOBALS['user'];
  return path_is_admin($path) && user_is_admin($user);
}

EDIT: Code above is intended to be conceptual reference, it wouldn't actually work with the way Overlay determines its paths, afaik.

The overlay already has an 'access overlay' permission which controls who can see it - so I think it's similar to your suggestion...

However, I still don't see how that fixes things? The first screenshot I made above was with a user who had that permission. But the problem is, on the exact same site, a user without the 'access overlay' permission sees the third screenshot when your patch is applied. Most sites would rather have them see the second one.

Essentially, this is all related to what I said in #8. Basically, I think that not just Overlay, but all of Drupal, should do what you suggested - determine whether or not to show the "administrator view" by checking information about both the path AND the user. Right now, though, the admin theme feature in Drupal does it only by path. Changing that to also base it on the user (i.e. to also use a permission) as I suggested in #8 seems to me like a good way out of this logjam. However, I fear that it might be considered too big of a change for Drupal 7 at this point :(

Right, ideally system_custom_theme() would use same concept in deciding to use an admin theme. In lieu having an "user_is_admin()" function of what if we made the system_custom_theme() function only the admin theme based on user_access('access administration pages')?

In my opinion either reusing 'access administration pages' or adding a new permission would both be fine... The big question is whether a change this big to how Drupal deals with the admin theme would still have a shot for Drupal 7. It gets my vote, though :)

subscribing

Subscribing. Related issue: #678592: Admin theme is used is for adding taxonomy term, but not for editing taxonomy term.

The big question is whether a change this big to how Drupal deals with the admin theme would still have a shot for Drupal 7. It gets my vote, though.

It gets my vote too. Since we have hook_admin_paths() as a concept that extends beyond just selecting the admin theme, the current "Use the administration theme when editing or creating content" checkbox on the admin/appearance page makes no sense. Is #18 still the latest thinking on this, or does a new patch need to be rolled based on conversations since then? (sorry, I'm joining this late, so have some catching up to do)

I think that David and I agree that hook_admin_paths() should always return the same set of paths regardless of a user's permissions. I don't think we've particularly resolved whether or not the "Use the administration theme when editing or creating content" should affect the returned list of node_admin_paths(). It seems to me that it would.

To accommodate for the difference between admins and non-admins, we need to check a permission, most likely "access administration pages" (now labeled as "Use the administration pages and help"). In system_custom_theme() we would show the admin theme based on a the result of path_is_admin() && user_access('access administration pages').

Overlay module quite separately will determine which pages appear in overlays as a combination of path_is_admin() && user_access('access overlay').

We may want overlay module itself to implement hook_custom_theme() to enforce that the overlay use the admin theme (I'm not sure whether we want this or not). There could be a situation where a user has "access overlay" but not "access administration pages", in which case the overlay would open for node/x/edit screens but use the front-end theme.

Hang on I'll do a reroll with the current state of things. We still need to decide what to do with the 'node_admin_theme' variable and if that affects the return value of node_admin_paths().

Here we are. Changes since #18:

- system_admin_theme() now checks that a user has "access administration pages" permission before setting an admin theme.
- Added overlay_admin_theme() to enforce that the overlay always used the admin theme. I figured that made sense considering the permission is labeled "Access the administrative overlay", implying that it would use the admin theme.
- Removed my arg-as-the-theme-callback approach from block.module, so it's now unchanged.

This approach makes it so that all the situations David described in #23 are now possible.
Screenshot #1: Give users the "access overlay" permissions (and optionally the "access administration pages" permission).
Screenshot #2: Give users neither of these permissions.
Screenshot #3: Give the user "access administration pages" only.

I should also note that any module can decide to turn on or off the administrative theme (in the overlay or without it) also by implementing hook_custom_theme() and giving the module a weight of 1. Though I'm not particularly pleased with our adding admin.css in hook_custom_theme(), it could be manually disabled through hook_css_alter() if necessary for modules that want to turn off the admin theme at certain paths.

Took a quick look and this seems good - and #31 is a great summary. Some comments:

1. I think I am coming around on 'node_admin_theme'... Part of the issue is just the variable name (since it's now about more than just themes, the old name is somewhat inaccurate), but variable names can change or be ignored :) And also the UI text to some degree. Maybe we at least would want to reword that somehow.
2. The change in priority of hook_custom_theme() over the theme callback [edit: I meant the other way around!] is also starting to make sense... after this patch, the two places in core (at least that I know of) that would use the theme callback are the block demo pages and the system batch page, and both of those really do have a good reason to make sure they are not casually overridden, so perhaps that's an indication this is the right way to go. At the previous issue, some people were pretty adamant that they had use cases, e.g. Domain Access, where they wanted to always act last and override everything though, outside of the menu system (e.g. http://drupal.org/node/553944#comment-2259624), but I guess they still would have ways to do that via the menu system. We might want to get more input here. The fundamental problem is that a page can only have one theme at a time, yet everyone who tries to set it believes they should win out :)
3. For menu_get_custom_theme(), I wonder if we want to just still run hook_custom_theme() first and then let the theme callback override it afterwards (rather than skipping it sometimes) - or if not, then at least document that this hook is not guaranteed to run on every page, since I think that would be a bit unexpected.
4. I think testHookCustomTheme() lost something here... maybe we should first test that hook_custom_theme() actually works, and only then test that it is overridden by the theme callback?

5. +function system_custom_theme() {
   +  if (path_is_admin(current_path()) && user_access('access administration pages')) {
   

   Perhaps change the order in that if statement: I think there are likely pages where path_is_admin() would never need to run and it's probably not the cheapest function ever, so no reason to force a call to it if they don't even have access anyway.

6. +  // A menu rebuild is needed in case admin paths might be different because
   +  // of changed administration theme settings.
   +  variable_set('menu_rebuild_needed', TRUE);
   

   I don't think this is needed anymore, is it? Might be a carryover from previous patches?

7. Probably will need some doc changes for 'theme callback' (in hook_menu docs) and for hook_custom_theme().
8. Grep for _node_custom_theme() ... I think there are still other modules that use it?

Any news? Are we still going to change this for D7?

@David_Rothstein
Not sure if this still is due for D7, but to give you a head-start (#615138: Some pages display in the overlay in a non-adminstrative theme) I rerolled patch from #33 and included your suggestions.

3. I documented that this hook will not always be called.
4. Added an extra test
5. Changed into suggested order
6. -- no idea
7. Updated docs
8. book.module/translation.module

The latest overlay code now makes it possible per user to disable the overlay, so overlay_custom_theme() should consider that. Also, overlay_init() considers path_is_admin() as well, and closes the overlay if not an admin path. Not sure we still want to display an admin theme if not an admin path, it should not actually display in the overlay as coded currently. All-in-all I found this stop-gap patch (attached) to solve the immediate problems with #615138: Some pages display in the overlay in a non-adminstrative theme caused by #662868: user edit link should open in a overlay, and its very similar to a small part of the ongoing patch, so thought this is the best place to share. I think improvements from this patch (ie. per user overlay switching code) would be good to roll into the ongoing patch too. Not intending to interrupt, just sharing a small patch that is working for my use case.

Gábor: Might be worth posting at #615138: Some pages display in the overlay in a non-adminstrative theme as well?

Casey: Thanks for the reroll. This is looking pretty good. I will hopefully get a chance to work on this in the next few days - still hope we can get this into Drupal 7 :)

I am starting to think it would be a good idea to introduce a new 'view the administrative theme' permission here after all (rather than reusing 'access administration pages'). The reason is that this patch kind of already introduces a paradigm shift, by allowing the admin theme to differ by user, and at this stage of the cycle we want to minimize the paradigm shift. Since 'access administration pages' is used by core (and especially contrib) for access to other functionality, it might be better if we separated it out so that this new way of treating the admin theme wasn't forced on people, and sites (especially those upgrading from Drupal 6) could safely assign this permission to all users if they wanted to preserve the old paradigm.

If we have a clear permission controlling the display of the admin theme, then it's not clear this patch needs any overlay code at all - could really go either way. The main reason for the overlay code was to enforce consistency (so you don't jump between themes while navigating the overlay), but we already get that automatically as a result of the overall approach here.

Subscribe

Subscribe

I think this needs to be "major" priority since #615138: Some pages display in the overlay in a non-adminstrative theme is a major issue blocked by it, and it has become even more important with #896364-293: Screen reader users and some keyboard only users need a clear, quick way to disable the overlay having landed. Accessibility, usability, and theme DX all benefit greatly when pages in the Overlay use the Administration theme.

Here is the patch in #37 rerolled against HEAD. I rolled in Gábor's change at #39.

Thanks mrfelton. Here's another reroll that fixes one of the tests (Batch API theme) by logging in as a user. With the patches from this issue applied, users are not shown the administrative theme unless they are an administrative user. I find this to be a significant improvement, since it also means that the front-end theme is shown to users when they get an access denied page at an administrative path.

Good to see this revived! I am assigning this to myself and I will work a bit later tonight on incorporating the remaining feedback and fixing the remaining tests. We definitely need to get this finished ASAP...

(If someone else is actively working on it at the moment, please post back here so there is no duplication of effort.)

Regarding adding the code from #39, if we do this right we should not need any overlay-specific code in this patch at all (since this issue solves the problem more generally). And actually, the code added in overlay_custom_theme() is problematic at this point; it is too much of an API change because it means that any modules which put code in hook_init() to try to suppress the overlay would not quite work as expected anymore. Ironically, that change (which by itself would be the only solution left for #615138: Some pages display in the overlay in a non-adminstrative theme in the event that this issue gets bumped to D8) is probably more of an API change than the other changes in this issue. So I'm definitely convinced that (a) we should proceed with this issue for D7 and not bump it to D8, but (b) not add the overlay_custom_theme() in this patch.

Here's another reroll that fixes one of the tests (Batch API theme) by logging in as a user. With the patches from this issue applied, users are not shown the administrative theme unless they are an administrative user. I find this to be a significant improvement, since it also means that the front-end theme is shown to users when they get an access denied page at an administrative path.

Actually, that feature already exists in current HEAD - it is not introduced by this patch. So maybe there is another explanation for the test failure? I will check.

Oh, I get it... @quicksketch's changes to the batch API test are definitely correct, as well as his rationale.

The part that was wrong is the statement about this patch's effect on access denied pages (even in current HEAD, those do already show the front-end theme). But the batch API tests are for a user who has actual access to the page, so it's a different scenario.

Here is a reroll that should fix the test failures from above (we'll see if there are any new ones). It also fixes all the remaining issues from earlier feedback, and tries to absolutely minimize the API change this late in the cycle. In more detail:

• Used a new permission ("view the administration theme") rather than reusing an existing one. This means we don't have to worry about interfering with any modules which might already be overloading "access administration pages" for other purposes, and also gives us a simple upgrade path to preserve the current behavior on existing sites.
• Removed the overlay-specific code; without it the overlay will respect the above permission just like the rest of Drupal.
• Removed the code that moves the 'node_admin_theme' setting from system.module to node.module (it seems reasonable but is not related to this issue, and it is too late in D7 to change form submit handlers unless there is really a good reason to).
• Per #35.3, made it so hook_custom_theme() is always called even when it is going to be overridden. This is more consistent overall, especially this late in the cycle where people may have already written code that expects this hook to always run.
• Fixed all modules that previously used the _node_custom_theme() theme callback, to now check the value of the 'node_admin_theme' variable before returning the path list in hook_admin_paths() - thereby preserving their previous behavior for when the admin theme is used.
• Fixed the drupal_add_css() call for the admin theme CSS file (which was broken).
• Documentation fixes.
• Test fixes.

Overall, I'm pretty happy with this one. I can try to write up a general issue summary later as we try to make the case for getting this into D7 :)

Used a new permission ("view the administration theme") rather than reusing an existing one. This means we don't have to worry about interfering with any modules which might already be overloading "access administration pages" for other purposes, and also gives us a simple upgrade path to preserve the current behavior on existing sites.

I'm not really a fan of this change. When would you access pages underneath /admin (as granted by "access administration pages") and not have the admin theme? It seems like if you're on an admin page, it's in the admin theme and that should be that. This patch also seems like it's going to be increasing the chances of the administrative overlay not showing in the administrative theme (say a user has "access administration pages" and "access overlay" but not "view the administration theme"). I think this is unnecessary splitting out of a permission that already covers the use-case we want, and we're making administrators check off two boxes instead of one.

# Removed the overlay-specific code; without it the overlay will respect the above permission just like the rest of Drupal.

The overlay-code should be restored, since the "administrative overlay" should use the administrative theme at all times, not based on a separate permission.

Sorry I didn't mean to unassign David. Thanks for the reroll and fixed tests, I couldn't figure out that menu breadcrumb one for the life of me.

I could go either way on the separate permission. However, my main motivation was that I think keeping this separate really increases the chance that this can still make it into Drupal 7.

There is also a general argument in its favor. While it's true that if you've granted someone "access administration pages" you almost certainly also want them to see the admin theme, the reverse is not necessarily true. You might have an administrator with very limited permissions who has access to perform only one specific administrative task, so you want them to use the admin theme but do not want to grant them "access administration pages" due to the other pages this would automatically allow them access to (particularly some pages provided by contrib modules). One could argue that contrib modules shouldn't be reusing this permission very often, but I believe many of them do. Also, having "view the administration theme" as a separate permission provides an unambiguous way to figure out how to show the admin theme to certain users (you don't have to guess what controls it).

As for the overlay, if we have a separate permission then the overlay should definitely respect it. (There's no requirement that the overlay only ever use an admin theme - it can't anyway since the site might not even have an admin theme enabled - just that it be consistent and predictable.) However, I agree that if we do not have a separate permission, we have to put back something like that overlay code. The resulting complexity with the way overlay_custom_theme() and overlay_init() interact and how other modules are supposed to work with them is something I'd really like to avoid.

In any case, here's a reroll of the above patch that should fix those remaining test failures.

You might have an administrator with very limited permissions who has access to perform only one specific administrative task, so you want them to use the admin theme but do not want to grant them "access administration pages" due to the other pages this would automatically allow them access to (particularly some pages provided by contrib modules).

Okay, fair enough. The exact reason I got into the original issue at #615138: Some pages display in the overlay in a non-adminstrative theme was because Webform configuration tabs are just such a scenario. If the "edit" page of a node is in the admin theme, so should the "Results" and "Webform" tabs. So I could definitely see a situation where the node edit page (and similar Webform pages) would be in the admin theme even if the user doesn't have access to the admin section (or the overlay).

Regarding the Overlay, I still think that overlay.module should enforce the use of the admin theme within the overlay. Other modules can override this behavior by implementing hook_custom_theme() just like anything else. However this puts the "view the administrative theme" at odds with "view overlay" permissions, since a user could have "view overlay" without "view administrative theme". We don't really have an option to enforce the overlay using the admin theme if we're adding this new permission.

Hm, maybe the overlay code change is not as intrusive as I thought. I freely admit I do not understand how that part of the overlay works, with the $_GET['render'] stuff :) I think it does put a bit of an extra burden on modules which want to interact with the overlay to have to do so in both hook_custom_theme() and hook_init(), but it wouldn't be the end of the world.

However, it also sounds like we're in agreement that doing that would conflict with adding a separate "view the administrative theme" permission. My suggestion would therefore be to not change the overlay code here. If we want to discuss the overlay more, we could reopen #615138: Some pages display in the overlay in a non-adminstrative theme afterwards, but the sooner we get this issue RTBC (and the simpler the code in it is), the better. We are very close to the deadline on this one :)

And I think the current patch's behavior is pretty reasonable on its own. In current HEAD, as you browse around the overlay you wind up switching between the front-end theme and admin theme depending on which page you visit; the lack of consistency is extremely disconcerting, and there is no way to always see the admin theme. With the latest patch as it stands, all users are guaranteed a consistent experience; they either always see the admin theme in the overlay or always don't. If you want to change what they see, you just grant/remove the "view the administration theme" permission as appropriate.

And although some front-end themes look horrible in the overlay, others (like Bartik) look fine, so you might actually want to configure your site that way. Suppose you have an "editor" role and you want them to edit content in the overlay, but you also want them to still see the front-end theme (so that the "Preview" button on the node form actually shows the content in the style it will be displayed on the main site). Then you might conceivably grant them "access overlay" but not "view the administration theme" permissions, even while other higher-level admins on the site use the admin theme. It's a bit of a stretch, but I don't think it's an impossible scenario, so it shouldn't hurt to allow it.

Suppose you have an "editor" role and you want them to edit content in the overlay, but you also want them to still see the front-end theme (so that the "Preview" button on the node form actually shows the content in the style it will be displayed on the main site).

I think the 90%+ scenario would be that if you're avoiding showing the user the admin theme, you'd probably just disable the overlay for that role. I'm fairly sure no one expects the admin overlay to ever open in the front-end theme (unless there isn't an admin theme defined at all).

I think we may need some second opinions on this.

OK, I really want to get this RTBC before Thanksgiving :) What can we do to make that happen?

Trying to summarize the remaining issues, it sounds like there's agreement that (overlay module aside), a separate "view the administration theme" permission is better for a number of reasons.

I would posit that there is also (perhaps?) agreement that the overlay module is enough of an edge case here that we shouldn't be designing default Drupal core behavior around it, especially when there are lots of use cases that point in the other direction.

Given that, the remaining question is whether it is better to:

• Have the overlay module respect the "view the administration theme" permission, but with the cost that when you grant someone overlay access, you probably will also need to go and grant them that permission by hand.
• Have the overlay module ignore the "view the administration theme" permission, and do its own thing.

Any thoughts, anyone?

In the meantime, here is an updated patch (the taxonomy module got a new 'theme callback' pointing to the admin theme in the interim, which we need to remove).

Oops, looks like system_update_7066() got added in the meantime also, so that patch won't work. Let's try this one.

This issue needs a summary for potential reviewers and committers, and I promised one above. (The last point we're discussing shouldn't affect it much.)

Summary

About a year ago, the overlay went into core with a new concept of what it means for a Drupal page to be administrative. Around the same time, a bunch of changes went into core concerning the way a page's theme is determined. Those two sets of changes were somewhat contradictory and never played nicely together. The overlay was always intended to be able to produce something like the first two screenshots in #23 (where administrators could create content in the overlay using the administrative theme, while on the same site, regular users could create content using the front-end theme). But due to those conflicts, it never worked. And a number of other bugs resulted as well.

This patch fixes all those things up. It does that by making the existing hook_admin_paths() the one true way for modules to declare which of their pages are administrative, and the existing path_is_admin() the one true way to check if a page is administrative. The rest of core, including the theme system, now respects that; the administrative theme is shown on exactly the same pages for which path_is_admin() returns TRUE. However, the administration theme now only shows to users who have permission to see it, not necessarily to all users.

Bugs fixed by this patch

• The overlay no longer displays some random pages in the front-end theme and others in the admin theme (#615138: Some pages display in the overlay in a non-adminstrative theme). This (very confusing) situation is unavoidable with the current state of core, but fixed by this patch. For example, currently in core, paths like user/[uid]/edit are declared administrative by hook_admin_paths(), because an administrator editing someone else's account is clearly doing something administrative and we want it inside the overlay. However, they also currently display inside the overlay in the front-end theme, which looks really crazy. We can't simply fix that by setting them to display the admin theme, because that means regular users who are editing their own profile pages would see them in the admin theme also, which makes no sense. This patch fixes that by only showing them in the admin theme to administrators, not to all users.
• Similarly, as shown in the screenshots in #23, the standard install profile currently sets content creation pages to use the admin theme, but since this forces all content creators to see the admin theme, it only makes sense for "brochure-ware" type sites. The standard install profile is certainly intended to showcase a much more flexible range of Drupal sites than that. With this patch, the standard install profile allows administrators to create content in the admin theme, but if you grant end users permission to create content, they will still be able to do it via the front-end theme.
• Finally, this patch fixes a bug with the way in which hook_custom_theme() and hook_menu()'s "theme callback" functions determine the theme that the page will be displayed in. Currently in core, hook_custom_theme(), which runs on all pages, always takes final precedence. The problem is that that's too blunt of a tool; there are several pages that absolutely make no sense when displayed in a different theme (for example, the block demo pages must always display in the theme they are demoing, and a batch progress page must always display in the same theme as the page that started the batch). Currently, hook_custom_theme() implementations will accidentally clobber the theme on these pages as well, when trying to set the theme dynamically across the site (e.g., if they try to set the theme by user role). As a result of the administration theme being handled differently, this patch switches the order in which the above hooks take precedence, so modules implementing hook_custom_theme() can freely change the theme without accidentally clobbering pages that absolutely must display in a certain theme. The only things left using "theme callback" functions in hook_menu() are those that really do need to carefully control the theme, and they are now able to do so correctly.

Semi-API changes

This patch makes a couple changes that are technically API changes, but overall are needed to fix the above bugs, and in sum total, only should have a minimal effect.

• The order in which hook_custom_theme() and hook_menu() "theme callback" functions are given precedence is now reversed. But as described above, this actually fixes a bug; modules that implement hook_custom_theme() can still do just what they were doing before, only it will work better. For example, I took a look at the D7 development code for http://drupal.org/project/themekey (likely the contrib module that has the most advanced implementation of dynamically setting the theme on different pages). That module is already using hook_custom_theme() for everything, and therefore wouldn't need to change as a result of this patch. In fact, it currently needs to have some hackish code to work around the bug described above (it checks to see if you're on a block administration page and bails out without switching the theme if you are), but this code is not only hackish, it's also incomplete (it doesn't know to bail out on batch progress pages, AJAX requests, or other pages that absolutely must control the theme being used, nor could it ever know that, because it can't know all possible such pages defined by all possible modules). This patch makes it so it no longer has to try, and that module's hook_custom_theme() will work right out of the box.
• A number of contrib modules which define node-related paths currently do what core does and define a "theme callback" function pointing to _node_custom_theme(). That is no longer necessary after this patch. However, if they don't remove that line of code, nothing bad will happen (the function has been removed from core, and the menu system already knows to just ignore the theme callback in the case where it points to a function that doesn't exist). So basically, as long as they already updated their module to D7 and are properly using hook_admin_paths(), they won't need to change anything. If they want, they can preserve their module's exact behavior by making the code in hook_admin_paths() conditional on the "Use the administration theme when editing or creating content" checkbox on the Appearance page (i.e., the existing 'node_admin_theme' variable), but nothing bad will happen if they don't, and for many of them it probably doesn't make sense to anyway.

  In summary, the most that these contrib modules might need to do in response to this issue is something like Translation module does in the patch:

  --- modules/translation/translation.module	14 Nov 2010 21:04:45 -0000	1.90
  +++ modules/translation/translation.module	23 Nov 2010 05:14:50 -0000
  @@ -65,7 +65,6 @@ function translation_menu() {
       'access arguments' => array(1),
       'type' => MENU_LOCAL_TASK,
       'weight' => 2,
  -    'theme callback' => '_node_custom_theme',
       'file' => 'translation.pages.inc',
     );
     return $items;
  @@ -89,10 +88,12 @@ function _translation_tab_access($node) 
    * Implements hook_admin_paths().
    */
   function translation_admin_paths() {
  -  $paths = array(
  -    'node/*/translate' => TRUE,
  -  );
  -  return $paths;
  +  if (variable_get('node_admin_theme')) {
  +    $paths = array(
  +      'node/*/translate' => TRUE,
  +    );
  +    return $paths;
  +  }
   }
  

  But nothing bad happens if they don't do the first hunk, and the second one isn't always necessary either.

Fixing the broken test.

FYI: Following up on the above summary, I also took a look at what the Domain Theme module (part of http://drupal.org/project/domain) is doing. It too currently has some code in hook_custom_theme() to try to work around the above bug, and it too shouldn't be affected by this patch (it could remove the code after this patch if it wanted to, or it could choose not to).

That was the other module that I remember being an issue when we previously were trying to figure out what order all these theme hooks should be running in.

David's summary is excellent and says it all. I spent some time with the patch in #63 and it looks very good. If we had a persistent cache of the results of hook_admin_paths(), the variable_get()s in the hook implementations might cause a problem. But apparently we don't have caching there (who wrote that crap?? ahem.) so if and when we start caching those results, we can worry about when to clear the cache.

I also agree with the logic of page-specific theme callbacks taking precedence over hook_custom_theme, especially since we have the nuclear option of hook_menu_alter in case webchick decides to write any more April Fool's modules ("Screw you, hippie. The theme is what I say it is!") Also, I am very glad to see that David has looked at some of the trickier theme-switching D7 contrib modules and that this won't break them.

FWIW I don't think it's important to allow non-admin themes in the overlay - it seems fine to to just force the admin theme. But no matter what we decide on that, we should get this patch in as is and worry about overlay theme-switching separately, in a point release if necessary.

#61 summary and #63 patch are excellent. As per #44, this is a "major" issue, so I'm thrilled to see such an excellent solution for it.

Re #61, Semi-API change #2: Even though it's technically a BC break, I don't consider the removal of a function that starts with an underscore to be an API change. By definition, functions that start with an underscore are considered private implementation, not part of the public API. Modules that are using this function are violating the rules, and if they need to be updated to no longer violate the rules, so be it. I guess it's cool that our menu system is robust enough to not error when the 'theme callback' doesn't exist, but even if it didn't, I would support this patch. That this patch makes it so that the book and translation modules are fixed to no longer call a private node module function is a nice bonus.

Re #61, Semi-API change #1: Changing the order of precedence between hook_custom_theme() and 'theme callback' is indeed an API change, but I fully agree that it's a desirable one for all the reasons mentioned. In addition to what's been said, we also have a hook_menu_get_item_alter() if someone (webchick) wants to go crazy (one day out of the year) in overriding 'theme callback' based on a dynamic condition.

FWIW I don't think it's important to allow non-admin themes in the overlay - it seems fine to to just force the admin theme. But no matter what we decide on that, we should get this patch in as is and worry about overlay theme-switching separately, in a point release if necessary.

I think ksenzee is referring to the remaining question in #59. I think what #63 does is exactly right. If permissions are set to allow "Access the administrative overlay", but not allow "View the administration theme", then the Overlay is shown with its contents rendered with the front-end theme. True, most front-end themes might not look good, or have accessibility problems, in the Overlay, but this problem can be easily fixed by the site builder granting permissions appropriately. We shouldn't bend over backwards to restrict this combination of permissions, just like we shouldn't bend over backwards to restrict someone from enabling the Overlay and setting "Default" as the administration theme. But anyway, I agree that this can be discussed further in a separate issue.

The only flaw I could find with #63 is:

+++ modules/system/system.module	24 Nov 2010 15:05:10 -0000
@@ -1840,9 +1842,6 @@ function system_init() {
-  if (arg(0) == 'admin' || (variable_get('node_admin_theme', '0') && arg(0) == 'node' && (arg(1) == 'add' || arg(2) == 'edit' || arg(2) == 'delete'))) {
-    drupal_add_css($path . '/system.admin.css', array('group' => CSS_SYSTEM));
-  }
...
+++ modules/system/system.module	24 Nov 2010 15:05:10 -0000
@@ -1894,6 +1893,16 @@ function system_add_module_assets() {
+function system_custom_theme() {
+  if (user_access('view the administration theme') && path_is_admin(current_path())) {
+    drupal_add_css(drupal_get_path('module', 'system') . '/system.admin.css', array('group' => CSS_SYSTEM));
+    return variable_get('admin_theme');
+  }
+}

This means someone without "view administration theme" permission, but with permission to access administrative pages (like the Status Report page), would not be getting the CSS from system.admin.css. This makes no sense. The styles in system.admin.css are not coupled to any specific theme. If you set Default as your administration theme, you should still get that CSS on administrative pages (that's what happens in HEAD and with #63). Therefore, the only thing determining whether that CSS should be added is whether the page is administrative, not what theme is used. This patch fixes this one thing, and just that.

With the patch, this time.

Bending the rules here, slightly, and RTBC'ing. Really, though, this is David's patch, not mine, and ksenzee already RTBC'd it in #65. My only contribution to it, as per #66, is not making system.admin.css conditional on user_access('view the administration theme'), which just means making it work like HEAD already works, rather than something new. Meanwhile, this is our last weekend before RC, and this really should make it in before then.

This makes a lot of sense so I committed it to CVS HEAD.

The only question that came to mind is whether we need to update the install profiles -- I don't think that is a requirement.

Great, thanks. I agree the change in #67 is correct.

What this change does mean is that we now call path_is_admin() on each non-cached page request (in the earlier patches we were hoping to only do it for administrators). I did some quick micro-benchmarks via the script below (run on a site using the standard install profile), and the results showed that the cost of calling path_is_admin() without static caching (i.e., the first call on a given page request) is something like 1.8 msec on my computer...

I don't think that's a showstopper, but thought it was worth noting.

define('DRUPAL_ROOT', getcwd());
require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
$iterations = 5000;
$times = array();
for ($i = 0; $i < $iterations; $i++) {
  drupal_static_reset();
  $start = microtime(TRUE);
  path_is_admin(current_path());
  $end = microtime(TRUE);
  $times[] = $end - $start;
}
$total = array_sum($times);
$average_ms = 1000 * ($total / $iterations);
print "Took $total seconds to perform $iterations iterations (average of {$average_ms} msec per function call).";

I don't think you meant to change the status, did you?

Oops, yes, that was a crosspost. Glad this was committed :) If anyone is interested in following up on the performance issue, they can look into the above.

Dries, what updates were you thinking for the install profiles? I think the "view the administration theme" permission is better if we keep it as is and don't give it to non-administrators in the install profile (that way we keep the D7 behavior of not showing regular users the admin theme even on an "access denied" page and the like). The only reason to grant it on updates was to not break anything on a site upgrading from D6.

Oh, I just realized something - maybe that update function is not a good idea after all. For a site using an admin theme that updates from D6, I think that means that regular users will suddenly start seeing the admin theme when editing their user account pages (which definitely did not happen in D6).

Since there's no way to exactly preserve the D6 behavior anyway, maybe it's better if we don't have that update function and instead print out a message, telling sites that were using an admin theme that they will need to configure this permission manually for their administrators. Since Garland is the only theme that carried over from core D6 to core D7, people with a separate admin theme on their D6 site are already going to have to do some manual steps anyway after the upgrade (to get their themes back in place), so we can just add this to the instructions for that.

Thanks for the commit Dries and your work David. Great to see this fixed. I'll re-port Webform again soon and take a look. :)

Following up on #73, I'm pretty convinced I messed up with that update function :( There is really no way we can reliably decide which roles on a site upgrading from D6 might need the new permission, and the ones I did decide on in that update function are very likely to be wrong.

Here is a fix. It seems to be the current pattern that we drupal_set_message() things during D6->D7 update if it represents a change in behavior that the administrator is likely to want to configure manually, so that's what I did here.

dsm()-ing in the update is what we did in D5->D6 as well, like "hey, we now have update.module, you should enable it", so I think that should be good practice.

+++ modules/system/system.install	29 Nov 2010 04:44:08 -0000
@@ -2927,14 +2927,14 @@ function system_update_7066() {
+    drupal_set_message('The new "View the administration theme" permission is required in order to view your site\'s administration theme. You can assign this permission to your site\'s administrators on the <a href="' . url('admin/people/permissions', array('fragment' => 'module-system')) . '">permissions page</a>.');

We need t() or st(), right?

Not during updates, no. At least, we didn't in D6, and in D7 there is no consistent implementation but most of the drupal_set_message() calls don't.

http://drupal.org/node/322731
#871054: Inconsistent use of translated strings in update functions

Ok. Leaving translation to that other issue then.

Hm? I don't understand why we do this cheap hack.

Why can't we do something like:

if (variable_get('admin_theme')) {
   $role_names = user_roles();
   $role_permissions = user_role_permissions($role_names);
   foreach ($role_permissions as $rid => $permissions) {
    if (in_array('administer site configuration', $permissions) {
      user_role_grant_permissions($rid, array('view the administration theme'));
    }
  }
}

Tough I'm not really comfortable that suddenly just before RC1, switching on an admin theme now requires toggling two completely different settings in completely different parts of the admin interface. That's going to invalidate documentation, and I don't understand why it was required to fix the actual bug (theme callback vs. hook_admin_paths()) here.

That seems to make a lot of sense.

The attached patch implements a version of that (I used 'access administration pages' instead, based on the earlier discussion.) I think we still need to print the message though. What we're implementing here automatically is just an educated guess (and a good one), but it doesn't get the "editor on a brochureware site" scenario, or other roles that might be considered administrative but don't have that permission.

Why do you think a message here is a cheap hack? Oh wait, probably because it is :) But it's the same hack we already use in lots of other update functions. My patch to make that a bit cleaner is #774316: Remind users about new Drupal 8 features after migration from D7 :)

Tough I'm not really comfortable that suddenly just before RC1, switching on an admin theme now requires toggling two completely different settings in completely different parts of the admin interface. That's going to invalidate documentation

I think in most scenarios, if your site has administrators who are not user 1, then you've already gone through and clicked a bunch of checkboxes on the permissions page for them (or get that automatically via the admin role feature), and this is just one of the ones you already would have checked off.

I don't know of any specific documentation that this invalidates, but I can fix it if there is.

Another thing we could do is (before the string freeze) make a small change to the description on the "Administration theme" dropdown. Currently it's:

    '#description' => t('Choose "Default theme" to always use the same theme as the rest of the site.'),

It could become something like:

    '#description' => t('Only applies to users with the "View the administration theme" <a>permission</a>. Choose "Default theme" to always use the same theme as the rest of the site.'),

I thought about doing that in the original patch. I think there was a reason I decided not to, but I can't remember what it was.

I don't understand why it was required to fix the actual bug (theme callback vs. hook_admin_paths()) here.

Because we have paths like user/%user/edit which are already in hook_admin_paths() but which definitely wouldn't make sense to show in the administration theme to all users of the site.

Fixing a silly typo in the code comment.

The previous update function went out with RC1 so I guess it will slightly annoy some percentage of people who are actually updating their sites in line with the releases. Oh well. We should fix it before RC2, at least :)

I wonder if we should move the part that actually assigns the permission outside of the variable_get('admin_theme') check. It's harmless to run on other sites, and it makes things easier for these upgraded D6 sites if they later decide to turn on an admin theme (to try out Seven, for example).

Continuing to review my own patch :) I think it does make sense to do what I suggested in #84, so here it is.

#85: admin-theme-followup-669510-85.patch queued for re-testing.

I think this is the closest we can get to help automatically migrate users and then telling people how they can act from there. Translation is not to be used here per http://drupal.org/node/322731, and we use this pattern to inform people about things they should do after update elsewhere.

Committed to CVS HEAD. Thanks.

Well, I never caught this the first time around. Is this an API change that should be announced? Is the summary in #61 the best summary, or do we need another?

Thanks,
-Randy

#61 should still be accurate, yes.

The short version is:

1. If you were using the 'theme callback' in hook_menu() to set some of your module's pages to use the admin theme, you should remove that code. (Most probably, you were doing this for a path like node/%node/some-admin-action-provided-by-my-module and were using the core _node_custom_theme() function as the callback; in that case, the code will stop working on its own, but is still worth removing.)
2. Instead, make sure that those pages are listed in hook_admin_paths() - which they should have been listed in already anyway.

Module authors doing more advanced theme switching behavior might care about more of the details in #61, but most others probably won't.

I guess having an admin theme is a new D7 feature, so this doesn't need to be noted on the 6/7 update guide pages?

Hmmm...
http://drupal.org/update/modules/6/7#custom_theme probably needs this information added/corrected. Tagging appropriately.