Menu - Fixes that make menu module work in D6

just found this problem while trying to edit a dynamic item: ('My Account', 'user/%')
on submitting the form a foreach error occurs due fact that the path gets passed to user_load since user_load does not check if the % is not integer it errors out...this and similar problem is solved here: by http://drupal.org/node/148678
On applying that patch though a further error occurs because the function menu_get_item will return falls for paths that contain dynamic display.
I explained this a bit more @ http://drupal.org/node/148678#comment-259354

first of I think i found another bug, when editing items descriptions are not saved.

in regards to the problem of dynamic links i have tried attacking (i remember vaguelly talking about with pwolanin/chx the other night) it by creating a function that checks whethere link_path is dynamic or not (this would replace $item = menu_get_item($path) -> $item = menu_valid_path($path in the _validate ) ),

function menu_valid_path($path){
  $dynamic = FALSE;
  if (preg_match('/\/\%/', $path)) {
    $dynamic = TRUE;
  }
  
  if ($dynamic){
    $item = db_fetch_array(db_query("SELECT * FROM {menu_router} where path = '%s' ",$path));
    // $item['access'] needs to be defined here
  }
  else {
    $item = menu_get_item($path);
  }

  return $item;
}

The only problem is i cant seem to figure out an easy way to check the access of a dynamic link....:/

rerolled against head, also contains fix bug reported in #3.

Confirmed that this also fixes the notice: Undefined index: delete message when creating a node as noted in http://drupal.org/node/152281

hmm i was trying to trace the disappearing description problem but it seems that it might have been caused by something on my system (me messing about :P).
installed this patch on fresh head and descriptions are saved fine.

Editing "My accounts" item title:
- notice: Undefined index: options in \includes\menu.inc on line 539.
- redirects to "admin/build/menu/navigation" instead of "admin/build/menu-customize/navigation"

Disabling "My accounts":
- redirects to "admin/build/menu/navigation" instead of "admin/build/menu-customize/navigation"
- item is not flagged as customized (therefore no reset link)

Adding an item from "admin/build/menu-customize/navigation/add":
- warning: mysqli_real_escape_string() expects parameter 2 to be string, array given in \includes\database.mysqli.inc on line 362.
- The entire list of items in Navigation ("admin/build/menu-customize/navigation") disappears after creating a menu item for node/1

Adding an item from "node/1/edit":
- OK
- deleted OK from "node/1/edit"
- delted OK from "admin/build/menu-customize/navigation"

As noted in the original issue - Access denied pages cause the top-level (parent) Navigation items to appear as both the primary and secondary links.

General:
- Redirects go to "admin/build/menu/navigation" instead of "admin/build/menu-customize/navigation"
- Disabling/Enabling system menu items doesn't flag them as customized, therefore no reset link.
- Why can I delete the navigation, primary links, and secondary links menu items?

Editing a menu item:
- warning: Missing argument 4 for menu_edit_item() in \modules\menu\menu.module on line 248.
- Only possible parents for any menu item are "Root" and any disabled item (ie. "Compose tips").
- After saving a change --> notice: Undefined index: options in \includes\menu.inc on line 539.

On install:
* user warning: Unknown column 'mlid' in 'field list' query: INSERT INTO menu_custom (menu_name, title, description, mlid) VALUES ('navigation', 'Navigation', 'The navigation menu is provided by Drupal and is the main interactive menu for any site. It is usually the only menu that contains personalized links for authenticated users, and is often not even visible to anonymous users.', 0) in \includes\database.mysqli.inc on line 154.
* user warning: Unknown column 'mlid' in 'field list' query: INSERT INTO menu_custom (menu_name, title, description, mlid) VALUES ('primary-links', 'Primary links', 'Primary links are often used at the theme layer to show the major sections of a site. A typical representation for primary links would be tabs along the top.', 0) in \includes\database.mysqli.inc on line 154.
* user warning: Unknown column 'mlid' in 'field list' query: INSERT INTO menu_custom (menu_name, title, description, mlid) VALUES ('secondary-links', 'Secondary links', 'Secondary links are often used for pages like legal notices, contact details, and other secondary navigation items that play a lesser role than primary links', 0) in \includes\database.mysqli.inc on line 154.

On /admin/build/menu:
* notice: Undefined variable: content in \modules\menu\menu.module on line 154.

Getting closer... ;)

Editing the "My account" menu item (other items appear OK):
* notice: Undefined index: options in \includes\menu.inc on line 539.

Reseting a parent menu item:
- Made it appear as though it wasn't a parent (no expansion arrow / empty expanded column), even though it still has children.

Disabling "Administer":
* notice: Uninitialized string offset: 0 in \includes\menu.inc on line 506.
* notice: Uninitialized string offset: 0 in \includes\menu.inc on line 513.
* notice: Uninitialized string offset: 0 in \includes\menu.inc on line 514.
* notice: Undefined index: load_functions in \includes\menu.inc on line 330.
* notice: Undefined index: access_callback in \includes\menu.inc on line 363.
* notice: Undefined index: access_arguments in \includes\menu.inc on line 369.
* warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, '' was given in \includes\menu.inc on line 376.
* notice: Undefined index: link_title in \includes\menu.inc on line 536.
* notice: Undefined index: options in \includes\menu.inc on line 539.

Applying this patch on a fresh HEAD checkout cured at least the two following issues for me:

• pre-patch, editing a newly created node resulted in node system paths being displayed by path module, but without a path Title (a required field on this form); one would have to enter a Title to resave the node
• pre-patch, menu items created to lead to new nodes had very strange vertical spacing both before and after the item, at least on Firefox

Clean install --> "Menus" item appears as though it isn't a parent (no expansion arrow / empty expanded column), even though it has children.

After Editing "My account": notice: Undefined index: options in \includes\menu.inc on line 539.

Can't add a link to the front page (path=)

Delete a menu item for node/1 from admin/build/menu-customize/navigation while node/1/edit is open. Check the delete box on node/1/edit and submit. Item is recreated! (Expected: Item remains deleted and possibly provide a message)

Delete a menu item from node/1/edit while admin/build/menu-customize/navigation is open. Attempt to delete from admin/build/menu-customize/navigation. Results in "Page not found". (Expected: Notification/error message)

Attempted to delete a system menu item by going directly to admin/build/menu/item/51/delete. Confirm form is displayed, click delete, "The menu item Themes has been deleted." is displayed, although it wasn't actually deleted. (Expected: "Access denied")

Everything seems to be working properly - I haven't come across any errors/warnings/notices.

No longer applies cleanly, needs to be rerolled.

Retested - No errors/warnings/notices that I can find.

If it works -- the code looks good.

I've just gone through and added a number of items to several different menus, created a custom menu of my own, etc. Things seem to be working fine, although using wildcards still generates an error. To see this in action, add user/% to the primary links menu:

notice: Undefined index: external in /Users/jeff/Sites/menu/modules/menu/menu.module on line 702.

Adding the item DOES work, though, and permissions etc are all applied properly.

That fixes the only problem I was able to find when working my way through. Kudos.

A big +1. Menu editing is way, way cleaner now.

I'm probably not looking hard enough, but is $form_item['original_item']['external'] ever set when adding a new item?

What I'm seeing is this:

257: The edit form builder initializes an empty menu item. It doesn't have an 'external' key.
264: The edit form builder puts the item into $form['original_item'].
326: The form validator calls menu_valid_path on the form values.
702: menu_valid_path tries to access the 'external' key of 'original_item'.

Should the 'external' key be placed into the initializing array in the form builder?

Whoops, too slow. Never mind.

If the rollback had any actual logical impact, then the patch attached will not work correctly (even worse, it'll seem to apply cleanly and then break the system without obvious reasons); because I haven't understood a tenth of the new menu system.

But if, as it looked to me, it was merely a matter of a few collisions where the same line was changed in both modifications, then this should be a clean (if painstakingly resolved) re-roll.

I've tested this patch, especially focusing on changing, saving and deleting a menu link since those were the hunks that failed. All worked for me.

*Sorry, by logical impact, I mean impact on the logic changes made by this patch. Also, forgot to set the flag back to CNR.

*Sorry, by logical impact, I mean impact on the logic changes made by this patch. Also, forgot to set the flag back to CNR.

CNW was actually correct, since my version still incorrectly used the deletion API in nodeapi. Ironically, this change was one of the hunks that *didn't* get broken by the rollback, so I didn't notice it on resolving only the conflicts. I also didn't think of testing the node-creation/deletion of menu links, or I would have noticed.

I've put the code of the deletion callback back into the 'delete' case of nodeapi, and removed the callback; now it should be fine.

menu.inc was put under the spell-checker, and it just so happened to be that this patch appended a period to one of the misspelled comments, causing a conflict.

Re-roll fixes this. I hope this patch can go in soon, or we'll spend more time re-rolling than actually improving it. ;)

Bugs fixed in this patch:
- Menus link in admin overview said ". as well as" instead of ", as well as".
- /admin/build/menu/item/22/reset says Array for confirm form text, link and submit handler redirect were missing trailing slash, submit handler included hardcoded "navigation"
- Changed "Root" in parent select to "" to match what taxonomy.module uses
- Fixed an extra "." in Doxygen
- Moved menu OTF fieldset down as first below body, does not belong below title IMO
- Renamed OTF fieldset to "Menu settings" as "for this post" seems redundant to me

Tasks I did successfully:
- Edited a menu, menu item
- Reset a menu item
- Added a menu item
- Disabled/enable menu item
- Deleted a menu, menu item

Bugs mentioned in initial post that seem fine (I didn't read whole thread):
- Parent dropdown seemed ordered right
- Menu OTF seemed good (I moved it down though)
- Reset expanded menu item and "No" came back fine
- Went to 403 page and didn't see anything wacky

Bugs I still see:
- How do I delete a menu? Too tired to code this tonight...

What is the status now for the bugs?

An observation, why did you choose to link 'menu_custom' and 'menu_links' by menu_name and not mlid?

I like the fieldset below the content fields. IMO that's where it belongs, but I could be convinced otherwise I'm sure and don't think that should hold this up.

Whether you want to create a menu item for the node depends very much on its type, since menus are for a static site structure. Blog entries are almost never put into menus, book pages might be on occasion, pages are very often. From the viewpoint of a user, it makes little sense to make a menu item for a news entry, but a lot of sense to place a static page somewhere in the sitemap.

Therefore: Could the fieldset be on top for 'page' type nodes, and below for everything else? There may be a good case for hard-coding this.

This patch implements the above suggestion on top of patch #58, so skip this one if the suggestion is baloney.

'#weight' => $form['#node']->type == 'page' ? -2 : 5,

-2 was the weight it had prior to patch #58 (top of content), and 5 is the weight in #58.

I've committed this patch so let's deal with the remaining issues in follow-up patches. Thanks guys! :)

More followup issues:
http://drupal.org/node/156782
http://drupal.org/node/156793

Related discussion: http://groups.drupal.org/node/6982#comment-21598

It appears that this workaround of caching the form is breaking CAPTCHA integration. Is there a better solution to this problem than just caching the form?