The modules admin page is still not coded for the new system.

Attached is my second attempt.

I used devel module to see that the admin/by-task page only generates one query for the access check and then saw where my first attempt generated lots of queries to do the access check. So, this version does only query and saves the results in a static variable. I removed the check for 'admin/help' thinking that it might take mysql longer to exclude this than it would take system.module to process and ignore it -- I'm not sure if that was a wise trade-off given the call to _menu_translate.

I'm still trying to use the mid for the associative array index to solve 126320.

Oh, the pitfalls of vi! That dangling { looked to be on the line above it. I fixed the style error :)

I realize this is one of those cases where it would be easier for you to do it, than for me to figure it out, or for you to explain it to me, but I am digging in and trying to learn the new code. I did look at system_main_admin_page before submitting the last patch which is why I had code that queried {menu}.

However, I don't see how to get the menu items per module without calling hook_menu since the module isn't stored in the menu table. As far as I can see, you either need to add the module to the menu table or call hook_menu in this one instance. As this is only an admin page that doesn't get called very frequently, is there a functional reason to not call hook_menu here? I don't see a performance reason given the nature of this page.

I can add the module to the menu table by modifying menu_rebuild, but before embarking down that road, it would be great to here your response to the above.

Just to expand upon my last comment, attached is a sample patch that adds the module name to the {menu} table. I'm not convinced that it's worth the extra retrieval for every single menu item especially if this is the only use for it.

A couple of other somewhat related issues:

• I noticed that the module column is defined inconsistently in several tables as either 64, 128, or 255 characters. Which is it? This should probably be rolled as a separate patch. Do you know if there has been discussion relating to this? I couldn't find any.
• Also in menu_rebuild, there's a small functional and performance difference between array_merge() and a +=. Is array_merge used intentionally so that a module can override menu settings? If so, this doesn't make sense to me because the module_implements() will return modules in weighted order and this lets higher weighted modules (those that are normally run later in the process and have lower precedence) to overwrite modules with lower weights.
• There doesn't appear to be a function for doing the module_invoke_all and adding the module name as I've done in patched menu_rebuild. This seems like a generic enough thing that this could be put into a a new function in module.inc (but not module_invoke_all because that would slow down other things that don't need it).

Thinking about a smaller {menu} table, rather than a {menu_modules} table what if we had a {menu} and {menu_support} table. {menu} would contain fields needed to display and route everything (a 90% or 99% use-case), and {menu_support} would contain fields needed for other tasks, such as admin stuff, and this would include the module.

is there anything in the current table that you think could be moved to the {menu_support} table?

If you like the idea, I'll work on it. I'm just looking for a "yes this is a good ideawork on this" and "here are some fields you can probably move".

      db_query("CREATE TABLE {menu} (
        mid int NOT NULL default 0,
        pid int NOT NULL default 0,
        path varchar(255) NOT NULL default '',
        load_functions varchar(255) NOT NULL default '',
        to_arg_functions varchar(255) NOT NULL default '',
        access_callback varchar(255) NOT NULL default '',
        access_arguments text,
        page_callback varchar(255) NOT NULL default '',
        page_arguments text,
        fit int NOT NULL default 0,
        number_parts int NOT NULL default 0,
        mleft int NOT NULL default 0,
        mright int NOT NULL default 0,
        visible int NOT NULL default 0,
        parents varchar(255) NOT NULL default '',
        depth int NOT NULL default 0,
        has_children int NOT NULL default 0,
        tab int NOT NULL default 0,
        title varchar(255) NOT NULL default '',
        title_callback varchar(255) NOT NULL default '',
        title_arguments varchar(255) NOT NULL default '',
        parent varchar(255) NOT NULL default '',
        type int NOT NULL default 0,
        block_callback varchar(255) NOT NULL default '',
        description varchar(255) NOT NULL default '',
        position varchar(255) NOT NULL default '',
        link_path varchar(255) NOT NULL default '',
        attributes varchar(255) NOT NULL default '',
        query varchar(255) NOT NULL default '',
        fragment varchar(255) NOT NULL default '',
        absolute INT NOT NULL default 0,
        html INT NOT NULL default 0,
        PRIMARY KEY (path)
      )");

Also related, the table name {menu} isn't really descriptive. This is a temporary table, ... somewhat like a cache, even though it doesn't use the Drupal caching functions. If we named it {menu_cache} it would be clearer that it is a cache table (that can be blown away); also, by not naming it {cache_menu} it would be somewhat noticeable that this isn't a standard Drupal cache.

I don't see where some of these fields are used anywhere, html for example:

  html INT NOT NULL default 0,

It's possible that this is used in a call to l() (not sure here). I don't think that we'll ever be quering on values such as this. Have you played with the performance of serializing data like this such that it only takes up space when they are set? If so, would it make sense to combine several of these infrequently used values into an array that gets serialized? This is another way to make {menu} smaller.

Attached is an updated patch that creates the admin by-module page. Some of my previous changes are superseded by the new {menu_router} table, so this patch is simpler than the previous one. Having not studied the {menu_router} table, was it safe to assume that mlid is unique?

This patch just keeps getting simpler...

@chx, I was thinking that $router_items[] was cached in menu_get_item(), but now that I look closer, I see that only the already returned $path values are cached. If there was a significant different in code size, I'd probably opt for simpler/smaller code on this admin page. It's not done that often, and we need to carry this code for almost every page load (unless this gets factored out with the include file patch).

Upon further review, I like your patch more. It's faster and cleaner. I ran a comparison test on the two possible patches and can confirm that your patch has fewer queries. I did find one problem, which is that the results were not as deep as they should be. I think that the query should use depth >= 2.

The previous patch had two problems. 1) it sorted by key instead of title, 2) it put the configure at the end where-as in 5.x the first item in the list is always configure and the last item is always help. The attached patch solves both these problems. If we'd prefer to put them both first or last, that is a simple change ($admin_tasks[] instead of array_unshift() or vice-versa), but I choose to mimic the current behavior.

btw, the key it was sorting on was a simple numeric key based on the order it was added, which as far as I can tell, had the same problem as not sorting with the expense of a sort.

@Shakur, Are you saying that if you go to admin/build/menu and move, for example, "Site Configuration" (and everything below it) to the "Primary menu" that these admin items are no longer displayed on the admin page? If so, Drupal 5.x has the exact same problem, and it exists for by the by-task and by-module lists. If that's it, then please create a separate issue for it? Again, if that's the problem, would you agree to switch back to code-needs-review?