Attached is a patch to implement the admin by-modules page. Hopefully the simplicity of this patch is indicitive of the ease of the new menu system and not because I missed anything important. I'm not sure if the % arguments in the menu definition indexes need to be taken into account here, or if they'll be automatically handled by the menu_get_item lookup. If I did miss something important, just point me in the right direction, and I'll try to fix it.

The patch also changes the array index to solve 126320.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Status: Needs review » Needs work

if ($items = module_invoke($module, 'menu', TRUE)) {

Nope. Use the {menu} table, see other queries to {menu} in system.module as an example.

chx’s picture

And also, you have a code style error with a { in a new line. You were about the last from whom I would have expected a code style error :)

douggreen’s picture

FileSize
3.34 KB

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 :)

douggreen’s picture

Status: Needs work » Needs review
chx’s picture

Status: Needs review » Needs work

Doug, the new menu system never calls hook_menu outside of the builder. Really look at system_main_admin_page , therein you will see this:

  $result = db_query("SELECT * FROM {menu} WHERE path LIKE 'admin/%%' AND depth = 2 AND visible
= 1 AND path != 'admin/help' ORDER BY mleft");
  while ($item = db_fetch_object($result)) {
    _menu_translate($item, $map, MENU_RENDER_LINK);
    if (!$item->access) {
      continue;
    }

This is the archetypical 'get a bunch of menu items' code. There is no hook_menu invocation in sight, the menu table is used to grab the items not just to access check.

douggreen’s picture

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.

douggreen’s picture

FileSize
7.11 KB

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).
chx’s picture

Doug, what about storing it in a menu_modules table, then? I am also worried about menu being too wide, I will think a bit on it, maybe I will some serialized fields together but that's another issue. The patch begins to look menu-ish :) Konstantin said that arg() is the same as arg(NULL) , I will remove those frm elsewhere, but you do not need it. I think it was Gordon who pointed out much earlier that $modules[$item->module]->menu[] = $item; creates no notices (I thought so too it does) so no need for initializing the array. You forgot to pass in (array)$item to l as third argument. Flipping is very good.

douggreen’s picture

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.

douggreen’s picture

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.

douggreen’s picture

Status: Needs work » Needs review
FileSize
4.08 KB

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?

douggreen’s picture

FileSize
3.12 KB

This patch just keeps getting simpler...

pwolanin’s picture

if no one answered before- mlid is unique (the primary key)

Also, I'd swear there was another patch that adds a 'module' field to {menu_router}.

chx’s picture

FileSize
4.03 KB

I do not like your approach. menu_get_item runs a relatively expensive query which is usually not a concern because it is supposed to run once on a page. Here, it is ran a few ten times, which I do not particurarly like even for an admin page. Why not copy the query from the main admin page and sort it out later?

chx’s picture

Doh, the ton of that followup is not nice -- so Doug, thanks for keeping up with this issue but I think I know a better solution :)

douggreen’s picture

@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).

chx’s picture

a) factor out b) simplify the code by moving the db_query into a separate helper called by both sys.admin page

douggreen’s picture

FileSize
4.04 KB

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.

webernet’s picture

Did some testing:

- Modules appear to be sorted by module name, rather than the (translatable?) name from the info file (ie. the "Content translation" section from "Translation.module" appears with the "T" modules)

-The individual links for each module don't appear to sorted (ie. under "User" module, "Access control" comes below "User settings")

pwolanin’s picture

The query in the patch doesn't quite make sense.

query should probably include hidden >= 0 AND module = 'system'

I'm not sure why you're doing the ORDER BY p1 ASC, p2 ASC, p3 ASC Since you're saying items can be at any depth, you should do it out to p5 (i.e. MENU_MAX_DEPTH - 1). In that case, why not use menu_tree_data()?

pwolanin’s picture

Status: Needs review » Needs work

needs work

flk’s picture

FileSize
4.3 KB

rerolled...dont see why running the query through menu_tree_data would be of any use.

flk’s picture

Status: Needs work » Reviewed & tested by the community
Steven’s picture

Status: Reviewed & tested by the community » Needs work

The Drupal 5 version of this page sorted the module's links alphabetically, while the standard 'configure permissions' and 'get help' links appeared first and last, respectively. This seemed to make sense, though perhaps the standard links should both go either first or last.

In any case, because the links are localized after retrieval, we'll need to sort them in PHP. The current ordering is completely random.

webernet: the problem with module sorting by filename rather than human-readable name is a problem on the admin/build/modules page too. It's not really part of this patch, and I'd say open a separate issue that covers both pages.

flk’s picture

weird, i dont how the hell i rtbc'ed
*grimaces*

flk’s picture

FileSize
4.35 KB

sorted links by name, standard links i have placed right at the bottom.

douggreen’s picture

Status: Needs work » Needs review
FileSize
4.8 KB

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.

douggreen’s picture

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.

flk’s picture

Status: Needs review » Needs work

thats weird i could've sworn i was using sort, oh well.
i still think having the standard links (configure, help) at the bottom might be an good idea but thats neither here or there.
This patch still needs work:
What happens if the administer item in the navigation menu is moved to a new another menu whether it be primary/secondary or user defined one?....atm if you move the administer item to a different menu tree that page is left totally empty.

douggreen’s picture

@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?

pwolanin’s picture

I think that the by-module page appear the same regardless of whether the links are moved. However, this seems to be an area of disagreement.

pwolanin’s picture

per discussion with chx - let's try to do this page so it's constant even if the links are moved. It seems to make sense to Shakur and I that a module's listed functions shouldn't be changed by moving inks, even if by doing so you simplify the site's "dashboard" as represented by the /admin by-task listing.

flk’s picture

FileSize
4.65 KB

removed the check menu in the navigation....now it finds all occurence's of 'admin/%' in any of the menus.
It would've been nice to set the by-module as the default page if the administer item has been moved from the navigation menu but that sadly doesnt seem feasible.

flk’s picture

FileSize
4.74 KB

included where clause for module = 'system' to stop the number of false positives being returned.

pwolanin’s picture

we should take this out of the query too "AND ml.depth >= 2"

pwolanin’s picture

hmmm, when I try to apply the patch I get:

(Stripping trailing CRs from patch.)
patching file modules/system/system.module
patch: **** malformed patch at line 33: @@ -2689,57 +2703,44 @@ function system_admin_by_module() {

flk’s picture

FileSize
4.42 KB

hmm strange dont know how that happened.
recreated teh patch and removed ml.depth check in the process

pwolanin’s picture

Help module's use of this code seems to have been overlooked:

Fatal error: Call to undefined function: system_get_module_admin_tasks() in /modules/help/help.module on line 126
pwolanin’s picture

Status: Needs work » Needs review
FileSize
3.61 KB

ok, this restores the function per module - seems to be working.

pwolanin’s picture

FileSize
3.49 KB

slightly better?

bennybobw’s picture

Status: Needs review » Reviewed & tested by the community

Everything looks good here. I checked the code, went to the page, and clicked through all the links, and the links on the help page. Setting to RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Works for me. Committed. Thanks.

Anonymous’s picture

Status: Fixed » Closed (fixed)