Define hook_menu_get_item_alter() as a reliable hook that runs before the page is doomed

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

Style-wise, I like the current patch.

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)

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.

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.

Will check this tomorrow.

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.

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?

+function _block_custom_theme($theme = NULL) {

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:

 m.page_callback, m.page_arguments,

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

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.

+++ includes/menu.inc	29 Aug 2009 20:31:30 -0000
@@ -1365,6 +1371,38 @@ function menu_get_active_help() {
+    $router_item = menu_get_item();
+    if (!empty($router_item['access']) && !empty($router_item['theme_callback']) && function_exists($router_item['theme_callback'])) {
+      $custom_theme = call_user_func_array($router_item['theme_callback'], $router_item['theme_arguments']);
+    }
+++ modules/block/block.module	29 Aug 2009 20:31:30 -0000
@@ -172,6 +173,8 @@ function block_menu() {
+      'theme callback' => '_block_custom_theme',
+      'theme arguments' => array($key),
@@ -187,6 +190,22 @@ function _block_themes_access($theme) {
 /**
+ * Theme callback for the block configuration pages.
+ *
+ * @param $theme
+ *   The theme whose blocks are being configured. If not set, the default theme
+ *   is assumed.
+ * @return
+ *   The theme that should be used for the block configuration page, or NULL
+ *   to indicate that the default theme should be used.
+ */
+function _block_custom_theme($theme = NULL) {
+  // We return exactly what was passed in, to guarantee that the page will
+  // always be displayed using the theme whose blocks are being configured.
+  return $theme;
+}

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?

+++ includes/menu.inc	29 Aug 2009 20:31:30 -0000
@@ -2829,20 +2880,24 @@ function menu_path_is_external($path) {
-function _menu_site_is_offline() {
+function _menu_site_is_offline($check_only = FALSE) {
...
   if (variable_get('maintenance_mode', 0)) {
...
-      if ($_GET['q'] != 'admin/config/development/maintenance') {
+      if (!$check_only && $_GET['q'] != 'admin/config/development/maintenance') {
@@ -2857,8 +2912,10 @@ function _menu_site_is_offline() {
+      if (!$check_only) {

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

+++ modules/system/system.module	29 Aug 2009 20:31:30 -0000
@@ -1329,6 +1330,13 @@ function _system_themes_access($theme) {
 /**
+ * Theme callback for site administration pages.
+ */
+function system_admin_theme() {
+  return variable_get('admin_theme');
+}

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.

+++ modules/system/system.module	29 Aug 2009 20:31:30 -0000
@@ -1441,14 +1449,10 @@ function _system_filetransfer_backend_fo
 function system_init() {
...
   if (arg(0) == 'admin' || (variable_get('node_admin_theme', '0') && arg(0) == 'node' && (arg(1) == 'add' || arg(2) == 'edit'))) {
...
     drupal_add_css(drupal_get_path('module', 'system') . '/admin.css', array('weight' => CSS_SYSTEM));
   }

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:

function hook_theme_initialize($theme) {
  if (variable_get('node_admin_theme', '') == $theme) {
    ...

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

IMO, this is an improvement and agree with RTBC.

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

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.

sub

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

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

+

+++ includes/menu.inc	26 Oct 2009 01:36:11 -0000
@@ -1480,9 +1480,20 @@ function menu_get_custom_theme($initiali
+    if (!empty($custom_themes)) {
+      $custom_theme = array_pop($custom_themes);
+    }
+    // Otherwise, execute the theme callback function for the current page, if
+    // there is one, in order to determine the custom theme to set.
+    else {
+      $router_item = menu_get_item();
+      if (!empty($router_item['access']) && !empty($router_item['theme_callback']) && function_exists($router_item['theme_callback'])) {
+        $custom_theme = call_user_func_array($router_item['theme_callback'], $router_item['theme_arguments']);
+      }

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.

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.

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

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

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.

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.

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

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:

function domain_theme_custom_theme($access) {
  global $_domain;
  $theme = domain_theme_lookup($_domain['domain_id']);
  if ($theme) {
    return $theme;
  }

}

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.

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

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.

The patch in #88 (which still applies with offset) should work for everyone at least as well as D6, and in some cases better. Modules like Domain Theme that implement hook_custom_theme() will by default override the menu router definitions, but they can also respect those definitions if they choose, as David points out in #84. The overlay module has similar needs -- it sets the theme based on the presence of a $_GET parameter -- and it's able to do that quite cleanly with this approach. And David is right -- this all needs to be resolved and documented two months ago. Let's get this patch in.

I'm +1 on that patch, too.

Sorry, this patch fell off my radar for a long while... :(

My initial query upon eyeballing this patch was similar to sun's in #62. We are doing something funky here where we're trying to keep use a "set defaults, then alter" behaviour, but we don't use our normal hook_blargy_blarg()/hook_blargy_blarg_alter() pattern.

David, I think you feel this is required because the normal module weighting system isn't sufficient for this, but is there anyway you could re-iterate why? Why couldn't System module implement hook_custom_theme() and check to see whether a path exists in hook_admin_paths() and Overlay come along later and make its own adjustments in hook_custom_theme_alter()?

Subscribing for #669510: Merge administration theme with hook_admin_paths().

subscribing

subscribing

I think I have to watch this issue when I start porting ThemeKey to Drupal 7.

As already mentioned in #98 I maintain a "theme switching module" called ThemeKey.

ThemeKey's Drupal 6 version switches the theme by setting $custom_theme in it's implementation of hook_init() depending on different rules. To port this module to Drupal 7 hook_custom_theme() seems more promising to me than dealing with the menu.

So when do you think a final decision will be taken? Right now it seems to early to me to start working on the port ...

OK, since the latest test passed, let's try RTBC and see what results.

Thanks. Committed to HEAD.

It's worth a note in the upgrade guide about this change if there isn't one already.

I am not sure I am happy about this solution.
#928160: Menu access checked before hook_init()???

Some thoughts:

• Is it a good idea to call menu_get_item() before hook_init() ? And if so, should we not always do it, instead of just in some cases? I am sure this uncertainty gives a lot of hard-to-reproduce bugs (which might already be reported, and we have no idea they are related with this issue).
• If we call menu_get_item() before hook_init(), do we need another hook before that, which is free of menu_get_item() ?
• Do we really need a theme initialized before hook_init() ? And if so, do we need another hook before hook_init, that is theme-free? For instance, redirect modules don't need a theme initialized, but some of them could benefit from a $router_item.
• If we already have a $router_item, should we make it available to hook_init() and hook_custom_theme() as a parameter? This would allow implementations of hook_custom_theme to have a look at $router_item['theme_callback'].
• Is the name, hook_custom_theme(), not crying loudly for prefix nameclashes with hook_theme()? Say, a module name is mymodule_custom, and another is mymodule, then what is mymodule_custom_theme? Or do we not allow modules to end with "_custom" ?

I mark this as "needs work" to get it back on the table, but I am not opposed to having it close again, and debating in a new issue.

Don't think this should ever have got in in the first place, however I think it'll be a new issue to roll it back in D8, can't see what can be done about it now.

D8 will be a big cleanup :/
Well, it can't be much worse than all those contrib modules doing menu_get_item() in hook_init().
But, in theory, what is the "correct order" of menu_get_item(), hook_init() and choosing a theme?

"a new issue" -> one of these?
#909178: Theme negotiation in menu_execute_active_handler()
#852562: Call a hook from menu_execute_active_handler, after the router item is found.
I'm not sure if menu_execute_active_handler() is really the correct place, but so far this is the place where menu_get_item() is supposed to be called, in the book.

Thanks for raising this critical concern, @donquixote. menu_get_item() being invoked before hook_init() definitely breaks at least one of my contributed modules. That module uses a special negative weight to be the first hook_init() invocation and dynamically alters the context of the current request, in order to make the menu system and everything else after hook_init() take into account that information. Since that module is not ported to D7 yet, I wasn't aware of this critical problem. AFAICS, that module cannot be able to work right now. Note that this module is an essential building block of http://localize.drupal.org

What if we would give up on hook_init(), and introduce a new hook that is called before hook_init() and before menu_get_item() ?

There are some other things in menu_execute_active_handler(), such as, a conditional menu_rebuild(), that are supposed to run before the first call to menu_get_item(). So if we move menu_get_item() before hook_init(), then we have to move these other things too.

Another solution would be to introduce new hooks within menu_execute_active_handler().

I assume "new hooks" are considered a "new feature", and thus we are not supposed to introduce them to D7. I wonder if this is a reasonable policy. New hooks are one of those things that can usually be expected not to break existing code (except with nameclashes).

-----

In any case, we should agree on some "correct order" for menu_get_item(), hook_init() and theme negotiation.
Imo, menu_get_item() including the access checks should be run before choosing a theme, so theme switcher modules can use it as a context.
We then need hooks that run before and after menu_get_item(), and before and after a theme is chosen.
- Theme switcher modules might want context information from the $router_item.
- Redirection logic (such as with globalredirect or path_redirect) does not need a theme, but could in some cases want the context from the $router_item.
- Ajax requests usually need a theme (even json format could contain themed html somewhere) and of course a $router_item. On the other hand, they do not need a lot of the stuff that some modules do in hook_init() - such as, breadcrumb, css and js files.
- Breadcrumb building (the trail, not the html) could be done after menu_get_item(), but before choosing a theme, so a theme switcher module can use the breadcrumb trail as a context (something I would like to do with "crumbs").

As for theme switching via hook_menu(): Instead of $router_item['theme_callback'] we could have $router_item['theme_hints']. This would be a bunch of keywords that theme switcher modules can use to choose a theme.

@David_Rothstein (#111):
menu_get_item() calls a bunch of callbacks for wildcard loading and access checking. A lot of these use information from global state. Some of this global state looks different after hook_init(), because it is altered by hook_init() implementations. Thus, menu_get_item() will behave differently if called before or within hook_init(), than it would if called in menu_execute_active_handler.

After the first call to menu_get_item(), the result is cached in a static var and will never change again. It would be even worse otherwise, because contrib expects menu_get_item() to always return the same result.

So, it would be a good idea to have a well-defined moment for the first call to menu_get_item(), and not let modules touch it before that moment.

Maybe there are more aspects that I am missing atm.

Swap we can hook_init and drupal_theme_initialize but if drupal_theme_initialize now depends on menu then the first hook_init to call theme break the rest of hook_init will.

@David: Since you asked for more details... I can only provide my concrete example case, as I don't have any other issues with this: http://drupal.org/project/og_user_roles uses a negative module weight to be the first module that is invoked for hook_init(). In hook_init(), the module checks whether the current path belongs to a organic group context, and if it does, it dynamically alters the global $user object to add more user roles. This code therefore ran before any other hook_init() implementation and also before any invocation of menu_get_item(). Since the module altered the $user object, the first call to menu_get_item() is negotiating the menu router item and active page callback, but most importantly checking the user's access - based on the dynamically enhanced $user object.

In other words, this sequence worked:

og_user_roles_init()
  - Figure out group context (via custom functions)
  - Apply user roles, if any
og_init()
  - Figure out group context (via menu_get_item()'n'stuff)
  - Do something else (no idea)
menu_execute_active_handler()
  - Figure out active handler (via menu_get_item())
  - Check access
  - Usual processing, depending on access

I guess, for my use-case, it would be sufficient if the code was moved into system_init(), so og_user_roles could simply tweak the hook invocation order via hook_module_implements_alter(). Can't speak about other use-cases.

Well isn't this a pile of spaghetti.

It ain't pretty, but I think you could alter global state using hook_custom_theme(). That runs before menu_get_item(). See menu_get_custom_theme().

Cue in James Earl Jones dubbing Drupal 5 hook_menu: " I've been waiting for you, hook_init. We meet again, at last. The circle is now complete." :D

Edit: aka we are using again a menu hook more or less to complete the functionality of hook_init. We lived with that then, we will live now.

@chx (#115, #118), I you no understands.

@sun (#116)
This looks like a hack to me, though I can very well imagine that it is the only possible solution in D6 / D7.
What if on a page for group A you want to display stuff from a different group B, that involves access checks for group B stuff? Now the same permissions from the added roles will apply to group B, that were originally meant for group A.

I think with a few added hooks we could solve most of the "what goes first" problems. And the "hack" thing is a different story, not for this issue.

----

@all:
I think there are two separate questions to discuss:

1) Philosophy / D8: What is the correct order for menu_get_item, access checking, and choosing a theme? And how many hooks can we put in between? For this question, let's totally forget about the existing hook_init(), even if in the end we might call one of the new hooks hook_init() again.

2) D7: Now that we decided on a "correct order" in theory, how close to that can we get in D7 ? A big problem is a hook_init() that is filled with just any kind of unrelated stuff, because for the longest time it has been the only hook between hook_boot and hook_preprocess_page. I think it's too late to expect module maintainers to move their implementations out of hook_init(), into any new hook.

to "2) D7":

Swap we can hook_init and drupal_theme_initialize but if drupal_theme_initialize now depends on menu then the first hook_init to call theme break the rest of hook_init will.

ehm "If we drupal_theme_initialize after hook_init, then any hook_init implementations that do theme() will breaks." -> yes.
That's the D7 problem with existing hook_init implementations, that leave us little choice.
(i) some modules do menu_get_item() in hook_init, and want information from the access check and wildcard loader results.
(ii) some modules do stuff that is supposed to run before menu_get_item access checking (and wildcard loading), to alter some global state.
(iii) some modules might want to do stuff that is supposed to run between menu_get_item wildcard loading and menu_get_item access checking. Duh.
(iv) some modules might do theme() in hook_init.

I think the least expensive/intrusive solution could be to provide new dedicated hooks for the (ii) and (iii) stuff, that can run before hook_init() and before theme initialization.

Tagging as beta blocker. I think we need to figure this out before-hand as according to #928160: Menu access checked before hook_init()??? the current behaviour is creating quite some frustrations for module developers porting their stuff. We either need to fix it, or we need to document the new workarounds that allow these modules to work.

I don't have any particularly useful insights atm since it's 5am. :P Am curious to hear from David, though.

IMO, we don't need to block beta for this. there are no schema implications here. hook_custom_theme() is workaround, until we decide if we want to add another hook.

I can only concur: this is not a beta blocker, we already have our hook even if it is hook_custom_theme and so we only need to document this fact. We are not adding any more hooks at this point.

To make a more predictable flow, should we always call menu_get_item() before hook_init(), instead of just sometimes?
This would let some code always break, that would otherwise just sometimes break. Better for troubleshooting.

And, not to forget the nameclash thing:
http://google.com/codesearch?as_q=name&btnG=Search+Code&hl=en&as_package...

Consequence: If there is two modules with $module1.'_custom' == $module2, then $module1 should not implement hook_custom_theme, and $module2 should not implement hook_theme. As long as this does not happen, everything is safe.

@donquixote: Re-thinking the bootstrap process hooks is out of scope for D7. I also don't get what that "nameclash thing" has to do with this issue.

I agree that relying on hook invocation order and whatnot is quite some spaghetti. My particular case could probably also be solved by adding an alter hook invocation from within menu_get_item(), after the router item has been determined, and before any access checks or processing happens. That's the actual step in the code path that og_user_roles (and I can only guess also stuff like Context/Butler) is interested in.

Um, do you really think that I would remotely consider to implement the hook_init() mess if I could use hook_boot() or any of the other pre-full-bootstrap hooks? og_user_roles cannot use hook_boot(), as modules are not loaded, path is not initialized, and various other complications exist.

"nameclash"
I think I was too paranoid here.
The thing I complained about was a hook name that has another hook name as a '_' suffix. This will create a problem if token ever implements hook_custom_theme, or if token_custom implements hook_theme. If things like this do not happen (that is, if people are careful enough), then we have no problem.
The above post shows at least one more such hook name conflict: hook_language_init vs hook_init. The battle is already lost. I give up on this and wait for D8 and #867772: Use PHP 5.3 namespaces.

----------------------

If anyone else happens to call menu_get_item() in one of the above hooks earlier than where you put yours, the same bug occurs all over again regardless of what core does.

Some documented rules on when you can call menu_get_item() and when you can't would help - a bit. So you can at least hope that menu_get_item is less likely to be called in earlier hooks.

the following hooks and callback functions get triggered between hook_boot() and hook_init()

Some of these hooks run only in some cases, and others are invoked again in other places out of bootstrap. Except hook_custom_theme, there is not much left.

@David_Rothstein (#133),
See my $router_item['theme_hints'] idea in #113. hook_custom_theme() would receive $router_item as an argument, and could use the theme hints as an indicator to use the admin theme or whatever.
Atm the 'theme_callback' is only considered if there is no hook_custom_theme(). This seems wrong to me. There are some cases where the setting in $router_item should have priority, and there are other cases where hook_custom_theme() should have the last word, and there are cases where hook_custom_theme() should make a decision based on information in the $router_item.
All of this would mean that we menu_get_item() before hook_custom_theme(), which again would make this hook useless for things that want to run before menu_get_item().

I don't see why adding new hooks should be a problem for a close-to-release branch. I would even say that new (backported) hooks for D6 would be not only acceptable but in some cases a very good idea. Contrib is doing it every day. Even if D7 goes beta and stable before we finish this discussion, I don't see any technical reasons for not adding a few extra hooks in the bootstrap.
If this is the current release policy, it deserves some debate. Especially when we get rid of nameclashes in D8.

In any case, here's a very simple patch for @donquixote's idea of making sure that the core bootstrap process itself always call menu_get_item() at a consistent point in the process.

It would be cool if we can pass this thing as an argument to hook_custom_theme().

And there is more to it than that:
http://api.drupal.org/api/function/menu_execute_active_handler/7

A few things happen before menu_get_item(). One of them being a conditional menu_rebuild(). In some cases menu_get_item() is not called at all. We should consider that if we want to move menu_get_item() into a different place.

In any case, here's a very simple patch for @donquixote's idea of making sure that the core bootstrap process itself always call menu_get_item() at a consistent point in the process.

I have a hard time accepting dooming a page to end up with 403 before hook_init() (= before even initializing each module) as reasonable behavior.

Misusing hook_custom_theme() — a hook with a very well-defined purpose — to get ahead of the first menu_get_item() call and get a chance to 'enhance' the $user object is very hackish. Moving the menu_get_item() call ahead of hook_custom_theme() improves consistency (which is a good thing in general), but it kills even that faint glimmer of hope!

If anyone else happens to call menu_get_item() in one of the above hooks earlier than where you put yours, the same bug occurs all over again regardless of what core does.

Are we really helpless here?

I don't claim to understand all the implications, but can't we specify that menu_get_item() will not fill in the 'access' element (and thus not call any access callbacks) until hook_init() has run for all modules?

Dynamically changing the $user object for certain pages looks like a reasonable pattern to solve an entire class of challenges. Maybe we need a hook_user_alter() that's called before the access callback (but ideally after hook_init())?

I agree with @donquixote about adding new well-defined hooks. It's never too late to bring some order into chaos.

So, your idea is (more or less):
1. menu_get_item() without access check
2. hook_custom_theme() and hook_init()
3. access check.

I think this will cause other problems: What if we choose a theme and add decorations based on the $router_item found in step 1., and then end up with a no-access? What if some modules want to show a specific theme for no-access pages? What if we don't want a user to distinguish no-access from page-not-found?

Maybe we need a hook_user_alter() that's called before the access callback (but ideally after hook_init())?

This thing should happen before choosing a theme.
hook_user_alter() does not need any theme functionality, but choosing a theme can depend on the user. We do even check if someone has access to a theme!

I agree with @donquixote about adding new well-defined hooks. It's never too late to bring some order into chaos.

I think adding new hooks is safe (except for nameclashes, but we are never safe from that)
On the other hand, splitting up existing hooks, or changing their behavior, is dangerous and will make people angry.
Introducing a new hook (hook_user_alter) and asking a small number of contrib authors to move their stuff out of hook_init and into the new hook, might still be technically acceptable. Especially, since these authors are probably not very happy with hook_init anyway.

@donquixote: If you want to have it boiled down this way, here are my priorities and dependency order:
1. Set up $user (core)
2. Load each module
3. Initialize each module (hook_init()) — after all there must be a reason why they were enabled...
4. Customize $user (hook_user_alter()), based on the page
5. 1/2/3 (except hook_custom_theme()) in comment #127 above
6. menu_get_item() and check accessibility (access callback)
7. Theme the resulting page (hook_custom_theme() and whatever), possibly an error page — only after we know what to display

Anyone can call menu_get_item() at any time, but if it's called before #6, then the access checking is simply skipped.

Is it necessary to allow/support calling theme() before #7? Maybe just offer default theming before #7?

I don't claim to understand what I'm talking about, but in my naiveté I don't see
— why hook_custom_theme() must precede hook_init(), except for its secret property of preceding hook_init(), allowing hacking, which is a Bad and Undocumented Thing anyway
— why we need to worry about theming the 403 page when we can't even get the access right

Most if not all of this is already cast in stone, I know...

why hook_custom_theme() must precede hook_init()

Because contrib got used to having theme() available during hook_init().

Because contrib got used to having theme() available during hook_init().

In d6 it was not allowed to call theme() or t() within a hook_init implementation:
http://drupal.org/node/219910#comment-1254086
http://drupal.org/node/740100

In d7 it will be allowed:
http://drupal.org/node/405578#comment-2206974

So hook_custom_theme() has to be run before hook_init().

For a theme switching module like ThemeKey this should be OK. For a d7 port of ThemeKey we'll move the logic from hook_init() to hook_custom_theme().

But all modifications to the user object like mentioned in previous post should happen before hook_custom_theme(), because ThemeKey offers switching rule depending on user roles, ids etc.
This might require to change some other contrib modules ...

This might require to change some other contrib modules ...

These are modules that would break anyway, so I think it's an acceptable price.
And I am pro a new dedicated hook for that, between hook_boot and hook_custom_theme.

What exactly are you theming during hook_init()?

And how does that relate to its stated purpose on http://api.drupal.org/api/function/hook_init/7?

I don't know of any examples of theme() during hook_init. But there are just too many modules around to assume that none of them do it.

"stated purpose"
I don't know of any other hook between hook_init and hook_preprocess_page. Noone cares about a "stated purpose".

You are only not allowed to call t() with a % placeholder in D6 hook_init() or the theme will be inited an than you cannot change it, but t() with other placeholders is fine :-)

Well, in D7 you aren't supposed to theme anything until drupal_render_page() gets moving. So theme() during hook_init() is always wrong in D7 AFAICT.

%placeholder theming in t()? Interesting! What can reasonably be done other than wrapping in <em class="placeholder">? Requiring the theming system to be initialized for that purpose sounds like a bug in t()...

Noone cares about a "stated purpose".

Then we're on a very slippery slope.

Quality should be distributed evenly in a project. It makes no sense to carefully craft thousands of tests and fail to document the essential patterns. Every chain breaks at the weakest link.

Obviously you can't force people to care about stated purpose, but those who misuse hooks should not come around whining when their assumptions turn out to be nothing more than assumptions. How could the Drupal community possibly want to guarantee that anyone's assumptions will always hold true?

Casual comments like http://drupal.org/node/405578#comment-2206974 cannot be "documentation" that is fit to be built upon.

Noone cares about a "stated purpose".

Then we're on a very slippery slope.

Truth is: modules use hook_init() if hook_boot() would be too early and hook_preprocess_page() would be too late. The only way around that is to provide more hooks.

That said, at the moment I fail to imagine any real use case for theme() or even t() in hook_init(). Maybe for html that goes into an email or into the database, instead of the page?

------

Well, in D7 you aren't supposed to theme anything until drupal_render_page() gets moving. So theme() during hook_init() is always wrong in D7 AFAICT.

Interesting. Then why do we initialize theme before hook_init() ? Why not wait until menu_execute_active_handler() ?

@salvis re: #146 "Requiring the theming system to be initialized for that purpose sounds like a bug in t()..." See #601548: Loosen the dependency between t() and the theming layer

Attached patch should at least solve my use-case. Didn't test it, but I'm pretty sure it will.

That looks really good to me as well.

So let's see whether this flies. Wasn't able to think of a simple use-case, and also didn't want to put og_user_roles' use-case into the example, so I went with an abstract example. ;)

+++ modules/system/system.api.php	4 Oct 2010 20:43:08 -0000
@@ -742,6 +742,38 @@ function hook_page_build(&$page) {
+ * menu router item after it hass been retrieved and before it is translated and

typo

+++ modules/system/system.api.php	4 Oct 2010 20:43:08 -0000
@@ -742,6 +742,38 @@ function hook_page_build(&$page) {
+function hook_menu_breadcrumb_alter(&$router_item, $path, $original_map) {

bad function name

Are we all aware that this will affect every call to menu_get_item(), even those with a $path argument? Intended?
Not saying this has to be a problem, only if we want to use it as alternative to hook_init, for setting a global state.

And this does not really solve the uncertainty for hook_init, where we don't know if menu_get_item did already run or not.

EDIT:
Ok, seen it now. Hook implementations can check "$path == $_GET['q']".
Are we sure we want this?
Introducing a hook_menu_get_item_alter() is a new feature that opens a whole bunch of possibilities and would deserve its own feature request.
For this issue we only use a side effect of the new alter hook, and don't really alter anything. And for all use cases discussed in this issue, we are only interested in the first call to menu_get_item(), the one with "$path == $_GET['q']". Seems this would be solved with something simpler than an alter hook for all menu_get_item calls.

D'oh! Thanks for reviewing, moshe!

Yes, this should cover my use case as well, and it might even simplify some of the other chores.

@donquixote - I do think we want this, and we want it in this issue. Its an elegant fix to this and other issues.

see #943616: hook_custom_theme() does not work as expected

#943616: hook_custom_theme() does not work as expected is closed (works as designed) — I'd really appreciate getting this committed so that I can continue working on Forum Access for D7.

What can we do to get this moving?

This.

#154: drupal.menu-get-item-alter.154.patch queued for re-testing.

Set a more appropriate title.

This was reopened at #107 and took a slightly different turn.

To sum up: menu_get_item() is called before hook_init(), and the result of the access check is stored in a local static variable for later use. If it fails, there's no way to recover. A number of modules that need to
— provide access to paths that would otherwise not be accessible,
— massage other aspects of menu router items in real time,
— enhance the $user object in some way, or
— do something else really early
need to have a hook that runs before menu_get_item() to do their thing.

sun's proposed hook_menu_get_item_alter() is ideal for this.

Using hook_custom_theme() does not work so well, because there's no guarantee that menu_get_item() hasn't been called already when its your turn (and you cannot tell!), nor can you know whether the theme is already set and you can call t() or not.

See #928160: Menu access checked before hook_init()??? for a longer explanation regarding forum_access. sun mentioned og_user_roles, mkalkbrenner themekey — I'm sure there are other contribs that need this badly!

One question with this approach.
Consider the following situation.
- module1_menu_get_item_alter() sets some global state stuff depending on the router item.
- module2_menu_get_item_alter() changes the router item, making the global state stuff by module1 obsolete.

The clean way would be to run the alter() stuff first, and then call a separate hook, hook_menu_item_found(), where modules can do their global state magic.

Or is this one hook too much?

@donquixote: You have hook_module_implements_alter() to control and adjust the hook invocation order, if required. But that said, the use-case of actually dynamically altering the router item at run-time should be an edge-case. Most modules use hook_menu_alter() to alter router item definitions.

I just wonder if it makes sense to provide an alter hook invoked with each menu_get_item(), where what we actually want is a menu_item_found() hook invoked only one time in a request.

But whatever, maybe someone will find a use case for the alter hook.

Dynamically setting

  $router_item['access_callback'] = TRUE;

on selected items is extremely powerful to convince inflexible modules to allow access.

For example, Forum Access needs to tell comment.module to allow forum moderators to edit/delete comments in their forums. The alter hook works perfectly for this!

Hooking access callbacks through hook_menu_alter() is more complicated and presents a much higher risk of creating nasty conflicts. (Anyone remember Terminate&StayResident programs under MS-DOS fighting to hook interrupts?)

Yup, we really do need this.

+1 subscribing

#166 actually sounds like rather an abuse of this - it should be (as sun says) only very, very rarely needed.

Looking at the code around the patch - I'm confused as to why we think caching this is faster than the simple SQL query (attack of the MongoDB users?). In any case, assuming we leave that cache_set/get in I see a little optimization I'll roll as a separate patch: #964822: Small optimization for menu_get_item()

OT:

@pwolanin:

#166 actually sounds like rather an abuse of this - it should be (as sun says) only very, very rarely needed.

What alternative would you suggest? Can we maybe pursue this in #928160: Menu access checked before hook_init()??? or in a new issue?

Let's go with the existing patch and drupal_alter() for now. That edge-case may very well be the Butler project -- I'd like to see a proof of concept implementation in D7 contrib already; which this patch effectively allows (next to fixing a regression).

#154: drupal.menu-get-item-alter.154.patch queued for re-testing.

#154: drupal.menu-get-item-alter.154.patch queued for re-testing.

Looks like the least ugly fix for a whack-a-mole problem– and more importantly, final fix. And needed for certain contrib modules to be able to do in D7 what they can do in D6.

Citing webchick in http://drupal.org/drupal-7.0-beta3:

we urgently need your help in taking one last look at your queues over the next day or two and making sure there aren't any remaining critical issues buried in the "major" list.

I believe this matches one of the items in webchick's definition of 'critical': "presenting undue hardship onto a contrib module."

There is no reliable hook to keep a page from failing the access test. Any module that needs to do this has to resort to dirty hacks. It's not impossible, but I have yet to see a clean way to do this, if we can't have hook_menu_get_item_alter().

This is not just a problem for some contribs — forcing contribs to hack is undermining Drupal's stability.

EDIT: The present incarnation of this issue started at #107, the short version at #149.

Yeah, I agree with this issue being critical. This is a good example of something that not only blocks contrib, but we need to come up with the correct solution now, before we hit release candidate status, because there are API freeze implications.

However, I talked to chx about this patch yesterday and he voiced some concerns about it. I'd like to hear more details from him before checking this patch. I'll poke him later today in person. ;)

I'm pretty sure that chx's concerns are more or less the same as those already raised in #838408: Remove hook_menu_active_handler_alter

There, hook_menu_active_handler_alter() was removed, because its usage could be "dangerous beyond extreme". Many people disagreed with the removal, especially, because every hook in Drupal can technically be abused to do bad things.

However. In this case we have a regression compared to D6, since modules no longer have a clean entry point to intercept the first call to menu_get_item() for the current request. While it's true that the hook introduced by this patch can be abused to do bad things, there are many actual use-cases, which prove that the main purpose is to restore the ability to do good things, from which at least one is a key building block for http://localize.drupal.org.

@webchick: Tell chx he'll have to help me with Forum Access if he doesn't agree with the patch... ;)

Another side note.
If I want to alter some global state thing (such as the $user object) before any wildcard loading?
hook_init() can be too late. hook_menu_get_item_alter() is too late. hook_boot() is probably too early.

hook_custom_theme() ? Ok ok, that's probably where this needs to happen now. It feels a bit stupid having a hook called custom_theme and using it for something completely unrelated..

Menu in Drupal 6 provides a uniform, single, simply-to-audit access mechanism for paths: the access callback and the argument. If you, as salvis has written in the other issue need to do something different with the access of comment/cid/edit then change its access callback on hook_menu_alter and be done with it (tell me why that does not work if it doesnt). This way it's fairly easy to verify how access is controlled for any path: the router table access callback / arguments tell you. End of story. With runtime access changes you need to analyse every module whether it alters the access of the path in question.

Also, as the code example shows if you change access only when looking at the given path then you can get two behaviours: one, you see a link that leads to 403 which is not a nice user experience but the other is even worse: you do not see a link to a page you can access -- the security hole is a lot more obscure this way.

So I am solidly against runtime access changes. But, the hook is apparently needed. So what about this?

I can't speak for salvis, but not allowing access* to be changed is perfectly fine for me.

A module of mine shows how access callback chaining can be done. It's still one single code chain to analyse instead of a miriad alter.

I agree that ideologically, access modification shouldn't be allowed, but I don't really see any practical reason to not allow it.

Discussed with chx - I think the path should be protected too to avoid weirdness and potential bypass of the access by changing the values going into the access callback.

Any other properties need to be protected?

Asked the other way round, which of the properties do we want to be able to alter, that we can't alter in hook_menu_alter() ?

I think there are just too many things awesome Drupal people might want to do, change the theme callback depending on some request context, prefix the title etc which are all can be not path specific so hook_menu_alter() does not work.

About keys, debated load_functions with myself and pwolanin and i think that actually allows for more fun. So these three keys look good to me. Asked salvis to try access chaining as linked above and then RTBC this.

Ok, with chx' help I got chaining to work in a satisfactory way, and I can see now why it makes sense to protect those access keys.

#184 does what it promises.

I talked with @chx at BADcamp today about this patch; I totally agree with not allowing access altering, and the usefulness of this patch otherwise is worth its weight in gold. Now we can store arbitrary array keys on the router item, and have it globally accessible and statically cached (without having to declare custom globals). There are all sorts of possibilities. I like this very much.

IMO, protecting those keys is nanny drupal. It is "we know better than you". If you don't want to audit the contrib code that you are running on your site, then zero out this hook in module_implements_alter(). i'm ok if i get overruled, but thats my opinion.

Protecting the values looks messy to me as well, if we do it here why not the thousands of other places you can shoot yourself in the foot? Since that's three of us I'm putting it back to CNR.

Then let's go with http://drupal.org/node/553944#comment-3529924 Moshe's idea w hook_implements_alter is good.

This is fine, I am a /huge/ fan of allowing hook_module_implements_alter() to take care of disabling things that we don't like.

Hmm, hook_module_implements_alter(). I've never used it but it sounds really useful.

Now that I think about it, the access alter blocking looks inconsistent with our philosophy elsewhere. Altering access, however, should be done carefully, and like @David_Rothstein says we can use documentation to communicate that. Here's #154 re-uploaded.

OK, sounds like we've built consensus around this approach. I also dislike the idea of Drupal being a nanny; the patches from earlier today were giving me the heebie jeebies. Glad that moshe was able to come up with a compromise on chx's legitimate concern (though hook_menu_implements_alter() still scares the hell out of me ;)).

It sounds like there are enough use cases in contrib for this now that menu_get_item() instantiates the theme way earlier than Drupal 6 and below to classify this as a regression.

The only problem I had was that the docs weren't very clear to me. So I worked with chx to touch those up a bit, and then committed the following patch to HEAD. Thanks!

Thanks!

So does this API change break anything? Need to be announced? If so, please give a summary of what people need to do.

Thanks,
-Randy

No, it's just a shiny new hook.

I don't know whether it's completely accurate, but #127 gives a good overview of what happens. Any of the hook implementations might call menu_get_item() and thus cause the new hook to be called just in time.

It just works! :)

We do need to document the new hook in the upgrade docs. Otherwise, people who don't closely follow all core development won't necessarily know of the existance of the new hook. And, to that end, it probably wouldn't hurt to announce it as an API change. Even though it's not going to break any existing code, it might facilitate cleaning up existing code. ;)

Yep. Missed that. Thanks.

Demoting to "normal" because this is just a docs issue now.

changing tag scheme for update page

There is another issue where this has a relevance.
Organic groups uses hook_init() to enrich the global $user object with the groups this user is a member of.

<?php
function og_init() {
  // We have to perform a load in order to assure that the $user->og_groups bits are present.
  global $user;
  if ($user->uid) {
    // $user gets modified by reference.
    og_user('load', array(), $user);dpm($user);
?>

The og_forum module uses this information in a wildcard loader
for the router path 'node/%og_forum_group_type/og/forum/%'

<?php
function og_forum_group_type_load($arg) {
  $node = node_load($arg);
  if (og_is_group_type($node->type)) {
     global $user;
     $tid = og_forum_get_forum_container($arg);
     if (og_forum_is_public($tid)) {
       drupal_set_message('is public');
       return $tid;
     } else if (!empty($user->og_groups) && array_key_exists($arg, $user->og_groups)) {
?>

This can only work if hook_init() runs before menu_get_item().
In my case it is menu_breadcrumb_init() that calls menu_get_item() before og_init(). But we know that there is more than one module that call menu_get_item() in hook_init().
With the above patch and previous changes in D7, Organic Groups will now have to implement hook_custom_theme to do its magic to the $user object, instead of hook_init().
Is this what we want, or should we rather have a dedicated hook for things that should run before menu_get_item(), and before theme negotiation? Something like hook_pre_init(), or hook_earlybird() ?

Use hook_boot(). The $user object is already instantiated then.

Ok, this would work, in this case.
But hook_boot is special, and there are things that you don't want to run before the bootstrap is finished. hook_boot() and hook_init() have quite a high population, so something extra would allow more fine-grained control.
But ok, let's wait until we find a real use case for an additional hook. I think it would not hurt too much if we add it later.

Slightly off-topic but potentially useful to those who participated in this thread:

The new Chain Menu Access API module has grown out of #180 above. The code was written mostly by chx and it provides a simple API for correctly chaining all variations of access callbacks through a simple function call.

Now this is sick. og_init does actually call menu_get_item() itself, indirectly, in more than one way. I don't think we want that to happen during hook_boot.

If you want to know, here is the issue and backtrace:
#775362-6: og_forum: access checking fails, if menu_get_item() runs before og_init()

Now how is this relevant?
Just another case where the sequence of menu_get_item() and stuff that is expected to run before it gets messed up.
I don't know if this should still be discussed in this issue. Maybe not.

It looks as though someone already added this to the 6/7 module update guide:
http://drupal.org/update/modules/6/7#custom_theme