Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Title: update for D6 ? » Update Admin Menu for 6.x
Version: 7.x-3.x-dev » 5.x-1.x-dev

I had a quick look at the new menu system. First of all - bad news: The global variable $_menu does not exist anymore. It seems that there's no function yet designed to retrieve (and alter) the complete, expanded menu tree.

This is shocking… Because until now the main advantage of admin_menu was that it doesn't alter the menu in any way. We might need to completely rewrite the module to extract administrative menu items to a new menu at installation time.

I'll greatly appreciate any input and/or patches from anyone having deeper insights into the new menu system.

sun’s picture

Status: Active » Postponed

After talking to chx on IRC the new menu system will have a hook_menu_alter() that will allow the operations needed for admin_menu. However, the hook still is under heavy development, so we'll have to wait until there is a solid ground on which we can base this update.

yched’s picture

OK, thanks for following this :-)

sun’s picture

Title: Update Admin Menu for 6.x » Port Admin Menu to 6.x
Priority: Normal » Critical
Status: Postponed » Active

Since Drupal 6 beta is about to be released, Admin Menu should be available ASAP.

Currently, I'm very busy with several projects, so if anyone has successfully or unsuccessfully attempted to port Admin Menu for 6.x I would be glad to test, review, fix or commit any patch. :)

sun’s picture

Version: 5.x-1.x-dev » 5.x-2.x-dev
Status: Active » Needs work
FileSize
20.81 KB

First try. Includes plenty of debug code.

Critical issue: menu_links does not contain any local tasks. In this patch, I've tried to retrieve them manually through a crude database query. Even if this could work out somehow, we'd still have an unresolved issue regarding adding and altering menu items.

A completely different approach could be:

  1. Invoke menu_router_build() to get a "copy" of the current menu structure (cached).
  2. Perform alterations on this structure.
  3. Implement a fork of _menu_navigation_links_rebuild().
  4. Implement a magic function which turns this structure into a menu tree, like menu_tree_output() expects it.
alliax’s picture

subscribing and so sad to have started a multi install in d6 without checking on this module.. so sad..

WorldFallz’s picture

I just loaded up a brand spanking new multi-site dev environment based on D6. I'm no coder, but I 'm a big fan of this module and would love to contribute testing. Should I start with the patch posted here, which is probabably out-of-date already, or will a 6.x-dev be available sometime soon? FYI I"m windows based and have access to FF2, IE6, and IE7 for testing purposes.

sun’s picture

@WorldFallz: Patch in #5 should still apply, but the outlined problems remain.

keesje’s picture

subscribing

koorneef’s picture

subscribing

sun’s picture

FileSize
19.73 KB

New patch with improvements by smk-ka. Displays some local tasks (not all).

alliax’s picture

sorry to ask here, but i'm interested by the patch and this time there's just too many things to modify:
how can I apply a patch file to a module file? which software do you use to automatically remove/add the lines in the patch?

Cheers,
James

sun’s picture

ekendra’s picture

subscribing. cheers!

manerhabe’s picture

Is it supposed to spit the menu out at the bottom in a unordered list format (does so with multiple core themes)? Just wondering, because my patch might of applied incorrectly. I didn't get any errors, but I had to get some old versions of files (admin_menu.info, .module, .inc) from CVS because the patch was out of date since it was created for an old dev version no longer available on the releases page.

derjochenmeyer’s picture

subscribing.

derjochenmeyer’s picture

i would be happy to test :) patching fails with 5.x-dev and 2.4 :(

eigentor’s picture

I also tried tthe patch on 2.4: 3 out of seven hunks failed :( Daniel, gib dir einen Ruck, wir wollen doch auch in Drupal 6 aminmenuisieren - sorry for the german interlude

chx’s picture

I would think you can do

$item = db_fetch_array(db_query("SELECT mlid FROM {menu_links} WHERE link_path = 'admin' AND menu_name = 'navigation'"));
menu_tree_all_data('navigation', $item);
John Bickar’s picture

Yay! Admin Menu rules!

Subscribing. Patch in 11 failed for me on 5.x-2.x-dev

patch < admin_menu_7.patch 
patching file admin_menu.inc
Hunk #1 FAILED at 19.
Hunk #2 succeeded at 171 with fuzz 1 (offset 3 lines).
Hunk #3 succeeded at 188 (offset 3 lines).
Hunk #4 succeeded at 208 (offset 3 lines).
Hunk #5 succeeded at 216 with fuzz 1 (offset 3 lines).
Hunk #6 FAILED at 227.
Hunk #7 succeeded at 270 with fuzz 2 (offset 18 lines).
2 out of 7 hunks FAILED -- saving rejects to file admin_menu.inc.rej
patching file admin_menu.info
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file admin_menu.info.rej
patching file admin_menu.module
Hunk #1 succeeded at 14 with fuzz 1.
Hunk #2 FAILED at 29.
Hunk #3 FAILED at 71.
Hunk #4 FAILED at 97.
Hunk #5 succeeded at 345 with fuzz 1 (offset 36 lines).
Hunk #6 succeeded at 361 (offset 36 lines).
Hunk #7 succeeded at 410 (offset 34 lines).
3 out of 7 hunks FAILED -- saving rejects to file admin_menu.module.rej
sun’s picture

FileSize
21.88 KB

Re-rolled patch against latest HEAD.

@chx: Thanks for your input! I've added your proposed code to admin_menu_tree_all_data(), commented-out for now, because:

$item_admin = db_fetch_array(db_query("SELECT * FROM {menu_links} WHERE router_path = 'admin'"));
menu_tree_all_data('navigation', $item_admin);

returns all top level items of the 'navigation' menu instead of all items below the 'admin[ister]' menu item. However, we could work around this later.

The real question is: I do not understand, why the menu system only returns the top level items below 'admin' (i.e. "Content management", "Site building", aso.), but not all menu items below 'admin'.
If only 'mlid' is selected, it doesn't return any items below 'admin' at all.

John Bickar’s picture

Patch fails for me on admin_menu.info

patch < admin_menu_8.patch 
patching file admin_menu.inc
patching file admin_menu.info
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file admin_menu.info.rej
patching file admin_menu.module

But I got it working by manually applying the changes (woo hoo!). Some notes:

  • 'Tools' links to admin/build/views (not what I expected?)
  • 'Settings' links to admin/build/menu (again, not what I expected? just wondering if this functionality is by design)
  • Hovering over a menu item now shows the url in the status bar in Firefox 2 (sweet!)

Thanks for working on this! I'm just working on porting my site over to D6 and this will make it much easier!

steve.colson’s picture

Subscribing. Any issues with the latest patch?

wwwoliondorcom’s picture

hello,

As many others here I really miss the admin menu on drupal 6, so should we ALL send few dollars with Paypal to expect getting it faster?

As this is the only thread about admin menu that I read I don't know so much about the future development of this menu for Drupal 6, so, sorry if I wrote something that had been discussed before.

Thanks anyway for this great module!

sun’s picture

Assigned: Unassigned » sun

wwwoliondorcom, yes, that is probably the best idea to push this port. I've created a ChipIn for Drupal Administration Menu Port for D6, so everyone interested in this port can throw a few bucks in. I did not add a limit, however, due to the already identified issues regarding the new menu system this port will require a lot of hard work, maybe even a complete rewrite of the module.

dennys’s picture

I use this patch for 2.4 but got the error message:

patching file admin_menu.inc
patching file admin_menu.info
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file admin_menu.info.rej
patching file admin_menu.module

And I also use this patch for 2008-Mar-20 version but still got error.

patching file admin_menu.inc
patching file admin_menu.info
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file admin_menu.info.rej
patching file admin_menu.module
Hunk #2 FAILED at 48.
1 out of 7 hunks FAILED -- saving rejects to file admin_menu.module.rej
sun’s picture

As written above, patch in #21 is against latest code in HEAD (in CVS).

dennys’s picture

Sorry, I just download the CVS but still get error.

patching file admin_menu.inc
patching file admin_menu.info
patching file admin_menu.module
Hunk #2 FAILED at 48.
1 out of 7 hunks FAILED -- saving rejects to file admin_menu.module.rej
sun’s picture

FileSize
21.74 KB

Re-rolled patch against latest HEAD. Applies on a fresh CVS checkout only.

mrtoner’s picture

Applied patch in#29 to the latest CVS versions (cherry-picked from the repository since I don't know what I'm doing in there). No errors. The only issue I'm seeing at this point is that the favicon, logged users, and logout items are not displaying. Otherwise, the admin menus are working fine.

dennys’s picture

I applied patch #29 too, I think it's ok, thanks. How about release a snapshot for D6 and more people can help to test.

sun’s picture

Sorry guys, it seems you got it wrong - please read comments #1 and #25 again. This port is far from complete, and if above patch partially works for you, that is caused by coincidence. In case you intend to use this on a production site, I do not recommend this, because there will be no support if something goes wrong.

mrtoner’s picture

Daniel, my comment wasn't meant to indicate that the patched module works perfectly; it was meant to offset the prior 7 comments indicating problems with the patch. You'll be happy to know that beyond the issues I mentioned, some of the menus do not have all the submenus I expect. :-)

Still, it's worth using on my 6.x sites because I'm still more productive.

eigentor’s picture

Maybe we should chip in some money http://unleashedmind.chipin.com/drupal-administration-menu-port-for-d6 to give Daniel a strong incentive to investigate the depth of the new menu system ;) As seen from his comments, patches simply won't do. I fully understand that a maybe complete rewrite exceeds what he can do in his spare time.

robertgarrigos’s picture

Definitely! I've added my few hundred cents to ti ;-)

Pancho’s picture

Category: feature » task
FileSize
21.17 KB

Just a reroll of patch #29 against HEAD (besides some fuzz, one hunk failed).
The port is still somewhat feature incomplete, but the most important things seem to work well enough for testing and maybe non-production usage.

jamm’s picture

subscribing

ardee-1’s picture

Subscribing.

rapsli’s picture

patch works for me... only basic functions, but it makes working so much nicer. Maybe we can release a first alpha version for D6 as I believe there will be lots of testers and a lot of users will appreciate it (even though some of the nice functionality is not in yet)

WorldFallz’s picture

+1 to the patch in #36. Without the bells and whistles of the added local tasks, but still far better than any other admin option out there.

IceCreamYou’s picture

tracking

alliax’s picture

yes, if someone could post somewhere on the net the latest dev module file with the patch applied on it, it will be so fine. Anything like admin menu on D6 will be better than nothing or than this other module, I forgot the name but it was not good.

sun’s picture

FileSize
21.48 KB

Last patch by Pancho (#36) was incomplete.
Re-rolled patch in #29 against latest HEAD. Applies on a fresh CVS checkout only.

I'll clean-up the existing code now and create a DRUPAL-6--1 branch, along with a development snapshot for 6.x. However, comment #32 still applies.

sun’s picture

http://drupal.org/node/251566 should be available within the next 12 hours.

eigentor’s picture

drupal.org/node/251566: access is denied to that node.

Pancho’s picture

> Last patch by Pancho (#36) was incomplete.

Not a big thing, but I can't see why my patch should have been incomplete. It's been just slightly smaller, because Eclipse doesn't support the -p flag in diff.

sun’s picture

@Pancho: Sorry, I was wrong. My diff tool was confused because the order of files in your patch was different.

alliax’s picture

Thank you very much for putting something for D6 even if not as useful as for D5, it's a timesaver anyway! Thanks.

joris.verschueren’s picture

great that a D6 version is available! even if it hasn't got all functionalities, I feel much more at home on my website again!

Joris

sun’s picture

Version: 5.x-2.x-dev » 6.x-1.x-dev
FileSize
7.39 KB

smk-ka and I spent some hours on this port this evening. Attached patch re-introduces base-functionality of Drupal Administration Menu we already had in Drupal 4.7.x. This means all local tasks in the administrative menu are back again. Additional improvements like direct access to create content items and all other add-ons are still missing.

Beware that your database user needs to have sufficient permissions to create temporary tables, and, this patch requires the MyISAM table engine currently.
This will hopefully change in an upcoming code-sprint. Currently, we are investigating the idea to store a mix of menu_router, menu_links, and hook_admin_menu() items in a new admin_menu table to eliminate the need for a temporary table.

Additionally, caching has been re-enabled. This will save you approx. 100ms on each page request.

While I really would like to get some feedback on this patch, I recommend to not use it unless you know how to revert your module to the last official release.

sun’s picture

Since this port still needs much work and the previous ChipIn somehow ended too early:
http://unleashedmind.chipin.com/drupal-administration-menu-port-for-d6-2

Thanks to all who have contributed until now!

snufkin’s picture

subscribe

sun’s picture

FileSize
11.95 KB

This patch is just for reference. Uses a permanent table holding a mixture of items from menu_router and menu_links. Still no hook_admin_menu items.
We will investigate a completely different way now.

DO NOT USE, unless you know how to manually remove a module.

dergachev’s picture

subscribe

Gábor Hojtsy’s picture

FileSize
19.67 KB

I have actually been playing with this latest patch, and it seems to work quite right now (although admittedly not too nice a code, it resembles the menu API internals in complication :). Anyway, I was working on making this patch easier to use by applying schema API goodness (install and uninstall) as well as fixing the Cache API todo which was included.

Not sure what the completely different way would be, but knowing more about it could help us collaborate on bringing a full feature set for Drupal 6.

pwolanin’s picture

FileSize
3.6 KB
110 bytes

ok, I was discussing this with Gabor this morning, and it seems to me that you are all too stuck in the Drupal 5 way of doing things.

Attached is a demo module I wrote in the last couple hours that I think shows the essential features you need. You need to make a new links menu, and store your stuff there. For the moment at least, throw out all your caching stuff and just let the menu system do its thing.

Obviously, this needs fine tuning, and you may need to code in the exclusion or renaming of some links, as well as handling the wildcards. Note that you get all the local tasks (excluding the ones with wildcards at the moment).

Enable the block to see the menu.

pwolanin’s picture

this still has some cruft that i'm not done fixing, but it works well enough to show the principle.

pwolanin’s picture

A little more progress- but for some reason the .info files are not found - I really don't understand why.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
36.48 KB

for some reason, the info files cannot be parsed during the shutdown phase. New patch deals with this problem.

a couple TODOs left, but it's pretty well along.

pwolanin’s picture

Assigned: sun » pwolanin
FileSize
36.52 KB

Dynamic user link and devel links seem to be working.

pwolanin’s picture

It could be a security hole to turn HTML on for all links. Let's just do it for those that need it.

pwolanin’s picture

FileSize
2.22 KB

here is a simpletest - needs to be committed in a tests subdirectory

Gábor Hojtsy’s picture

Tested functionality of patch in #60:

- menu did not show up at all first time I enabled the module (worked after reload)
- user count placed in "favicon menu" not at the place it used to be in D5
- altered menus are missing (content management submit content links, drupal.org issue links)

Gábor Hojtsy’s picture

Also looked at the code, and it looks pretty nice. There are some functions without phpdoc I am wondering whats their role exactly (like admin_menu_link_build()). Some more docs there will probably help.

Noticed, that calling of the admin_menu hook looks broken. That is supposed to let any more to add items however they want. It is called with the $info_files array, which is probably unintended. Then returned links are not saved with the same API as used above. Not sure what you envisioned here.

pwolanin’s picture

ok, here's a new patch - move the rebuilding to hook_footer, which should avoid a delay where new links weren't showing until the next page view.

pwolanin’s picture

Add links to create each content type.

pwolanin’s picture

FileSize
2.67 KB

slightly improved test with added code comments.

pwolanin’s picture

FileSize
2.64 KB

reworked test to use D7 API, per http://drupal.org/node/241156 which will simplify future updating

eigentor’s picture

Heyho, Dev Snapshot works for me. Great. As Admin Menu cannot be beaten as the top downloaded Module on drupalmodules.com, this will be good News to many.

Gábor Hojtsy’s picture

Eigentor: the dev snapshot is a quick hack as acknowledged by sun above. The patch improves on it considerably, bringing back quite a few of the lost features you got used to in D5. So it would be great, if you could test this patch.

sun’s picture

Whoa! This looks and works really nice already! Thanks, Peter! Thanks, acquia!
To make further development easier, I've committed the patch in #66 with (absolutely) minor changes to HEAD and DRUPAL-6--1 branches. Also committed test file in #68.

I think this port revealed two bugs in Drupal core:

  1. node.module: Registers menu items admin/content/types (overview of all content-types) and admin/content/node-type/type-name and corresponding links below (management of one content-type). Because of that, I see "Manage fields", "Display fields", and "Add field" menu items directly below the "Content management" menu, whereas those clearly should be below admin/content/types/type-name. While it is possible that it is CCK that registers those items, this is also a problem for admin_menu itself, because we introduced direct access links to "Edit <content-type>" below Content management » Content types in the latest 5.x-2.x version.
  2. menu.inc: I'm getting 7 notices on each page request:
    notice: Undefined index: attributes in H:\unleashedmind\test\drupal6\includes\menu.inc on line 517.
    

    due to a missing check in menu.inc:

        if ($link_translate && $item['options']['attributes']['title'] == $original_description) {
          $item['localized_options']['attributes']['title'] = $item['description'];
        }
    
pwolanin’s picture

@sun - that path naming in D6 is not a bug - it was a choice to avoid namespace problems. We did the same with some of the menu stuff. However, you are right that it makes some of this sort of thing a little more difficult.

The problem with menu.inc will be fixed by this patch that is already RTBC for D6: http://drupal.org/node/259483

pwolanin’s picture

@sun - you'll notice that the current code just strips out all those edit links that were in the wrong place. Probably the simplest thing to do is just add them back later in the right place. Maybe it would be easier to do if we kept track of them and then passed them along?

sun’s picture

Ugh. That sounds awkward. While we could alter those paths for admin menu to get all items displayed in the proper menus, I think that such kind of alterations can break pretty much, f.e. if another contrib module depends on arg(2) == 'node-type' && arg(3) to add JavaScript or whatever else to the page.

Or, could we use that 'parent_path' property for links that you've already implemented in admin_menu_admin_menu() to alter their position in the menu without changing their paths?

pwolanin’s picture

FileSize
3.32 KB

@sun - here's a patch that just removes them for now. As far as I can tell, they don't appear in the D5 version of this module when CCK is enabled?

pwolanin’s picture

Right - you (or another module even) could add them back, as I did in this last path for 'admin/by-module', as one option.

sun’s picture

The current development snapshot for D5 contains a bunch of improvements -- also those Edit content-type links I was referring to.

AFAIK, no other contrib module implements hook_admin_menu() yet. However, since CCK/Views/Panels belong to the de-facto standard modules, admin_menu provides enhanced support for them.

Attached patch tries to re-add content-type items below 'Content types'. Works for "Edit" items, but not for items of CCK.

pwolanin’s picture

FileSize
4.42 KB

@sun - you are doing something funny with the paths - you cannot make arbitrary changes to the paths, since corresponding router paths do not exist.

How about this - just construct them and put them under admin/content/types/list

sun’s picture

FileSize
7.74 KB

Hm... this does not respect access permissions for the current user. Both node/add/* and admin/content/node-type/* links are displayed for content-types the user is not allowed to create/edit.

Attached patch tries to circumvent this for edit content-type items by checking for a deleted item. However, adding them below "List" is unnecessary deep. The menu should really look like:

Content management
Content management » Content types
Content management » Content types » ...
Content management » Content types » Edit <content-type>
Content management » Content types » Edit <content-type> » Manage fields

I've basically figured out how to accomplish this structure now. But some issues remain:

- node/add* as well as admin/content/node-type/* links are not checked for sufficient access permissions.
- Content type names in title arguments for Edit <content-type> items are not injected.

I still didn't understand yet at which time title callbacks and access callbacks for links are actually executed.

pwolanin’s picture

@sun the access checking seems ok for me for node/add/* - are you sure you are not looking with an account with administer nodes permission?

pwolanin’s picture

Status: Needs review » Needs work

@sun, this looks wrong in your patch:

+  if (isset($deleted['node/add'])) {

Only links under admin/* might ever appear in $deleted.

And you cannot define a title callback for links - title callbacks are a bit funky. Basically they are meant for setting the page title for callbacks and such.

sun’s picture

Hrm... I also added

-    if (($item['type'] != MENU_CALLBACK) && ($item['_parts'][0] == 'admin') && (count($item['_parts']) > 1)) {
+    // Exclude menu callbacks, include items below admin/* and node/add/*.
+    if ($item['type'] != MENU_CALLBACK && (($item['_parts'][0] == 'admin' && count($item['_parts']) > 1) || (isset($item['_parts'][1]) && $item['_parts'][0] == 'node' && $item['_parts'][1] == 'add'))) {
pwolanin’s picture

FileSize
5.88 KB

ok, this patch gets the logout link and type names in place. CCK yet to come.

pwolanin’s picture

FileSize
6.73 KB

ok, I think this handles the CCK-type links in a pretty general way that should also extend to other modules that add paths under admin/content/node-types

I understand now why you were deleting and re-adding the node/add paths - I have not put that logic back at this point, but it's fine if you think it's needed.

sun’s picture

Status: Needs work » Needs review
FileSize
2.79 KB

#84 has been committed.

Attached patch implements menu links below node/add* dynamically, so we finally support any menu items below node/add. This is needed for modules like Advertisement, eCommerce, Ubercart, aso. that register items like node/add/ad/image.

As outlined by pwolanin in IRC, the last remaining issue is to fork http://api.drupal.org/api/function/_menu_navigation_links_rebuild/6 into admin_menu_link_save() to prevent neverending increment of mlids in {menu_links}, each time the cache is cleared or a module is (un)installed.

sun’s picture

FileSize
1.35 KB

#85 has been committed.

@pwolanin: Is it possible that it's that simple?

Based on a quick test, there were no new mlids created after clearing the cache. After enabling a module, only a small amount of new mlids have been created. After disabling and re-enabling some modules, the amount of new mlids increased, but still wasn't as much as during each clear-cache previously (approx. 200).

I know that it's missing the part that checks for stale menu links, which no longer have a corresponding menu_router entry. However, as modules can be enabled/disabled/installed/uninstalled at any time, that might not be needed at all.

Query used to check for new mlids:

SELECT max(mlid) FROM menu_links WHERE module = 'admin_menu';
pwolanin’s picture

Status: Needs review » Needs work

@sun - yes, it should be about that simple - the main thing is to get the existing mlid and plid (and has_children)

As alternative would be do iterate over the existing item and assign to the new item any keys that don't exist.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
4.07 KB

@sun - I re-ordered some of the operations a little so that things should work if the parent_path is changed, plus added the possibility of '' for 'parent_item' so you can specify a top-level item.

sun’s picture

Status: Needs review » Fixed

Works! Committed!

Peter, thank you very much for this awesome job! I think this port will have a noteworthy impact on the amount of users who think about upgrading to Drupal 6.x. (btw: to everyone who cares I recommend checking out http://drupal.org/project/upgrade_status)

I just found out that the new menu system in D6 finally takes care of the actual access permissions for items below Site configuration, which is (for the n'th time) outlined in #167020: Inaccessible site configuration menu items are displayed.

For a stable 6.x-1.0 release there are still some minor todos, primarily regarding re-implementing the improved features of admin_menu 5.x-2.5, and cleaning up the code and documentation. However, I'd like to move this to a new issue (#268373: Re-implement improvements from 5.x-2.5), since the preliminary goal of rewriting admin_menu for D6 has been accomplished now.

alliax’s picture

Thank you very much pwolanin and sun, I knew it was possible to do it, but we have to wait because too many module developpers still have most of their sites in drupal 5. Next time when drupal 7 will be out, I will be extra cautious and will not do the same mistake I made with drupal 6, thinking that I could start sites with D6 and that useful/essential modules will be updated in the following weeks, that's just not true, we're talking months!

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.