Main menu

Thanks for the report!

I haven't found any particular reason in the code/document blocks that states that the menu entry should be in 'Main menu', but I'd go for this (it could be in User Menu or Management Menu also).

The one thing I wanted to check on this one: Has the menu name changed for all D7, or just for new installs? Does anybody know when this changed?

In my blog, which was updated to D7 a few months ago, I still have primary-links. In a more recent install, it's main-menu. If there is any chance that this name may be variable, I'd like a comment in the code about that.

The actual change took place in #270917: UB Usability : Labeling primary link label., but I'm not certain of the effect on upgraded sites. I suspect that upgraded sites *must* keep their menu names. Which means that this capability to wire in a default menu that may not exist may not be that good an idea. We should mention that in the comments.

Last track I've found is that it changed when the menu system was improved by a menu API at #473082: Add custom menu API. The patch at #80 was committed in #84.

The diff of the menu.inc file is here, but as you can see this is just a cosmetic array change.

http://drupalcode.org/viewvc/drupal/drupal/includes/menu.inc?pathrev=HEA...

The previous patch about it is in the issue #273137: Split Navigation to User and Administration menu at #78, committed at #80.

Posted by Dries on March 20, 2009 at 7:18pm
Status: needs review » fixed

I'd say it is an old change that will not happen again.

And I do have found this other issue: #277196: Undefined index: main-menu where catch mentiones that there is no upgrade path for menu names and that it is a bad idea to do have them.. From my understanding of the patch, the 'Main menu' is created even if primary links exists, am I wrong? I think it is safe to put the menu entry in "main menu", but I'm unsure how many installations will miss that menu (considering they are up to date, including the new menu.inc file).

No, a D6 site upgraded to D7 does not have a 'main-menu' and it does have a 'primary-links'.

The bottom line to me is: You can't assume that any menu exists in Drupal.

With this patch, what actually happens is pretty odd (and probably OK).

1. Standard D6 install (which has primary-links)
2. Upgrade to D7
3. Enable the patched menu_example

What happens? The main-menu (which doesn't exist) has an item added to it. The *block* for main-menu does exist in the D7 site, so our menu entry shows up even though there is not officially a main-menu.

I will file an issue against D7 for this.

As far as this patch: I think this patch is fine. But let's add to it a note that says "You can never assume that a given menu will exist on a given Drupal site, so use this with that in mind."

Created #1007910: D6->D7 update doesn't convert 'primary-links' menu to 'main-menu'

I see.. Ok, we can put some code logic to verify that the menu exists before putting the example element there, but that would also require some string change, I mean: A menu entry that is in another menu, but we need to tell the users wich one is that menu. Not sure how to handle this, saving a Drupal variable during install and showing it with drupal_set_message() in the module _info() page?

I don't really think we should make it more complicated by checking. Let that be an exercise for the reader. Let's just put a comment here that you can't count on any menu's existence.

On the other hand, stricly speaking, this module is an example of 'menu entries', I mean, it is not building a menu, but attaching links to any of the existing menues.

What if we introduce menu_save(), menu_load() and menu_delete() in a .install file to create a custom menu for the module, and put that 'problematic' entry in that menu to avoid errors? we can then put a sentence to clarify that the good practice is to put menu entries in existing menues, and use a custom menu only when required by design, and that it is a problem as nothing guaranties that the menu exists.

That actually sounds fantastic.

But let's make deliberate examples of menu_save() and friends, in a new .inc file, rather than just demonstrating them as a part of this little (mostly irrelevant) problem. Then if we want to deal with this problem, we can do it, but refer them to the simple, obvious example.

yeah, I clearly see your point on making this simple. However when working on this I've found other missing 'includes' that IMO should go into the menu example module.

- Creating menues (this would hardly fit in a .inc, as requires menu rebuilding if not set in a proper Drupal's flow that does this automatically). I'd say, Drupal has a UI to create menues, I'd not code another one, and for this I'd put it in a .install file with a 'first importante note: this is not required at all'.
- Other menu flags appart from default, callback and local task: see http://api.drupal.org/api/drupal/includes--menu.inc/group/menu_flags/7 (i.e. a good example is a menu not editable by administrators, show or not in breadcrumb).
- Menu system already includes hooks: hook_menu_insert, hook_menu_update, hook_menu_delete: do we left them out of the example?

I'm including the .install to see if we can make this simple. I trust on your criteria, rfay.

Yes, that works for me. I thought you were thinking of something more exotic. This works fine for me.

It's great usage explanation.

 * It is a Best Practice to include menu entries in existing menues, but it not
 * guarantied by Drupal that the menu exists in all the Drupal sites. A site 
 * administrator may have removed all the predefined menues and create new ones,
 * so everytime you put a menu link in a menu you are responsible of checking 
 * that the menu already exists.

'menu' and 'menus' throughout.

Instead of all this text I think just "In one example in hook_menu() a 'default menu' is provided. However, unless we create the menu ourselves, we don't know that it exists. So here we'll create the necessary menu."

    'menu_name' => 'example_menu',
    'title' => $t('Example menu'),

Let's use a prefixed machine name: menu_example_example_menu

  $menu = menu_load('example_menu');
  // At this point we can change the menu entry and save again, but in this case
  // we are only interested in removing it.
  menu_delete($menu);

Shouldn't we check to see if it still existed?

Powered by Dreditor.

ok ok ok! menu_example.module needs a comment too on the menu entry that even if we can use any menu there, we are using a custom menu because we can't guaranty that other menus exist. I'll rework this. Thanks rfay!

OK, I wrapped up all this input into a patch.

The most radical part is that I set the primary links to be the new menu we create. Originally, I had it set for the secondary links, but that ruined the testing. Ironically, the tests require that the secondary links be the user menu. :-)

Send word if this is too destructive. Otherwise I'll commit it in a few days.

A quick look-through says this is fine to me. It might be nice to see if the menu is already there in hook_install() before menu_save() on it.

Thanks!

Are you sure about replacing the user's primary links with ours?

You're right. I didn't notice that. Thought we were just populating our own menu.

Why don't we figure out how to show our own menu on the current theme, instead? There's no guarantee that the primary links will be showing anyway. So I think it *would* be OK to take *our* menu and show it on the screen. If they're using context instead of blocks so be it.

I think this is basically a duplicate of: #2079685: Add SimpleTest for base Examples module

Once the backport happens there, this issue will be satisfied.

Of course, the D7 branch of Examples is in minimal-maintenance mode, so there you go.