Allow modules to specify per-page custom themes in hook_menu()

Here is the patch - it seems to work fine for me.

A side effect of the patch is that if you have the admin theme enabled and visit /admin as a user who does not have privileges, the "access denied" page shows you the main site theme rather than the administration theme (like it used to) -- I thought maybe this was a feature regression, but according to #137198: Write tests for: Admin theme visible at /admin, even if user can't access it may actually be a bugfix :)

There's also some unfortunate code surrounding the modules/system/admin.css file that this patch would like to remove but can't right now. I'm actually not sure what the point of that file is - it seems like rather than including it on every single admin page, it could be broken up into a few separate CSS files that get called only on the particular pages they are relevant for - but that's probably a topic for another issue.

The last submitted patch failed testing.

I don't think this approach would have occurred to me, but I like it a lot. It's clean and sensible. The test exceptions are because $themes[$custom_theme] isn't always set. Adding an isset() around it solves the problem.

However... I'd love to get rid of the global $custom_theme entirely. I'm attaching a patch that adds a menu_get_custom_theme() function and converts $custom_theme to a static variable inside that function. I don't love the function signature -- suggestions welcome -- but it might be better than keeping a global variable around.

Either way, this patch should go in. The custom theme system is pretty broken without it, and hitching a ride on the menu system is a nice solution.

This is a bit nit-picky, but the new function headers do not conform to our current doxygen standards. I realize we have a lot of doc that doesn't conform, but I think at least that new patches should do so.

http://drupal.org/node/1354

The attached patch fixes the docblocks (in places where it makes sense to do so).

It also fixes an issue with the site offline check (administrators should still get custom themes if they are logged in when the site is offline, but the original patch prevented that).

Style-wise, I like the current patch.

Cool.... anyone willing to go out on a limb and mark this RTBC? :)

It's not a tiny patch, but it's pretty straightforward, and code freeze is a comin'...

This is good, I especially like moving the crap out of system.module which really doesn't belong there relating to nodes, etc.

I find the theme_callback thing a little weird, but I suppose that is the best way to do it since you can't really get any logic in hook_menu.

Small feedback:

-function _menu_site_is_offline() {
+function _menu_site_is_offline($check_only = FALSE) {

I would recommend making a separate function _menu_allowed_offline();

That point isn't a big deal, just a more clear API IMO.

Otherwise, RTBC

DTALOA (Damn, that's a lot of acronyms)

Thanks Jacob.

I wanted to do the _menu_site_is_offline() thing the way you suggested, but due to the way the nested logic currently works in that function, I think you'd basically wind up with code duplication -- we'd have to have two separate functions that each kept track separately of what the various permissions/pages are that lead to the site being offline. So I thought it was preferable not to do that.

We could of course easily do something like this:

Not sure if you think that's worth it or not - or any alternative suggestions?

Hmm... I'm too busy to look in detail at the code and the un-spaghettifying that would have to be done, and I trust that what you've got is the best path forward for now, and certainly shouldn't hold up fixing this bug.

The last submitted patch failed testing.

Patch updated to chase HEAD.

This hasn't changed in any meaningful way since the version Jacob marked RTBC, so now that the tests have passed I'm taking the liberty to mark it as such again...

I reviewed this patch and tying theme logic onto the menu system does not "feel" like the right way to go for me. These are decoupled sub-systems that act independently from each other.

Coupling them means that a module would be able to define a theme to load for a menu callback. But themes can be managed independently from modules. So all modules can specify whatever, but if whatever is not available, the whole approach fails.

That, to start with.

Reverting to needs review, but in reality, I think the approach taken is a won't fix.

Hm. This approach is interesting. It simplifies a lot of really gnarly code and I can see how it would fix a bunch of bugs with the admin/non-admin theme split. We're attaching more baggage to the menu system, though, and mixing display logic with functional logic. Makes me kind of uneasy...even though I can see the power in this approach. Theme-per-site-section becomes *really* easy this way, among other things.

This could use a review by someone who does not have an Acquia employee number. ;) I'd especially love if chx could chime in here as a menu system maintainer.

Coupling them means that a module would be able to define a theme to load for a menu callback. But themes can be managed independently from modules. So all modules can specify whatever, but if whatever is not available, the whole approach fails.

@sun: I'm not sure how this comment specifically applies to this patch. There are already two modules in core that define themes to load for particular menu callbacks. They just do it in hackish, buggy ways -- in hook_init() in one case, and within the menu callback function in the other. The patch simply provides them a cleaner way to do what they are already doing now.

Also, the block module already needs to deal with the situation of finding out what themes are available and making sure it does not try to use a theme that isn't there. It does so by calling list_themes() and then checking the status of each theme it finds there. I agree this could certainly be improved, but it's existing core code and not touched by this patch one way or the other.

We're attaching more baggage to the menu system, though, and mixing display logic with functional logic.

@webchick: I would compare this in some ways to the title callback, which is already in the menu system and which is (essentially) display logic. The menu system is in charge of managing pages, and one of the defining elements of a page in Drupal is the theme that will be used to display it, so I think maybe it isn't too much of a stretch.

An opinion from @chx would indeed be interesting here :)

So all modules can specify whatever, but if whatever is not available, the whole approach fails.

It's also worth pointing out that in case a module ever tries to use a theme that isn't available, I'm pretty sure nothing terrible will happen - it should just fall back on using the default theme in that case (at least that was the intention of the code in the patch - I haven't actually tried it).

Will check this tomorrow.

Bump :)

I realize it's a busy time, but it would be great to make progress on this before the code freeze if we can. The alternate methods for fixing all the admin theme bugs (that don't involve an API change) are likely to be really really ugly and fragile, if they even work at all....

I agree with @sun that it is not a vary logical assocaition, but the coupling that exists now is far worse. In fact, pretty bad hardcoded business rules in different modules which should have nothing to do with each other.

I personally don't see a big problem with coupling url x = theme y through the menu system, because that is the main reason to change themes. However, I think to make a cleaner association (because as @sun mentioned, modules can specify themes that don't exist), the themes could be keyed.

We have two obvious ones (admin and default). That's probably sufficient, but we could also allow modules to supply a list of themes (like font face in CSS selectors)

$theme_options[] = array('bluemarine', 'garland', 'admin', 'default').

meaning, use bluemarine if it is available, else garland, else the admin theme, else the default theme.

That being said, I don't think that should stop this patch from getting in, it is any day WAY better than the current approach and fixes a pretty gnarly bug with the blocks page.

The last submitted patch failed testing.

Rerolled patch to keep this moving.

Hmm, I have mixed feeling about the new coupling here. However, the current method of setting a custom theme is indeed terrible.

This seems a little odd, since how does it act if the arg is a non-existent theme?

Also, I really don't think we should be loading these new theme elements onto menu/book links - why do we need the data there?

looking at this, even seems like we should remove:

from what's loaded since we don't use them to check the access or anything else.

This seems a little odd, since how does it act if the arg is a non-existent theme?

The code in drupal_theme_initialize() actually takes of handling that case; it simply falls back on the default theme:

You can see from the above that that behavior is already in Drupal; it works the same way now if you set an non-existent theme in hook_init().

Also, I really don't think we should be loading these new theme elements onto menu/book links - why do we need the data there?

Yeah, I had been wondering about that too - I eventually decided that since we are loading literally everything else there, I didn't particularly want to make an exception just for this one.

Although, if you follow the code flow, I believe that whatever gets loaded there eventually does get passed along to hook_translated_menu_link_alter(), so perhaps that's an argument for keeping the menu item complete.... It's probably an edge case, but who knows -- maybe someone has a use for checking the theme in that hook?

So overall, it seems like where we're at is that everyone is a little uneasy about this, but also everyone thinks it's better than what Drupal currently has. And no one has come up with a better solution.

To me, that sounds like an argument for putting it in Drupal 7... although I may be biased :) It's certainly easy enough to remove in Drupal 8 if someone comes up with a better plan then.

well, let's clean up the link loading at least - don't add the theme stuff.

webchick apparently expressed her thoughts better than me. However, there is one important keyword in her follow-up:

Theme-per-site-section becomes *really* easy this way, among other things.

"section" is something, we do not have in Drupal yet. That is most likely the cause for the "uneasy feelings".

An 'admin/' prefix in menu router item paths does not really form a section - because menu links can be shuffled around and moved elsewhere. This approach, however, assumes that sections are built out of menu router item paths.

A "section" forms a "context" - something logically unique, the site admin wants to separate.

Sections cannot be presumed or prebuilt in code, because they depend on the structural decisions taken by the site administrator.

Anyway.

This is a step forward, albeit not really nice, but at least, some movement.

Why can't we define array(4) (instead of array($key)) like in any other menu router item argument situation and skip that argument-pass-thru function?

Many changes to a function... while you could just do a simple variable_get() elsewhere. Doesn't make sense to me.

A function that returns a variable_get() (that does not even specify a default value) makes no sense to me. Developers know how to use variable_get(), it's one of the simplest APIs in Drupal.

Here, we are testing for menu router paths again.

(And loading an admin.css of system module depending on a configurable admin theme is totally, absolutely nonsense.)

However, the code leads to the question whether we shouldn't trigger a hook in case a different theme than the default is initialized, so code similar to this could be streamlined to:

I'm on crack. Are you, too?

Please note that the remark about system.module's admin.css was about existing code, not this patch. Still nonsense, but hey, just wanted to clarify that. ;)

New patch:

1. Removed the changes from book.module and menu.admin.inc, as per #26.
2. Removed the system_admin_theme() function and replaced with direct calls to variable_get(), as per #27.

Other comments:

Why can't we define array(4) (instead of array($key)) like in any other menu router item argument situation and skip that argument-pass-thru function?

The block_menu() function is already doing that $key thing when it defines its menu items, rather than the more standard way. (I think the reason it does this is because it needs to "pseudo-dynamically" adjust the menu item type and weight.) So to change this would require finding a solution to that (unrelated) problem and rewriting block_menu() as well... definitely outside the scope of this patch, I think.

Also, I'm pretty sure that even if we did that, we'd still need the _block_custom_theme() function, no?

Many changes to a function... while you could just do a simple variable_get() elsewhere. Doesn't make sense to me.

Using variable_get('maintenance_mode', 0) would mean that the custom themes would not show up properly when a site administrator is browsing the site while it is offline.

The _menu_site_is_offline() function contains several different complicated conditions that determine whether or not the site is actually "offline" for the current user, so I took the approach of modifying that function rather than trying to duplicate that code elsewhere.

However, the code leads to the question whether we shouldn't trigger a hook in case a different theme than the default is initialized

Something like hook_theme_intialize() indeed sounds like it would be interesting. Not in the patch for now, because I'm wary of scope creep, but it seems worth thinking about. I'm also reasonably convinced that the whole admin.css thing (which core would use this hook for) is entirely broken and can be considered a bug to be removed :)

Sections cannot be presumed or prebuilt in code, because they depend on the structural decisions taken by the site administrator.

This is a good point, but I think it depends on the use case. Certainly there are many cases where this concept of sections would be too limiting, but there are others where it would work fine. For example, if I write a large module (think of something like Aegir or Ubercart) and want to provide an option for the site administrator to choose a particular theme for all my module's administration pages, then the fact that my entire module lives under admin/mymodule means this works fine -- and in fact, we would want the theme to persist even if the site administrator moves things around.

Also, not all code that uses this has to be perfectly generic. Maybe I'm building a site where I know that the sections do correspond to the menu paths. So I can easily write some custom code that uses the "theme callback" to achieve that. It's true that would stop working correctly if someone starts rearranging the sections completely via the admin UI, but maybe I know that on my particular site, no one is ever going to have a reason to do that...

This patch has been reviewed by at least 5 people, and each issue that was raised has been fixed and/or addressed.

I think it's up to Dries or webchick to make a final decision :)

IMO, this is an improvement and agree with RTBC.

I *really* like this patch, but... erm... no test?

Eh, I was hoping no one would notice? :)

Actually, I did look into writing a test before, and I didn't initially because it turns out that this functionality is already being tested pretty well indirectly (e.g., there are existing tests for the admin theme and for the correct theme being used on the block configuration pages).

However, it's also probably worth doing a more direct test of the "theme callback" functionality itself. The attached patch does this. In order to make these new tests pass, it was also necessary to fix a (semi-related) bug whereby Drupal allowed modules to successfully set the page theme to a theme that had been disabled, with amusing results (imagine a theme with no blocks...). So a fix for that is also included here.

Read this over again, and I am liking it more and more.

Damn you! You had to go and add tests! You know that is my one true weakness! ;)

But the more I think about it, the more I find myself liking this patch. I was worried about the extra coupling here, but it's not really /that/ much worse than the coupling we currently have between setting up the paths and the pages rendered when those paths are hit. And the amount of ugly and horrible code here that's removed is very nice. Additionally, since we removed per-user themes this release, it'd be nice to give contrib some extra power here. And Moshe giving it a +1 is also good to see, since OG is one of the modules that used the $custom_theme variable in D6 and below.

However! This needs a re-roll after one of the patches I committed tonight. ;(

Have at it, testbot.

Ah, tests... so that is the secret :)

Looks like the reroll didn't even change any of the code itself (just the code around it), so this seems like it's ready to go.

The last submitted patch failed testing.

Looks like we just need to rename system_update_7038() to system_update_7039() -- but let's see if the testbot says anything else is wrong with it.

sub

The last submitted patch failed testing.

Here's a reroll (just needed to fix a small conflict in system.module).

Actually, that's not quite right.

The tests never would have caught this, but update functions no longer involve $ret parameters anymore, so system_update_7039() introduced by this patch needed to be updated for that...

This one should be good.

No substantive changes since the last time this was RTBC, so RTBC.

Alright. Given that everyone is in support and that it fixes a bug, I committed it to CVS HEAD.

Doesn't this need some doc in the 6/7 module update guide? And maybe the 6/7 theme update guide as well?

If not, mark it back to "fixed".

FYI: this broke #539022: Batch API should use the current theme to run the batches

This does need to be documented in the module guide (probably not the theme guide), but before doing that we should make sure it is complete.

In discussion at #539022: Batch API should use the current theme to run the batches, it came out that although this method works well for setting themes per-page or per-section, it's not as good if you want to set a theme dynamically across the site based on particular conditions. The example @sun brought up was http://drupal.org/project/switchtheme, and another example would be the overlay patch. You could theoretically do it with hook_menu_alter(), but it would be really scary.

We need to add something back for these cases that works like the old global $custom_theme used to work.

With some small changes to the code that went in here, we could easily make it so that http://api.drupal.org/api/function/menu_set_custom_theme/7 can be called by modules to set a particular theme, and that would at least get us back to where we were.

However, when using that method, they would still hit all the bugs they hit previously. If we want to try to solve that as well here, I'm not sure there are too many great solutions:

• Tell them to call this function in hook_boot(), which would probably work, but sounds like a bad idea.
• Introduce another hook that runs earlier in the bootstrap than hook_init(), but only for non-cached page views. This would also probably work, but is adding yet another hook for a pretty narrow purpose.
• Don't worry about it; just accept the fact that people switching themes in this manner will not benefit from the same bugfix that we got in here for people setting themes on a per-page or per-section basis.

Didn't look at the innards, but can we turn menu_set_custom_theme() into a hook?

OK, I think Katherine managed to convince me a little while ago that a focused hook (as proposed in #49) is better than a more generic hook (as proposed in #48) because it is best to focus module authors on the main thing they would be expected to use this for, and how in the world would we come up with a generic name for "the hook that runs first on non-cached pages, before hook_init" anyway? :)

The attached patch does this. This is a (very small) API change, but required to restore functionality from Drupal 6 that was unfortunately lost (and to do so in a way that avoids bugs like Drupal 6 had)... so let's get it in. Also, as mentioned above, it will be required in order for the Overlay to work correctly.

+

Let's also move the menu implementation to menu_custom_theme(). There is no reason to make a special case out of that.

This review is powered by Dreditor.

Hm, I don't know about that... First, the code would have to go in system.module (where I'm not sure it really belongs). Second, it would require that any module which wants to dynamically set the theme play around with module weights to make sure their hook runs before the core one.

The intention here is that any module can dynamically set the page theme and override the default core behavior of determining it from the menu system, so if we know that's the order we want, I think it makes sense to separate the code out as in this patch, rather than relying on the not-very-flexible module weight system :)

yeah, that would likely require Overlay and perhaps other modules to have a weight smaller than System module, just for setting the theme. Clearly a case where the current module hook weighting system #fails. So I agree with David's reasoning, but didn't look at the patch yet.

Agreed that playing with hook weights this way would not be a good idea. Looking at the patch, in short it introduces a hook_custom_theme(), which is invoked on all modules in menu_get_custom_theme(). The last module to say something there results in that theme selected (if multiple modules return a value). The rest of the code is tests and API documentation. I think the code and implementation looks solid and will indeed cover the overlay's need to be able to switch themes on pages such as the node submission page.

Given your API example though, I'm wondering who's responsibility is to validate the theme name and how is it used later. This is not documented and would probably not be evident without studying the code flow. Would be good to document, especially when using a $_GET value.

The last submitted patch failed testing.

Rerolled patch, so it applies without fuzz to latest HEAD. It applies for me and passes tests. As discussed above, I'm only interested in cleaning up the validation responsibilities but otherwise think this is good to go. Let's go back to reviews and get a testbot run as well.

+1 on the documentation component of the patch in #57.

I don't have any comment on the code parts.

Also, someone needs to sort out what changed from Drupal 6 to Drupal 7 as a result of the various patches that were committed in this issue, and write a module upgrade guide section about it. Please! :)

OK, attempting to explain the validation as requested by Gábor turned out to be trickier than I thought and revealed a bit of a problem :) The theme system does the validation too late to really be useful for this hook, so the example provided in hook_custom_theme() wouldn't have worked too well. There wasn't an actual direct bug - but basically, one bad hook implementation would have overridden any other hooks, theme callbacks, etc and always forced the default theme to trump everything else, which is confusing.

The attached patch cleans things up a bit, so now the validation happens earlier and it should work the way we actually want it to. I had to change the existing drupal_theme_access() function to take either a theme name or object, rather than just an object like it used to (so although technically it's an additional API change, it's not one that can break any existing code). I know we usually don't like to have the same function take either a string or an object, but in this case it seemed by far the simplest way....

Also, don't worry, I will make sure something gets documented in the module upgrade guide :) I was hoping to wait until we got this committed and do it all at once rather than writing something which might turn out to be wrong, but at this point, I'll do it soon either way.

Oops, fixing a minor typo plus a clarification in the PHPDoc for hook_custom_theme().

Minor doc issue: Current standards require a blank line between the @params section and the @return in the doc header. (this is violated at the top of your patch). Otherwise, the doc is good enough for me. Again, refraining from commenting on the code, aside from style (it reads well and is clearly written).

The doc standards issue was not introduced by the patch but rather was already there - however, I suppose we might as well fix it while we're in there :)

Yeah, there are tons of violations of the doc standards in the code base. I just try to make sure that patches I review adhere to the standards before they get committed, so we can at least be moving in the right direction. Wasn't trying to blame you for the problem, just asking for you to fix it. :)

I'm happy with the doc/style now. Leaving code review to someone else.

@David: I like how you solved the problem with theme validation. I have a little note that the patched code checks for access on all themes returned by the hooks, while we only need the first valid theme. This might mean some extra unneeded checks on sites where multiple modules try to influence the theme. I do not have numbers for that, and it is unlikely that this will cause noticable difference, so I'm supportive of this patch. Nice test coverage also!

drupal_theme_access() taking either a string or an object seems a bit unfortunate in terms of API consistency - is there no way to standardize this, or make the caller responsible for loading the theme object if they just have the name?

@pwolanin: sure, on the expense of repeated code in callers all around, it is obviously doable. You did not modify the status so seems like you are not convinced about this being a showstopper(?).

Hm, I don't know about that... First, the code would have to go in system.module (where I'm not sure it really belongs). Second, it would require that any module which wants to dynamically set the theme play around with module weights to make sure their hook runs before the core one.

I don't follow the reasoning. As far as I know, the last module that set the theme takes precedence, and contrib hooks typically run after core hooks.

E.g. Sections will have a higher priority for sure... much higher than core or any other module only to make sure it' able to override other theme settings. I believe this patch would help very much to solve many issues between colliding theme switcher modules or at least make it much easier to debug than trying to trace a global variable and where it have been changed!

Today - it's really difficult to analyse this type of issues and they pop up nearly weekly. I always tell people - disable all other modules that alter the theme variable, but this is not really a solution.

@Damien: well, you are right in that David's patch taking the first return value instead of the last from hook implementations looks like nonstandard.

@Gabor: no, David's patch takes the last value. So I don't see any reason not to move the path-based implementation into the system module.

Oh, right.

This patch moves the menu based custom theme setting to the system module. Moving to menu module (naming menu_custom_theme()) as suggested by Damien would not work, since menu module is not required, but the theme callbacks should be obeyed regardless.

Since the access checking is done on returned hook values, the access check was not moved to system_custom_theme().

Let's review this one then.

BTW did not actually run the tests, but would expect them to fail due to menu_test_custom_theme() being run earlier then system_custom_theme() (due to alphabetical ordering). Looked at how hooks are invoked and still did not find any code in D7 which would make "system" hooks run earlier, so even for the tests to pass, we'd need to fiddle with some module weights probably.

The last submitted patch failed testing.

Over at IRC, @Damien suggests we add a menu_test.install with a menu_test_install() to set weight of menu_test.module above system module to come over this issue. Just like how contrib would need to work around this core behavior. I leave it to you to decide whether this is better then the baked-in default menu theme handler in David's patch.

This should theoretically pass tests, but I did not run tests locally.

Good for me.

What is the use case where a module would want to dynamically override the theme, but only on pages that do not have a theme callback defined? The only advantage of the latest approach that I can see is that it makes things a bit easier for those modules, but at the same time, it makes things more difficult for everyone else. Again, with this approach, any module that starts with a letter lower than 't' and wants to override core's default theme control will need to play around with module weights just for this one feature - and in D7, this includes contrib modules too, since they don't get any special weighting (see http://api.drupal.org/api/function/system_list/7).

Thinking of use cases, the overlay definitely wants to dynamically override theme callbacks (and always use the admin theme inside the overlay), so it isn't helped by this. A theme switcher module probably wants to override them too (I think?). So I'm not sure I understand the use case that this latest patch helps with. Also, performance-wise, it means the theme callback will be run even when it isn't being used (although I guess that's pretty minor)....

In addition, if we do go with this approach, the documentation for hook_custom_theme() that is currently in the patch would need to change.

I missed the change of behavior in D7. This was done by #211439: (Regression) module_list() function does not sort correctly, under the clear assumption that contrib modules will *have* to change their weights, even if there is no proper facility for them to do so:

This will cause contrib modules some amount of work with the weights system when updating. Still, it's really a minimal amount of work, and it's better to drop this buggy and inconsistent API now in favor of a fixed, better one, even if some modules were depending on running after core modules.

(cwgordon7 in #211439-7: (Regression) module_list() function does not sort correctly)

Thinking of use cases, the overlay definitely wants to dynamically override theme callbacks (and always use the admin theme inside the overlay), so it isn't helped by this. A theme switcher module probably wants to override them too

This is correct, and is the use-case of modules like SwitchTheme or Sections, which very potentially need to work next to System and Overlay module being installed in D7. This problem did not exist to this extent in D6, because there was only one core module (System), which only tried to conditionally set a $custom_theme on admin pages, and no 'theme callback' in the menu system, which presets a theme too early in the request.

Given all these recent regressions, moving a part of the theme system into the menu system still feels very wrong to me, as already mentioned in #14.

I think we ideally would move the entire theme key/indicator and initialization into the $page rendering array, so the usual concepts of hook_page_build() and hook_page_alter() apply. That's also where it belongs: We want to render a page, and the page output should use a theme. Potentially also simplifying and speeding up raw AJAX/JSON and XML output, because those don't want nor need a theme, but we still initialize one. However, I somehow fear that's too late for D7.... :(

The system we have now should work fine with modules like SwitchTheme or Sections - and definitely much better than D6. That's basically what it was designed for. I think the last point we're debating here (the module weight stuff) is a pretty small one. Ultimately, no matter what the API looks like, there is a fundamental problem that a single page can only have one theme, so only one module which is trying to set the theme can ever win :) There will always need to be at least some coordination between the various modules if they are ever going to be installed on the same site.

The idea of using hook_page_build() and hook_page_alter() is kind of interesting, but unfortunately that happens way too late in the page request. We need to set the theme early in the page request in order to solve all the bugs mentioned originally in this issue, which were driving people crazy in Drupal 6. To allow it to run later in the page request, we'd need to really change the way that the theme system initialization happens, so sounds like that would be tough for Drupal 7...

Anyway, the attached patch is the same as #75 but updates the hook_custom_theme() documentation to be accurate. Basically, I think either this one or the one in #62 is RTBC. I personally think #62 is better, but either works.

Please, please, please test the patch logic to see that this approach works with the implementation of Domain Theme, in the Domain Access suite.

http://drupal.org/project/domain

Here, we do theme switching based on the HTTP_HOST value, and (in D6) set $custom_theme in hook_init().

In the Domain Access paradigm, the menu path is irrelevant. The module resets the theme for the entire domain. So if you remove that ability, you break existing functionality.

NOTE: I am AFK at DrupalCamp Stockholm and cannot test this until next week. Marking as CNW, since I don't want to see this committed if it breaks my module.

But this patch, on surface, seems to only catch one use case for theme switching, But reading it, it appears that I can run the following:

If this is correct, great. If not, we need to rethink this patch.

I'm going out on a limb and suggest to do the following:

• Roll back the menu system coupling. It is wrong to couple these sub-systems, and a menu router path has nothing to do with defining a context to inherit a theme from. A menu link would rather be suitable to provide a meaningful context, but I think that's out of the game for D7.

  Due to the regression of $custom_theme no longer working, we now try to do the same in two different locations, once in a hook_custom_theme(), and once in the menu system's active handler. We do not want to have two different interfaces for the same functionality.

• Introduce the suggested new hook_custom_theme() instead, along with the early execution in the request. Additionally,
  1. invoke the hook implementations in a way that each implementation gets the $theme that was potentially set by a previous hook invocation passed as argument
  2. optionally pass the menu router item for the current path as second argument

I'm with sun on that. The menu system is the wrong place to store this data. If modules need to act on menu data, that is always available to them through a static lookup call. But tying this system to the menu makes assumptions about use-case that are not always true.

Turning $custom_theme into a hook is a better approach, IMO.

Re #81, yes, code like that should work. And the patch includes a test case which would fail if it ever stopped working :)

Re #82 and #83, while it's unfortunate we missed a use case in the initial patch that was committed here, we are fixing it now with the followup patch, and I certainly don't see any reason to roll back the whole thing. There's no problem whatsoever in having two different ways to set the theme; it's not really that different from the hook_page_build() and hook_page_alter() example discussed above. The idea is simply that we first use the menu system to set up the default association between pages (or groups of functionally-related pages) and themes, and then provide a second hook that allows you to alter that dynamically on any page request. They are really two different things (which is one of the reasons why I prefer the patch in #62 to the one in #80 - I think it makes the separation more clear).

I agree with @sun that associating the theme with menu links rather than menu router paths might be better (since it would make things more convenient for sites which have chosen to group pages in a different way, or who are implementing more fine-grained control over different pages that share the same router item). But we probably can't do that in D7, and even without that, we are still way ahead of D6, where the state-of-the-art method for all of this involved calling the arg() function a bunch of times :) There is a reason that arg() was used in D6 rather than examining the menu item directly - the menu item itself isn't useful, because rather what people were after with arg() was rudimentary path matching. Using the menu system does that all for them automatically, and in a much, much cleaner way. Furthermore, we now actually do put something in the menu item that might conceivably be useful - you can, for example, check $menu_item['theme_callback'] which gives you some information about who is controlling the default theme for the page, in case you know you want to defer to them when running your dynamic hook.

invoke the hook implementations in a way that each implementation gets the $theme that was potentially set by a previous hook invocation passed as argument

What is a scenario where you think this information would be useful? By itself, I'm not sure it tells you anything.

I actually think if we want any extra information in this hook it might be a list of modules which acted before you (in case you know you want to defer to one of them even though your module's weight is higher - i.e., this would be a rudimentary way of getting around the limitations of the module hook weight system).

If I run the code in #81, will it be overwritten by what is stored in the menu router?

With the patch in #62 your code will automatically take precedence over the menu router. With the patch in #80, it will also, but only if you set your module weight to be high enough.

My preference is to only allow this value to be set once. If we do both, then the menu-router setting should fire first, and then the alter hook.