Make menu_set_active_item a complete inner redirection

commin.inc shouldn't call private menu.inc functions...

I looked around in core and found no problem with this. The only concern could be that now contextual items are added for the original and the changed path. However, when the tabs are rendered only the children of the active nontask item are taken into consideration so I fail to see how this could cause superflous tabs to appear.

I don't understand this patch. menu_get_menu already calls _menu_append_contextual_items() ...

Committed to CVS HEAD. Figured it out with the help of chx.

Reopening because this commit results in hook_menu(FALSE) called multiple times on certain pages; for example on taxonomy/term.

This happens when a module calls menu_set_active_location for example, taxonomy on taxonomy/term/tid pages.

As many modules, including core modules, have code inside hook_menu(FALSE) that is believed to run only once, this is a problem.

Possible solutions

1. Roll this back, reopen http://drupal.org/node/88474
2. Surround all code that should be only run once with static $ran; if (!isset($ran)) { ...
3. Introduce a hook_start
4. An additional parameter to hook_menu (see below) that allows code to distinguish between first and subsequent runs.
5. ?

I'm against 4, because such a modification could be done much cleaner with the introduction of hook_start. IMO, using hook_menu for this has grown organically.

Rolling this back, will affect the validity of hook_menu(false) results when ?q= changes on a menu_set_location change.

Note: http://drupal.org/node/88474 is dependend on this issue.

Here's the discussion from IRC (quoted with chx permission):

[21:53] chx: Heine: hook_menu(FALSE) is called multple, so what?
[21:54] Heine: My thought exactly
[21:54] Heine: I'll fix the docs
[21:54] chx: Heine: the $_GET['q'] change meanwhile
[21:54] chx: Heine: or so I remember
[21:54] chx: Heine: if you dig the original , you will find my comment on this
[21:54] Heine: Yep, its all very logical (digging as we speak)
[21:55] ChrisKennedy: from menu.inc: "This hook is also a good place to put code which should run exactly once per page view. Put it in an if (!may_cache) block."
[21:55] kkaefer: I'd say it's by design
[21:55] Heine: Ke
[21:55] chx: ChrisKennedy: ouch! true.
[21:55] chx: kkaefer: ^^ we are in trouble
[21:55] ChrisKennedy: yep
[21:56] chx: I forgot
[21:56] Heine: chx, this is not a problem
[21:56] chx: darn, I shoudl have renamed hook_init in common.inc earlier
[21:56] kkaefer: remove that statement and we're done ;)
[21:56] chx: how so?
[21:56] chx: people have lots' o' startup code in there
[21:56] Heine: static
[21:56] ChrisKennedy: you're breaking all modules that were written based on that...
[21:56] Heine: So what
[21:57] Heine: Breaking an API a day keeps the doctor away
[21:57] kkaefer: apis exist to get broken
[21:57] Heine: Maybe an additional note in the upgrade docs.
[21:58] chx: kkaefer: Heine: we can _not_ have hook_menu(FALSE) called more than once, that'll will lead to wicked stuff
[21:58] chx: OR
[21:58] Heine: When ?q= changes hook_menu NEEDS to be called again
[21:58] chx: we need to ask people to take care not to reexecute their start code
[21:58] chx: but that'll get ugly
[21:59] chx: because core has lots o' css adding in hook menu FALSE
[21:59] Heine: or we should add a hook that is guaranteed to be executed only once
[21:59] chx: I vote in rolling back kkaefer's fix
[21:59] chx: Heine: hook_start
[21:59] Heine: yes
[21:59] chx: Heine: just rename the hook_init in common.inc
[21:59] Heine: much cleaner
[21:59] chx: Heine: piece of cake
[21:59] chx: do we want to do that past RC1???
[22:00] chx: what would rolling back that thingy cause?
[22:00] chx: what would be broken?
[22:00] Heine: Not sure what would be broken, but hook_menu(false) will not be correct anymore for changes via menu_set_active_item
[22:04] chx: there was a patch of mine
[22:04] chx: which builded on kkaefer's fix
[22:05] chx: / we do an internal redirect.
[22:05] chx: in user.module
[22:05] chx weeps
[22:05] Heine: I'll reopen http://drupal.org/node/88707
[22:05] chx: my poor little menu patch
[22:05] chx: it will be broken again :(
[22:05] chx: because I fail to see any other way out than hook_start
[22:06] chx: Heine: mention that user.module already has a fix built on it
[22:06] Heine: statics everywhere
[22:06] Heine: Where?
[22:07] chx: - Patch #88474 by chx, webernet, etc: fixed 'access denied' problem
[22:07] chx: this depends on 88707
[22:07] Heine: I see (in user_menu)
[22:07] chx: hm, instead of statics what about a $first which defaults to FALSE
[22:07] chx: so
[22:08] Heine: second param?
[22:08] chx: yes
[22:08] chx: menu_set_active_item calls _menu_append_contextual_items
[22:08] chx: one could add a parameter to the latter
[22:09] chx: which also defaults to FALSE
[22:09] chx: and then it could be passed along
[22:09] chx: because the first call will be from menu_get_menu and THERE you could add a TRUE
[22:09] chx: it will be then executed once per page
[22:09] chx: so you can wrap your start stuff in an if ($first) without keeping a static
[22:10] chx: AND in case you do not do this, not much will be broken
[22:10] chx: the solution requires little extra code and the API change is mostly BC
[22:10] Heine: chx, then I would advocate a hook_start.
[22:10] chx: a whole new hook after RC1?
[22:11] chx: well, if your stuff remains inside hook_menu
[22:11] chx: it will still work somewhat
[22:11] chx: only risk that it runs more than once
[22:11] chx: yes
[22:11] chx: Heine: post this discussion and let Dries choose
[22:11] Heine: if one doesn't include in $first one doesn't care about hook_start

hook_start sound good idea... however please check out node.module it uses a CSS to be added once and this one is only added once for sure...

@hass, that is due to the properties of drupal_add_css as node_menu is also run twice with $maycache = FALSE.

Note that in http://drupal.org/node/88474 we can use drupal_goto (external redirect) instead of the menu_set_active_item (internal redirect) and that patch was submitted in there as #6 and reviewed in #9 and even RTBC'd just it was said to be ugly. Ugly it may be but solution it is. If you roll this back and I will submit one on 88474 to this effect. This is the least code to be changed, also. It will be a regression because then the original bug here 403/404 pages can only be cached pages but ah well. You can code around it by adding node/403 in a hook_menu to $may_cache part. Drupal 6 will have absolutely no problem with this.

Rollbacked. Thanks.

After recently setting up a new installation of Drupal 5.0 Gold I've encountered this problem:

I create two pages. One with a URL ALIAS of "lost-404", the other "lost-403". NEITHER HAVE MENU LINKS.

Now when I set those to be my error pages (Administration > Site Configuration > error pages)

If I visit http://www.example.com/drupal/this-page-doesnt-exist the /node/ page is shown. NOT THE CUSTOM PAGE.

Now if I go in, create menu links to both the "lost-404" and "lost-403" page, and delete them... the custom pages are used.

Thus I believe that this bug should still be open.

Well, after more discussion in #drupal-support and more testing I believe I've narrowed it down to being an issue with aliased pages. I determined that by making another UNMENU'D page and setting it as my error page, this time using "node/4" rather than an alias (I didn't even give the page an alias). It worked.

So it works to use the node-path but not if you use an alias without a menu item. Hope this helps.

** closed again in favor of a new bug report: http://drupal.org/node/109748 **