Can't manipulate Menu via plugin

Linc

In default.master.php:

$this->Menu->AddLink('Activity', T('Activity'), '/activity');

In Vanilla's class.hooks.php (abbreviated):

public function Base_Render_Before(&$Sender) { 
         $Sender->Menu->AddLink(T('Discussions'), T('Discussions'), '/discussions', FALSE);
}

In my plugin (after accounting for this bug):

public function Base_Render_Before(&$Sender) {
         $Sender->Menu->RemoveLinks('Activity');
   }

...does nothing, but it DOES work for 'Discussions'. Printing the Menu array reveals it only has 'Discussions' in it when Base_Render_Before fires. Why is that? How can I manipulate those elements pre-render if they're called in the View itself?

Linc

Am I off base to suggest that Menu construction should be happening in Dashboard's class.hooks.php and that there should be event fires in between each menu item so we can target where we want ours to go? (or some way of reordering them pre-render)

Linc

Kinda related too, since it involves menu building and positioning of custom groups/links: http://vanillaforums.org/discussion/11302

Linc

This is the root cause of issue #407.

Linc

This should fix it. http://github.com/lincolnwebs/Garden/commit/b12aee1e5f710cb56eea421554da8774012206bf

Linc

@Tim - Can we get the main menu creation bumped to the Controller? See last post.

Tim

This is a valid question and some good suggestions. Let me talk to @Mark and see what his opinion is.

Mark

I had done it that way originally (in the hooks file), but the complaints I got was that themers just do not understand this at all. Themers come in, look at the master view and say, "Where the hell is the menu? I just want to add one thing to it."

We realized that in Vanilla 1 we had gone this way (allowing people to add to the main menu programmatically), and very rarely did people ever do it.

The themer-vs-plugin-developer war has officially begun.

Linc

I don't see how having it in the controller would stop a themer from adding their items directly in the view. Clearly the AddItem command would still work anywhere before the ToString call in default.master.

Linc

Suggestion: before ToString() in default.master add an explanation

/** You could add your custom menu items here like this: **/

and everyone wins.

With it halfway like this the themers still have to figure out the functions and the developers are crippled - half the menu functions don't work as expected.

Linc

@Mark If the menu build is in the controller, themers can still add/remove menu items in the view with a template hack, and developers retain full control of the menu via plugin.

If the menu build is in the view, what to do becomes more obvious to the themer but doesn't actually help him do it easier. Meanwhile the developer has lost all ability to use the menu building functions.

If the only thing being gained by the move to View was obviousness, couldn't the same be accomplished with a comment in the default.master template (as above)?

Linc

If there's a war but only one side shows up, do they get their suggestion pulled into core? hehe

Linc

@Tim Pssst... I'm not above bribing for a pull.

:ninja:

Tim

@Lincoln

Yo. So since you brought up the subject of bribery (the magic words), we discussed how to solve this problem.

Currently the default master consists of a bunch of Menu->Addlink calls, followed immediately by a call to Menu->ToString. This leaves no way for plugins to modify menu contents or sort order. Internal to ToString() is a sorting function which looks at Garden.Menu.Sort (defined in config-defaults) and sorts the menu based on that.

We were thinking that changes could be made to the Menu module so that inside ToString there could be a FireEvent which could optionally provide a new sorting order array, as well as adding new menu items.

What do you think?

Linc

@Tim I guess there's no programmatic difference between hooking into Base_Render_Before vs. a fired event so if you guys like that solution better I don't see a reason it wouldn't work.

I still think it's oddly inconsistent though - now you'll be able to ADD to the nav menu with Base_Render_Before but only remove/sort SOME of them, whereas using the special hook instead will let you do all of it. You're gonna need a paragraph on the wiki just to explain the edge case you're creating.

Tim

You'll be able to have full control over the menu with the event. The internal list will be final by the time the event fires, at which point the plugin can do whatever magic it wants.

Linc

@Tim I understand that. I'm saying that by doing it that way you're still leaving inconsistent behavior for someone who tries to use Base_Render_Before. I suspect you will get confused people because any OTHER menus / modules can be manipulated by Base_Render_Before; this will remain the exception to the rule.

It fixes what I need fixed. It just feels hacky.

If it's a fired event or nothing, add the FireEvent pls.

Tim

I hear what you're saying. The reason we're thinking this way is better is because it lets the themers retain access to the menu while giving plugins the ability as well. Technically, hooking into Base_Render is kind of a kludgy way of doing things anyway, since its like "HOOK EVERYTHING" and then you do some random thing. I like having a specific event to hook for modifying the menu.

Linc

Understood, I just don't have enough sympathy for dirty themers I guess.

But does @Mark know you just called his magical methods kludgy?? :-o

Tim

Shh DAMN YOU FOR @MENTIONING HIM