Remove hook_menu_active_handler_alter

I agree here. This is just made for abuse (you just have to see the code example).

I mentioned this issue in the issue where this hook originated: #599804: Unify page, AJAX 'path', and AJAX 'callback' callbacks.

Too funny: @chx: the hook was your idea: it came about when you were code reviewing #599804: Unify page, AJAX 'path', and AJAX 'callback' callbacks for me in IRC. Or maybe I misunderstood what you were really trying to convey. Anyway, if you and Damien want it gone, no objections from me :)

Less is more in this case. ;)

Committed to CVS HEAD.

I really like that hook. You guys removed it for what reason. Oner hook during a page request is gonna slow down everything? You think your engineering is so fine that you will never need to dynamically alter what page callback gets called on a request. This is great for routing based on geo-ip or cookie or whatever. Removing this is arrogance, IMO.

Screw "possibility of abuse". You have such possibility all over drupal. Isn't that what hook_page_alter is all about? And template_preprocess_x can be abused and is abused routinely but it still belongs in Drupal. Drupal is not your nanny.

IMO an API change should probably not be committed 43 minutes after it's proposed. It should get at least a possibility of discussion.

Actually, I'd sure like to see us slow down on API changes all the time. And at this point they should have serious consideration.

Agreed with Moshe here. If not for contributed, then at the very least for custom modules, developers will thank for this hook to do site-specific customizations without having to hack core. Hacking core is what Drupal 5 was about, too.

Well, in the geo-ip example, assume you want each anon user to go to a geo-page thats personalized for his location. Assume that your location pages have varying 'page callback' so yes, the item must be changed. For a richer example, assume you are not allowed to provide service to people in certain countries (e.g. http://sourceforge.net/blog/clarifying-sourceforgenets-denial-of-site-ac...). So in these cases you change the 'page callback' to a dedicated access denied page. No matter what page is requested for these unlucky folks, they get this dedicated page. There are other ways to implement these requirements but they are less clear, IMO.

In D5 we had a massive array which we loaded into memory on each page. Thats also gone (thanks to chx). In general, I don't really understand the objection "but we are now like D5". IMO, thats a good thing. It was hard for some user cases to adapt to D6 static menus. In D7, we had best of both worlds. Only those who needed it would pay the price of dynamic menu, and only on the pages which merit it. Its up to those implementers to write fast code there.

Yeah, rfay's right that we were probably too hasty (43 minutes) in removing a hook this late in D7. I think one thing that contributed to that was the poor example code (look at the patch, and no wonder those minus signs look attractive). I'm a little stuck, however, finding a good example: let me explain:

The keys of the active menu router item that can be altered to affect what happens are:
access
include_file
page_callback
page_arguments
delivery_callback

There is no use-case for altering deliver_callback here, because we have a dedicated hook_page_delivery_callback_alter().

The example code that got removed was showing setting 'access' to FALSE, which has no real use-case, because one could do the same thing with a hook_init() implementation that calls drupal_access_denied().

Another possibility is setting 'access' to TRUE, but I don't think that has a real use-case, because I think the right way to do that is with a hook_init() implementation that adds roles to the $user object (for example, certain ip address might add the "intranet" role).

Altering include_file only makes sense if page_callback is also altered. Finding a use-case for altering page_arguments is more or less the same thing as finding a use-case for altering page_callback. So, is there a use-case for altering page_callback?

The example in #11 seems like it could be solved with a module that sets the 'site_403' variable to a path tied to a page callback that can serve a custom 403 page based on dynamic request information.

Another example I've been considering is HTTP Content Negotiation, using best practices described in http://www.w3.org/2001/tag/doc/alternatives-discovery.html. So, let's say a user agent requests the front page (default path of 'node'), but the http-accept header indicates it can only accept RSS. So we want to use node_feed() instead of node_page_default() as our page callback. And this can't be solved simply with hook_page_delivery_callback_alter(), because in addition to output format differences, we also need to make sure that node_view_multiple() is called with $view_mode='rss' instead of 'teaser', so we really do need a different page callback. But the W3 recommendation is to always have a URI for specific representations in addition to implementing content negotiation for generic URIs, and in the RSS example, we do: 'rss.xml'. So, we can solve this with a hook_url_inbound_alter(), not just for the RSS example, but I think for all similar content negotiation use-cases.

So, @sun, @moshe: if you can come up with a good use-case for the hook, please submit a roll-back patch that uses it for the example code.

[EDIT: just clarifying my position with respect to #11 that, IMO, a hook_init() that calls drupal_access_denied() combined with a 'site_403' path with a sufficiently intelligent page callback is a more clear rather than less clear implementation of the #11 use-case. But that's subjective.]

Well, I don't have a problem with the example in the original code. @eff says that this could be accomplished during hook_init() but so what? It is more clear to accomplish it in the menu system, whose job includes checking access for current user to a given path. Why use a catch-all hook when a specific one will do. In any case, ...

My use case in #11 stands. site_403 variable is not a clear solution to any of it. The incoming anon requests needs a different page callback based on user's geo. How can 403 variable clearly service this need? You want to put all requests through the 403 handling?

IMO the example is fine and the rollback should be undone. If someone wants to code a patch with my geo example, thats even better.

Hm.

While I think the flexibility this hook gave is ... interesting (see #363580-72: OpenID login fails when in maintenance mode), I don't see how the provided examples are good reasons for re-adding it.

- access: What's wrong with using 'access callback'? What can't be done with that that you can do with this hook?

- page_callback. What's wrong with:

// 'page callback' => 'mymodule_geostuff_dispatcher'

function mymodule_geostuff_dispatcher($arg) {
  switch(get_user_area()) {
    case 'Europe':
      return mymodule_geostuff_europe($arg);
   case 'America':
      return mymodule_geostuff_america($arg);
   case 'Mars':
     // No martians allowed!
     return MENU_ACCESS_DENIED; // In case you didn't know, yes that's another new feature of D7, see http://api.drupal.org/api/function/drupal_access_denied/7
  }
}

While maybe not so fancy as using that hook, I think this is much cleaner than defining something in hook_menu() and then magically changing it in some other hook for someone that is used to Drupal. The above function is very easy to understand.

- include_file: I just don't want to see the code that would change the include file dynamically... except on dailywtf.org ;)

- page_arguments: Can someone come up with a valid use case here? The only thing that hook has access to that we don't have in the page callback through globals is $router_item, aka, passing something of the menu item to the callback function.

Oh, looks like pasting a URL in a comment in < ?php ?> doesn't work so well :)

What's wrong with using 'access callback'? What can't be done with that that you can do with this hook?

What's wrong with 'access callback' is that it's not 'access callbacks'. So to implement a global access denied to all IPs in country X using 'access callback', you would need to hook_menu_alter() every router item to your own access callback, but then have that callback call the original access callback if the request is from an allowed IP, and I'm not sure if the menu system easily supports a module extending a router item with an 'original access callback' key during its hook_menu_alter() implementation.

But, I still question the value of hook_menu_active_handler_alter(). I think we should follow the recommendations of http://www.w3.org/2001/tag/doc/alternatives-discovery.html, not just for http-accept and language negotiation, but for any dynamic request context. So, to re-route all page requests originating from IPs from a certain country, with a custom page callback per country, implement a separate hook_menu() item for each custom landing page (e.g., 'denied/china', 'denied/nigeria', ...), and implement a hook_url_inbound_alter() to set $path accordingly based on the IP. This facilitates easier testing: for example, if you're developing this website for a client, you can then ask your client to verify that http://example.com/denied/china displays what it needs to, rather than asking your client to figure out how to generate a request from a Chinese IP address in order to see what gets displayed for those users.

See #363580-82: OpenID login fails when in maintenance mode. We will re-introduce a similiar hook which has the ability to set/unset offline mode and globally deny access, forward to another site or do whatever just before the menu item is loaded.

Imho, this can be set to fixed unless someone can come up with a valid use case that requires access to $router_item.

@Moshe/@sun: how about you continue lobbying for the richer hook in #363580: OpenID login fails when in maintenance mode (not sure if you'll convince chx, but seems like that's the issue in which to try).

Before marking this one fixed though, here's a cleanup patch that completes the removal (mostly docs cleanup, but also a stray line of code missed by the initial patch).

I committed #18 but I'm going to set this back to 'needs review' so we can continue to talk about whether to roll this back or not. I'll also investigate #363580: OpenID login fails when in maintenance mode.

Dries wanted this issue "needs review" in #20, but to have that, we need a patch that testbot will accept, so here it is.

I'm marking this fixed in light of the effort to clean up the API change queue.
Reasoning:
- There has been no comment for 4 months.
- The referenced issue #363580: OpenID login fails when in maintenance mode has been committed in the meantime and with among others moshe weitzmann's approval (who disagreed about the removal of the hook above)
- While certain people may have preferred the removed hook, it seems from the above examples there is actually nothing that can't be done in contrib now that it is gone.

In case of a re-adding of a similar hook open a new issue (against Drupal 8) and link here.
If you think this should be re-opened and the removal rolled back please mark "critical".

Re-opening at least temporarily: does this need an API change announcement (by rfay to the dev list, or a listing on the 6/7 module update page)? If not, please revert the tag/status change.

Reading through this again, I realized this hook was added early in the D7 cycle, and then removed later in the cycle, so it's not a 6/7 update guide issue after all. Marking back to fixed.