multiple menu support

Thanks for taking this issue. We discussed here a bit. Two challenges: menu module needs to edit menus defined by other modules. And menu titles are highly dynamic, the navigation block is titled check_plain($user->name)

Two tables, yes, sure. In the beginning I was even contemplating (and mailing devel) about splitting the hooks into hook_router and hook_menu but later realized that's not necessary or good so I contemplated two tables which indeed became necessary. Rock on.

You mean like this?

INSERT INTO foo (a, b) VALUES (1, 2), VALUES (3, 4), VALUES (5, 6);

That's MySQL-specific, so no, that's no good. Some other database engines allow multiple full INSERT statements in one query command, but MySQL does not for injection-prevention purposes. Just stick with one query call per insert.

menu_rebuild() is, hopefully, a rarely called function so its performance is not super-critical.

user_current_to_arg is designed to always return the current user id. That's how my account link works even on user/564854 pages. _menu_check_access does not make any sense to, i surely miss something. I do not readily get what are you doing with admin/*, even if we decide it's a good idea, it's a separate patch, but while yes, it is similar to menu tree, the descriptions make it distinctive.

Otherwise, it's a fantastic piece of effort. Thank you really much.

Changing menu_tree to return a structure -- I do not like the idea, book/outline module can easily reuse the menu table itself, it's trivial to write a query which gets the items in menu order.

Let's make the menu_tree structure a separate issue, OK? I was also thinking of somewhat doing array-ish things in there. Let's dissect _menu_translate on a MENU_HANDLE_REQUEST . First of all, $path_map becomes map which is the exploded stuff of the current path. Second, to_arg_function is not called at all, there is a if ($operation == MENU_RENDER_LINK) which stops that. It's the object loading calls that happen all the time because the access checks needs those. I am sorry if the menu handbook is confusing, I will look at it. I do not readily see what gets broken, I tested that if I am user 1 and go to user/2/edit then my account still points to user/1 and all the tabs point to user/2. This was some time ago, but I believe it still stands.

This all can be cleaned up, lots after or during this patch because we longer need to mesh the user/%user router item with the user/_actual_user_id_ menu link. Yay.

subscribing

I can see your problem but first can we get the split storage and the multiple menus working and more on later issues? We need to support multiple menus in menu.module , too... optimization can come later -- not just can, it needs to, but first let's get this one out of the door.

pwolanin asked me to come and help out get this patch in line with the latest menu localization changes, but as it seems from his latest patch, the title and description translation code is already there, so I don't see what is the supposed conflict or problem with menu localization?!

Oh, I got an email from pwolanin, which I will repeat here, so it is saved for history.

Hi,

pwolanin wrote:
> regarding this patch: http://drupal.org/node/128082
>
> I'm trying to figure out how to merge it in with multiple menu patch I've
> been working on with advice from Karoly: http://drupal.org/node/137767
>
> In particular, which the new locale system should t() be called with
> user-entered text. For example, when the user enters a link and the text
> for the link, should t() be called on that as well as on the title? I
> guess a roughly equal question is whether t() now gets called on node
> titles?
>
>> From this: http://api.drupal.org/api/HEAD/function/node_page_view
> It doesn't seem to be the case.

The new menu localization system makes no difference between built-in menu items and user defined menu items, when it comes to localization. This is because we have no solution yet to translate user defined interface elements, but are in the works with one. If Drupal 6 will not include a user defined interface translation solution, we can still pull the plug on t()-ing user defined text, until that this is an accepted wrong way.

I am equally fine with checking for custom vs. built-in menu items, and only translate built in ones for now. Then put a TODO note into the comment in the "else" branch of the code, that there comes the user defined translation :)

We are still working out performance issues with user defined text translation (otherwise our model is very simple and works nicely). I hope we will have it done in time for Drupal 6.

Gabor

Ooooooh! You figured how to do the data collection-HTML separately? My brain exploded when I tried. How very nice. We will be able to reuse that in so many ways. Thanks!

Awesome! This version removes an empty Navigation block, and fixes system_admin_menu_block .

Another round of patch. This one introduces the oh-so-important menu_link_save API function. The feature set is at such a point that now I would like to see this one fixed, ironed out (like writing an UPDATE statement), etc. and later, in separate patches menu.module and book.module can use that menu_link_save . I have a text that explains the maths involved.

I am not so sure our work is so much easier if we restrict router items to the navigation menu. You can't delete everything from there the same as you can't delete any other menu. So I have removed this. However, with tasks, breadcrumb, title and navigation menu all working I am deeming this ready to review. Menu module supports come later. I added a module column to menu_links so that for example menu module does not start to customize book module links...

I am very happy how our duo was working on this big, big rehaul. It's good to have another man on board -- it makes our code even better. Instead of cleaning up in Drupal 7, Peter cleaned up (and/or forced me to clean up) the code. Fantastic!

There is still work to do in other patches:

1. menu module needs to completed
2. Book module now can use menu!
3. update path needs to be written
4. we still need permissions that let you reach the various admin pages but nothing else, because of the loss of the bubbling effect
5. the access checking needs to be deferred after _menu_tree_data so that becomes cacheable

Another action point: we need to check all menu_rebuild calls in core and remove them in favor of menu_link_save calls.

There is time for refinement later.

  $placeholders = implode(', ', array_fill(0, count($menu), "'%s'"));
  // Remove those items which router path does not exist any more.
  db_query('DELETE FROM {menu_links} WHERE router_path NOT IN ('. $placeholders .')', array_keys($menu));

What's your problem?

It was deliberate to leave out the module column from update -- you never want to update that.

I like Peter's method. It's indeed a clever trick using the fact that we know the max depth of our tree.

When enabling menu.module, you get:

Fatal error: Call to undefined function _menu_primary_links() in /Applications/MAMP/htdocs/head/includes/menu.inc on line 734

I was told by chx not to enable menu module, but since this is added by this patch, it probably should have a stub function or something as a safety.

Also, there is no update path. chx tells me that that will be added after this patch is committed.

When I enabled 'create book' permissions for authenticated users, neither "Create content" nor "Create book page" appears.

I clicked around to a bunch of stuff and it otherwise appeared to be in order, but I wasn't able to give it my total full attention so probably needs another set of eyes.

I believe that if you can get

When I enabled 'create book' permissions for authenticated users, neither "Create content" nor "Create book page" appears.

this one fixed, then it is good to go -- we have the backend in place, let's get it moving.

Should the code that deletes links if their callbacks are gone also try to re-parent any orphaned children? Or maybe these links should be disabled rather than deleted?

This falls into not babysitting broken stuff. But then again, a user might just move his / her stuff in there. Disabling won't work, we do not bubble, reparenting can be much, much work... let's give this more thought. Maybe anoher issue.

re-parenting needs to be implemented and tested.

Definitely a whole another issue. I believe the next two issues, in parallel are upgrade path and reparenting.

some of the admin/log/X items end up in the {menu} table with access_callback == 0

I believe some admin log paths are botched, there was already an issue for them. Please look at this.

The active trail includes the current page, so the last item in the breadcrumb is the current, rather than parent, page.

That's easy to fix...

should the allowed depth be > 6?

No.

Are the table indices working? We need to generate a reasonably large {menu_links} table to test it.

I will do that -- I will import (a part of) the Open Directory RDF structure dump.

When inserting a new menu item, do we need to check that the parent has depth < 6?

Yes, return FALSE if we can't save it.

When re-parenting, do we need to check that neither that item or any of its children will have depth > 6? What should the code do in this case?

Reparenting is another issue :)

Dmitrig01 reported http://dmitrizone.com/dump/Picture%202_.png .

I forgot one case in the menu tree hoarding, when you need to fail back more than one level, in the default install admin/user/user and logout just happens to stand like that. I removed ksort calls for testing ran insert into menu_links (menu_name, mlid, plid, href, router_path, disabled, external, has_children, expanded, weight, depth, p1, p2, p3, p4, p5, p6, module, link_title, options) values ('navigation', 500, 41, 'admin/user/user/1', 'admin/user/user', 0, 0, 0, 0, 0, 4, 1, 9, 41, 500, 0, 0, 'system', 'Users1', 'a:0:{}'); update menu_links set has_children = 1 where mlid = 41; and got the attached picture -- which is what we expect. Dmitri's bug is now fixed.

He also reported another, I will promptly fix that and then submit the patch.

So this patch contains two bug fixes, one detailed above and the callback tree needed sorting, I have added that, this fixed some problems when access callbacks became 0 like on admin/user/profile.

Peter, sorry, but this is not how expanded needs to be handled. See my patch. Expanded does not require new processing code, just iterating a new query if there are expanded items. I never wanted to write this myself, but ah well, it's not too complicated...

Screenshot of expanded in action.

Going through, found two more issues:
- My recent posts is gone from tracker, and after visiting any subsequent page, you get:

 # warning: Invalid argument supplied for foreach() in /Applications/MAMP/htdocs/head/modules/user/user.module on line 92.
 # user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1 query: SELECT * FROM users u WHERE in /Applications/MAMP/htdocs/head/includes/database.mysql.inc on line 161

... chx is fixing now.
- Attempting to add a path breaks horribly:

user warning: Unknown column 'link_path' in 'field list' query: UPDATE menu SET link_path = 'smooky' WHERE path = 'node/1' in /Applications/MAMP/htdocs/head/includes/database.mysql.inc on line 161.

chx claims it should not be trying to update menu at all, but menu_links. menu_links has no link_path field. it has a router_path field, but then it also has no path field. I gave up trying to fix it.

Two others I forgot about:
- Apostrophes are double-escaped when used as a menu title (made a profile category "Let's dance" that came out as HTML entities for the apostrophe)
- There are a couple notices as search/user:

notice: Undefined offset: 2 in /Applications/MAMP/htdocs/head/includes/menu.inc on line 443.
notice: Undefined offset: 2 in /Applications/MAMP/htdocs/head/includes/menu.inc on line 443.

search/user/ is fine.

Ahem. search/user/[somename] is fine.

This patch does not serve aliased links at this point, so I removed the path code. I needed to add dynamic translation back to menu_translate but only when tabs are rendered. There is no a function which is used by menu_translate and menu_link_translate. This fixed the search tabs as well. The double escape I am not fixing it's a side effect of the localization patch: the title callback is check_plain and then we run it through l(). I will talk with Goba what's the best avenue to fix it.

I am RTBC'ing at this point because I fixed everything webchick was able to find with one sweeping fix -- actually since the moment I saw the changed menu_translate it nagged me.

Let me sum up what we have gained with this patch: an API which lets you add links to any menu and the ability to render said menu. As a bonus, we got a lot of things, most important is one query for tabs instead of many and expanded support.

Clearly, a lot of hard work went into this patch. That's great! :)

But, boy, this is a _lot_ of code, and much of this is hard to understand. It will take me several iterations to get this review right. I spent 45 minutes reviewing it and I doesn't feel like this review is particularly useful. I'll need to spend more time with it, but I have to run now.

My overall, initial reaction is: poorly documented. With better documentation and code comments, I'd have been able to get a lot more done in 45 minutes.

In fact, I can't comment on the engineering/architecture yet, as I've yet to figure out how exactly everything works. The lack of good code comments are a bit problematic. Without better code comments, I'll have to sit down and spend some hours reviewing this. I'll want to find a free evening for that.

For outsiders, the old menu system was hard to understand. But for outsiders, the new menu system also risks being hard to understand. At least, that's how I feel now, but I'm positive that I'll see how it all works fairly soon. ;-)

1. For example, _menu_translate() is documented to return a $map object but it can also return FALSE (which is not documented).

2. In the documentation '$to_arg_functions' should probably be '$to_arg functions' (space) as the variable is called '$to_arg' and not '$to_arg_functions'.

3. Documentation is sparse at points. What does _menu_link_flatten do, and how?

4. "Returns a rendered menu tree based on the current page." -- while I understand what you mean, it could do with being a bit more verbose. "based on the current page" might not be explanatory for everyone.

5. Shouldn't "menu_tree_output" be an internal function? I.e. "_menu_tree_output"? The function's PHPdoc does not help me understand it either. I had to read to code to understand that it is a helper function of menu_tree().

6. "Get the data representing a named menu tree". What is 'the data'? The stuff? :)

7. If would help if you could document -- in the code -- how the algorithm works. As this is what you are soliciting feedback for, it would help reviewers do their thing.

8. What is the performance impact of this patch? On a small or medium sized Drupal site with a dozen menu items? How much extra queries does it add? It seems to be database heavy. Will it slow down or speed up Drupal?

9. There are about a dozen TODOs in the code. Sure this is RTBC?

10. _menu_router_build seems key and it has no code comments whatsoever.

To make a long story short. I only had a first look at it, and I think I understand the bigger picture. I'll need to dive in and inspect the different bits and pieces into more detail. If you want to help me do that, focus on writing better code comments (this is going to be a requirement anyway).

Either way, good job on taking on this legendary task. Most of what I saw, makes sense -- I think. It's just that I have to convince myself a bit more/better by spending more time with this patch.

Peter, I believe there is no difference between an expanded tree and a tree which is just there. Think of it, when you stand on administer, you see tree elements below admin , so what? Breadcrumbs will work, why not? The menu_set_active_trail will iterate down until the current path/tab root and that's it. Expanded does not differ from what we already have.

Finally, I'll look further, but I don't understand why this code every needs to get the path alias if the links for tabs, etc are being generated via calls to l() which should look up the path

I do not understand a single word. The code does not get the path alias. What I said is that previous versions (like HEAD) had the ability to store aliased links. This is lost. To reintroduce, we need to look at path code. Later.

3. Documentation is sparse at points. What does _menu_link_flatten do, and how?

You found the only function which is not documented to death. Congratulations! It's an internal function.

TODOs are further patches.

_menu_router_build is also an internal function which you should never ever call and it has comments inside.

If someone else -- let's hope Peter -- wants to add even more comments, be my guest, but my time is limited at this point.

Let me make myself clear: my core coding time for the next two or four days is zero. And no, I have not checked help.

Peter, I believe only more comments per Dries' call and help texts by your call need attention. Everything else can come after. Code freeze is approaching and this patch is already huge.

There are still some issues with expanded and breadcrumb IMO:

- Administer > Categories > Edit a vocabulary
- Administer > Input formats > configure an input format
- Administer > Users > edit an user

(Note: patch #67 applies to #HEAD with 3 lines offset in user.module)

If needed just rip the ~!@#$%^&*() expanded out of the patch , I totally did not want to do it (ever) especially not part of this patch but as Peter added it I had little choice but recode it.

I fixed riccardoR's finding which obviously had nothing to do with the expanded code. I added a billion more comments. Re. DB heavy, not at all, actually, tabs are now cheaper. Why have you thought this is DB heavy? Expanded can cost you a few queries (max five) but -- there is a price to be paid for exotic features. Normally, not a single expanded query is executed. _menu_router_build is internal, there are comments inside where needed.

And no, we do not ask anyone to comment on the algorithm -- if we would have kept the matrix implementation you would expect them to review that? We agreed on this is not necessary. We are asking whether it works and it does. And unless you ask for even more comment, it is ready.

One thing that I did notice was that this patch, although it looks huge (especially in terms of Kb) actually removes a significant number of lines.

Here is the ~count:
-628
+969

i.e. a total of 341 new lines of code, which I don't think is too bad, considering the amount of functionality this (re)enables.

@chx, @pwolanin: I am sorry for having created a misunderstanding with the word "expanded".
I have just realized that you were talking about the expanded link flag, while my feedback was about the normal tree expansion when navigating down the menu.

As I am here, I can confirm that the last patch fixes my finding.
Thanks

I agree wholeheartedly with your changes -- but this

+  // Keep track of which menus have expanded items
+  if ($item['expanded']) {

part is not needed -- you always want to check for expanded menu names because what if I just switched off the last expanded menu item? (BTW. if someone is concerned why my patches are smaller, that's because bzr puts smaller header per files, but the patch itself is the same)

Is it normal that the menu module is still broken? I applied the patch, did a fresh installation and went to the menu administration page: Unknown column 'visible' in 'where clause' query: SELECT m.*, me.disabled FROM menu m LEFT JOIN menu_custom me ON m.path = me.path WHERE visible = 1 OR (disabled = 1 AND admin = 0) ORDER BY mleft in includes/database.mysql.inc on line 161.

Other than the broken menu administration page, this patch seems to work. ;-)

Yes menu is module broken.

Tried to look through what is happening in this patch. But it does seem to have evolved a lot farther then what chx set out in his five initial tasks. Maybe we can get an overview of what is happening here. I see a lot of changes are around, but maybe we can get a short architectural overview. Sure pwolanin and chx knows what concepts and approaches are changed, but I cannot make it from the patch.

I also noted 13 TODO items in the latest patch, some in groups of three, one in an empty stub function. Are these supposed to be filled in with later improvements in the 6.x timeframe?

The patch has nothing to do with the original post... There was a TODO in HEAD (and a mail to devel, long ago) asking for splitting router and link items and Peter implemented this. This lets us scale the new menu_links table to quite new sizes -- book with use this. As a collateral, multiple menus are supported.

I would be useful if we could add that high-level discription (router vs menus) at the top of menu.inc.

Also, if {menu} is use for router-stuff, why not call the table {router}? And if {menu_links} is used for menu-stuff, why not call that {menu}? :-)

Added - changed comments to the top, changed table names. I truly hope this gets in now...

Perhaps my opinion doesn't matter that much, because I totally have no clue how the new menu-stuff works atm but wouldn't it be nicer if we prefix the new created tables with the modulename?
So that would make {menu_router} and {menu_links}.
I would love to see this for every core/contrib module, so examinating the database would be pretty easy and much easier to have a clue where to start looking at in the first place.

Just a thought, you can safely ignore this if you want to...

We never had anything similar, splitting a table by module. We can do that later for now all I care about is getting it in fast.

Please, please get off table renaming especially sweeping ones and get this in finally!

It applies still but not cleanly so I reroll.

I looked at the code once more, and I'm OK with it although I'm still not super-excited about the code comments. The function explanation seems decent, but the code comments in the function bodies seems somewhat lacking.

My review:

1. In the code comments, 'fail' should probably be 'fall'.

2. Elsewhere in Drupal core's database schema we use 'link' and not 'href' to denote a hyperlink.

3. 'user..' should be 'user.'.

4. "This function translates the objects in the map to current values with helper functions." -- Can we make this slightly more concrete? What are 'current values with helper functions'?

5. "When rendering a menu link, this function is similar to _menu_translate() but does other link-specific preparation." -- What is this supposed to mean? Please specify how it is different. In other words, what are these 'other link-specific preparations'?

6. "and dynamic paths are also changed accordingly." -- What do you mean with this? How are dynamic paths changed, and why?

7. "Build the data representing a menu tree" -- Sentence lacks trailing point.

8. In the comment module we use 'cid' and 'pid'. Here we use 'mlid' and 'plid'. I'd prefer to use 'mid' and 'pid' here.

9. 'has_children' should be 'children' -- we don't use 'is_hidden' either. We simply use 'hidden'.

10. 'link_title' should be 'title'.

Also, I'd like one other core developer to review the next iteration of this patch before I commit it. This will help guarantee that the code (and the code comments) make the new menu system accessible to new people. I want this person to look at the code changes and the code comments, not just click around a bit. I think this is important because we want to avoid a scenario where only 1 or 2 people can debug a menu system problem. This is an important patch so I'd appreciate it this could bubble to the top of our todo lists. Thanks.

Needless to say, this patch is pretty an amazing step forward. We're just trying to perfect it through review. :)

    // If the link title matches that of a router item, localize it.
    if (isset($item->title) && ($item->title == $item->link_title)) {

I would keep title and link_title separate. For the other requests, I am too tired to even try to explain this in code comments properly. We have a handbook page and that should suffice . But of course not. However, I am not going to write these comments today. Or ever. I do not know.

Yes. I stand for mlid/plid. At one point we were discussing this patch over the phone and my mind just blowed from the name of the identifiers. Imagine someone who is not neck deep into this.

#9. 'has_children' should be 'children' -- we don't use 'is_hidden' either. We simply use 'hidden'.

No. Look at #children in drupal_render -- from 'children' I would expect to hold the actual children, this is just a flag.

Worked on the comments w/ Andre Molnar.

fixed a couple of tiny typos

I don't think my previous review is properly addressed. Some of them still stand.

I'd like to see has_children renamed to children, and I'd like to see mlid and plid renamed to mid and pid, respectively.

Why do both routers and menu items need titles?

It's not clear whether andre actually understood the menu system and/or approves the changes.

Children is expected to be a data structure that holds stuff with a common parent. is_children is a flag whether such entities exist. title in menu_router is defined in hook_menu, can be run through t(), title menu_links is entered by the user, not suited for t(). Andre seemed to understand the changes...

Also, given that menu_router and menu_item get joined occassionally, renaming mlid and plid to mid and pid is a very bad idea, I believe.

Patch no longer applies because of http://drupal.org/node/137767 committed to HEAD.

Patching file 'profiles/default/default.profile'
Hunk #1 failed at line 111

I've just noticed that a warning is displayed every time menu_rebuild() gets called :

user warning: Table 'drupal6.menu' doesn't exist query: DELETE FROM menu in D:\var\htdocs\drupal_head\includes\database.mysql.inc on line 172.

The reason is menu_cache_clear_all() still referencing the old menu table.

function menu_cache_clear_all() {
  cache_clear_all('*', 'menu', TRUE);
}

The following modification seems to work correctly:

function menu_cache_clear_all() {
  cache_clear_all('*', 'menu_links', TRUE);
  cache_clear_all('*', 'menu_router', TRUE);
}

I just went through and read menu.inc pretty much top to bottom. I think it has to be the most documented component we have now. :-) By and large I do understand what is happening (although I was at chx's talk at DrupalCon), and like what I see.

Regarding mlid & plid: I say keep them as is. Requiring 3 character primary keys leads to far too much confusion, especially if one has to start joining things. vid and vid is annoying enough. We don't need mid and mid and pid and pid, too.

Regarding has_children: $has_children is used throughout the PHP code. The database column should match it for clarity. Using $children would be horribly confusing when dealing with this sort of data structure. I don't have a problem with a has_children boolean column.

Attached patch is a reroll against HEAD that fixes the items riccardo noted.

There are several typos and a modified settings.php included in #120. A lot of the comments would look better if they included a period to end the sentence too.

includes/menu.inc

If the link title matches that of a router item, locaize it. -> localize

An error occured loading an object -> occurred

see http://drupal.org/node/141866 for more. -> more information?

Don't show a link to the current page in the bc trail -> breadcrumb

We are on a tab -> should be on a new line.

TODO how do we hadle the case -> handle

theme_menu_local_tasks() -> doxygen indentation is off by one space.

Remove those items which router path does not exist any more -> Remove items if their router path does not exist any more

Mandatory key is only href and link_title. -> The only mandatory keys are href and link_title.

Find the router path which will serve this paths. -> "these paths" or "this path"

separate callbacks from pathes, making pathes ready for matching -> paths

Again I feel like in http://drupal.org/node/77919#comment-125579 -- why it is always my patches that get scrutinized to the last comma when incredibly broken stuff can get in just to be removed later (I will not name specifics, but it did happen lately). Nevermind. I am just grumpy as usual.

Really nevermind. Drupal will get better and at one point I will get the patience needed for it... just there is little time until the freeze (my linked comment was also before a bit more than ten days before the freeze) and there is so much to be done.

Alright, committed to CVS HEAD. :)

Marking this as 'code needs work' until the documentation has been written.

This prevents HEAD from installing on postgres. Patch attached. The patch makes it install cleanly but I can't promise there are not other pgsql-related issues.

FYI, now that I'm paying careful attention to pgsql, I've noticed several core patches recently that create id columns as type int, not type serial, for pgsql. Remember, db_next_id() for pgsql *requires* that any column it is called to generate an id for be type 'serial'. This requirement has nothing to do with the schema patch.

Committed. Thanks! :)