Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#40 | modules_admin_11.patch | 3.49 KB | pwolanin |
#39 | modules_admin_10.patch | 3.61 KB | pwolanin |
#37 | 136386_7.patch | 4.42 KB | flk |
#34 | 136386_6.patch | 4.74 KB | flk |
#33 | 136386_5.patch | 4.65 KB | flk |
Comments
Comment #1
chx CreditAttribution: chx commentedNope. Use the {menu} table, see other queries to {menu} in system.module as an example.
Comment #2
chx CreditAttribution: chx commentedAnd 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 :)
Comment #3
douggreen CreditAttribution: douggreen commentedAttached 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 :)
Comment #4
douggreen CreditAttribution: douggreen commentedComment #5
chx CreditAttribution: chx commentedDoug, the new menu system never calls hook_menu outside of the builder. Really look at system_main_admin_page , therein you will see this:
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.
Comment #6
douggreen CreditAttribution: douggreen commentedI 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.
Comment #7
douggreen CreditAttribution: douggreen commentedJust 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:
Comment #8
chx CreditAttribution: chx commentedDoug, 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.
Comment #9
douggreen CreditAttribution: douggreen commentedThinking 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".
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.
Comment #10
douggreen CreditAttribution: douggreen commentedI don't see where some of these fields are used anywhere, html for example:
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.
Comment #11
douggreen CreditAttribution: douggreen commentedAttached 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?
Comment #12
douggreen CreditAttribution: douggreen commentedThis patch just keeps getting simpler...
Comment #13
pwolanin CreditAttribution: pwolanin commentedif 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}.
Comment #14
chx CreditAttribution: chx commentedI 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?Comment #15
chx CreditAttribution: chx commentedDoh, 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 :)
Comment #16
douggreen CreditAttribution: douggreen commented@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).
Comment #17
chx CreditAttribution: chx commenteda) factor out b) simplify the code by moving the db_query into a separate helper called by both sys.admin page
Comment #18
douggreen CreditAttribution: douggreen commentedUpon 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
.Comment #19
webernet CreditAttribution: webernet commentedDid 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")
Comment #20
pwolanin CreditAttribution: pwolanin commentedThe 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()?Comment #21
pwolanin CreditAttribution: pwolanin commentedneeds work
Comment #22
flk CreditAttribution: flk commentedrerolled...dont see why running the query through menu_tree_data would be of any use.
Comment #23
flk CreditAttribution: flk commentedComment #24
Steven CreditAttribution: Steven commentedThe 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.
Comment #25
flk CreditAttribution: flk commentedweird, i dont how the hell i rtbc'ed
*grimaces*
Comment #26
flk CreditAttribution: flk commentedsorted links by name, standard links i have placed right at the bottom.
Comment #27
douggreen CreditAttribution: douggreen commentedThe 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.
Comment #28
douggreen CreditAttribution: douggreen commentedbtw, 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.
Comment #29
flk CreditAttribution: flk commentedthats 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.
Comment #30
douggreen CreditAttribution: douggreen commented@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?
Comment #31
pwolanin CreditAttribution: pwolanin commentedI 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.
Comment #32
pwolanin CreditAttribution: pwolanin commentedper 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.
Comment #33
flk CreditAttribution: flk commentedremoved 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.
Comment #34
flk CreditAttribution: flk commentedincluded where clause for module = 'system' to stop the number of false positives being returned.
Comment #35
pwolanin CreditAttribution: pwolanin commentedwe should take this out of the query too "AND ml.depth >= 2"
Comment #36
pwolanin CreditAttribution: pwolanin commentedhmmm, when I try to apply the patch I get:
Comment #37
flk CreditAttribution: flk commentedhmm strange dont know how that happened.
recreated teh patch and removed ml.depth check in the process
Comment #38
pwolanin CreditAttribution: pwolanin commentedHelp module's use of this code seems to have been overlooked:
Comment #39
pwolanin CreditAttribution: pwolanin commentedok, this restores the function per module - seems to be working.
Comment #40
pwolanin CreditAttribution: pwolanin commentedslightly better?
Comment #41
bennybobw CreditAttribution: bennybobw commentedEverything 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.
Comment #42
Dries CreditAttribution: Dries commentedWorks for me. Committed. Thanks.
Comment #43
(not verified) CreditAttribution: commented