re-parenting and caching for menu links
pwolanin - May 18, 2007 - 16:52
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | menu system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | pwolanin |
| Status: | closed |
Description
splitting this off from http://drupal.org/node/144753 to make a patch that just deals with menu.inc and other parts of the menu system that are separate from menu module, but which will be required for menu module to work.
#1
here's the minimal menu.inc part of the starting patch form the other issue.
#2
This patch is totally untested, but the re-parenting code is essentially done I think.
#3
2 thoughts:
1) should function menu_get_item_by_mlid() be renamed menu_link_load()?
2) There should probably be an API function menu_link_delete().
#4
Ok, starting into some good stuff- new API function: menu_tree_all_data()
Also, put back in the normal install the cache_menu table, and start fixing caching code.
#5
fixed some bugs in reparenting code
#6
Why do we need menu_tree_all_data here? I do not think it's needed.
#7
@chx - menu_tree_data() returns a tree based on the current page (maybe we should rename this function). It only returns the visible tree.
In contrast, the function I just wrote, returns the full tree for any menu. It's not impelmented yet, but there will be a flag for skipping the access check- I think this is what is needed for the admin interface for menu module. A user who can administer menus can re-arrange all the links, even if she cannot access the link, right?
Also - this too is not implemented yet - both of these two functions should cache the result before running _menu_link_translate() on each item. I think this is the only sane way to cache, since we can't possibly follow all the changes that might alter access to an item (for example some one using taxonomy_access changes the permissions associated with a term).
#8
menu_tree_all_data IMNSHO belongs to menu module -- I prefer writing a query instead of trying to provide a query for every possible occassion. menu module uses pager_query, anyways.
#9
I think we'll need this query for book module, so at the least I'd like to plan ahead with an appropriate API function rather than having book module manipulate/query the menus directly.
In what context does menu.module use a pager_query?
#10
Per discussion with Karoly on IRC:
1) menu and menu link items are always arrays, never objects
2) a few functions renamed - e.g. menu_get_item_by_mlid() becomes menu_link_load()
3) preparing for caching by moving access checks to the point after the menu tree is build
#11
Testing this code:
-Seems to work to reparent menu links.
-Basic caching of several data structures also implemented.
-Also, restored some functionality for primary and secondary links (of course you'll need the menu module to take easy advantage of this).
#12
re-rolling patch to account for this commit: http://drupal.org/node/140218
#13
previous patch missed some code in sysyem.admin.inc
#14
ok, another informative IRC w/ chx: we need some additional changes to enable menu.module.
in particular, one should always be able to do:
$item = menu_link_load($mlid);// make minimal changes- e.g. new plid
menu_link_save($item);
right now this may not work since href and link_title may be overwritten during _menu_link_translate().
solution- keep the original values. change DB column back to link_path from href. href is generated by _translate() as is title from link_title (possibly via localization code).
#15
Another follow up to chx regarding the addition of caching:
real quick benchmarking with ab -n100, mysql query cache disabled, hitting: http://127.0.0.1/drupal6/admin/settings/logging/dblog
(after allowing anonymous users all sorts of permissions) - this is one of the deeper menu links in the the navigation menu.
Requests per second: 5.97 [#/sec] (mean), or with -c5, Requests per second: 11.66 [#/sec] (mean)
After commenting out the caching code in function menu_tree_page_data()
Requests per second: 5.54 [#/sec] (mean), or with -c5, Requests per second: 10.72 [#/sec] (mean)
So the difference is not huge, but certainly measurable for this page, and probably more significant for a page with several menus, more links, etc.
#16
updated patch- fixes some code style issues. Includes new code for part of the 5.x -> 6.x update (the simple part).
#17
re-rolled to keep up with HEAD.
#18
per conversation with Josh K on IRC, minor additional feature- menu links on the path to root (i.e. in the active trail) also get the class "active-trail" on the
<a>changes relative to the last patch mostly in function theme_menu_item_link, but also minor changes in menu_tree_all_data, and menu_tree_page_data to insure that the current link always gets this class.
(note for code reviewers - a lot of the new functionality in the patch is enabling for menu module and ideally for book module, but has no UI here. You can, for example, run PHP in a node and use menu_link_load() and menu_link_save() to test out the reparenting code. i.e. to move items around in the navigation menu, or to add items to it or to menu_name == 'primary-links').
#19
with patch...
#20
menu_link_save had a broken query:
$parent = db_fetch_array(db_query("SELECT * FROM {menu_links} WHERE menu_name = '%s' AND mlid = %d", $item['plid']));I added the missing $menu_name. menu_flip_item works in menu.module. As I go forward with the module I will post more fixes if they are needed.#21
@chx - thanks
Minor change- this patch moves around the "active-trail" class to put it on the
<li>. Also renames in code the menu link element $item["path_to_root"] ro $item["in_active_trail"] for greater ease of understanding (with an existing DB, run menu_cache_clear_all() or menu_rebuild() to freshen cached data).#22
very minor change- put new parameter to theme function last so it won't break existing custom theme functions.
#23
oops- unlucky patch #13 doesn't include the part of the patch for menu.inc. Use this one instead.
#24
need to deal with menu code in random places, such as here:
http://api.drupal.org/api/HEAD/function/drupal_not_found
http://api.drupal.org/api/HEAD/function/drupal_access_denied
#25
re-rolled for new schema API. Moved table definitions to system.schema and fixed them up a little.
initial part of the 5.x->6.x update seems to work
in some functions renamed $item to $router_item to help make the code more self-explanatory and consistent.
fixed the code in common.inc for drupal_not_found and drupal_access_denied
#26
#27
re-rolled patch to account for http://cvs.drupal.org/viewcvs/drupal/drupal/includes/menu.inc?r1=1.167&r...
#28
investigating this somewhat related bug: http://drupal.org/node/147061
try a path like node/add/xxx
Error messages indicate a bug
#29
The error seems to be occurring, since the router path being returned is 'node/%/%' rather than 'node/add'
'node/%/%' is declared by the comment module, though I'm not sure what it's for:
<?php$items['node/%node/%'] = array(
'title' => 'View',
'page callback' => 'node_page_view',
'page arguments' => array(1, 2),
'access callback' => '_comment_view_access',
'access arguments' => array(1, 2),
'type' => MENU_CALLBACK,
);
?>
The problem seems to be in node_load - it dies if we pass a string in. Maybe related to the bug here: http://drupal.org/node/146667
PHP is willing to try to access a string as an array.
I get the same error if I go to 'node/ddd' -!!!- easiest solution, node_load needs to check is_array() on $param. The same general thing happens if you go to 'user/x'. Again, PHP is willing to take the string as an array.
attached patch changes user_load and node_load. The only other solution I can think of off hand is complicating the menu system by having 2 types of router wildcards - one for numeric values, one for strings.
Attached patch also puts in the menu module schema- per request form Karoly so that this patch doesn't collide with his patch for menu module.
#30
<?php$router_item['page_arguments'] = array_merge(menu_unserialize($router_item['page_arguments'], $map), array_slice($map, $router_item['number_parts']));
?>
this was
array_slice($partswhich limits the number of params needlessly.#31
This is everything but menu.module , that's still a separate issue. I fixed the system menu queries (hidden=0) and added default menus to menu.install -- neither this or the menu schema is strictly necessary for this patch BUT this separation let's Peter work on this, me on menu.module and Josh/Peter on outline. However, I need these small changes here so there is no collision. It causes no harm.
#32
Added better menu_link_delete() function per IRC w/ chx. If an item is deleted, children are re-attached to that item's parent.
Also, found a couple stupid bugs in menu_link_save(): need to use array_shift not array_pop, need to assign mlid to item if existing item.
RTBC? We need this ASAP so dependent patches (menu module, book module) can have a base to work from.
#33
minor fix- 'Home' link in breadcrumb was broken due to href/link_path confusion. Thanks to Gurpartap for pointing this out.
#34
I believe this is now ready to get in.
#35
This is frustrating, every time I roll this patch I remove the temporary block for menu module. Please stop readding it, ppl can't test the menu module patch!
#36
I sneaked in a user module patch inadvertly. Sorry.
#37
after applying chx last patch, I get:
# user warning: Unknown column 'ml.href' in 'where clause' query: SELECT * FROM menu_links ml INNER JOIN menu_router m ON ml.router_path = m.path WHERE ml.href like 'admin/%' AND ml.href != 'admin/help' AND ml.depth = 2 AND ml.menu_name = 'navigation' ORDER BY p1 ASC, p2 ASC, p3 ASC in /home/morphir/www/HEAD/includes/database.mysql.inc on line 163.# notice: Undefined variable: blocks in /home/morphir/www/HEAD/modules/system/system.admin.inc on line 35.
# warning: Invalid argument supplied for foreach() in /home/morphir/www/HEAD/modules/system/system.module on line 2447.
also:
the admin page disappeared (the fieldsets)
#38
morphir, the error is on your end, that line is removed in my patch: - $result = db_query("SELECT * FROM {menu_links} ml INNER JOIN {menu_router} m ON ml.router_path = m.path WHERE ml.href like 'admin/%' AND ml.href != 'admin/help' AND ml.depth = 2 AND ml.menu_name = 'navigation' ORDER BY p1 ASC, p2 ASC, p3 ASC");
#39
sorry, my fault. I got this right now I believe.
I get one error when creating a menu item:
warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, '' was given in /home/morphir/www/HEAD/includes/menu.inc on line 377.#40
yeah but creating a menu item is not this issue...
#41
It all worked for me...
BTW, I was told to go to admin/build/modules, enable menu.module, and stop there
#42
I was told to be more specific... so here goes :)
I visited administer, then site building, then modules.
Page titles were correct every time, so was the breadcrumb.
Nav menu was right too.
This is def RTBC
#43
yes, forget my error there. This one is RTBC.
I actually get no errors anymore.
#44
minor fix for external links
#45
Hmm... I followed the instructions and applied before loading a fresh HEAD. This results in tables {menu_custom}, {menu_links} and {menu_router} but no {menu}. However, there remain many queries to {menu}.
I'm not sure if the issue here is that the {menu} table should be created, or if the remaining references in menu.module need to be updated.
#46
After also applying http://drupal.org/node/144753 and rebuilding database, things seem to be working well.
#47
As that patch does not touch the database... it can't be anything else but not installin' from scratch.
#48
@joshk - don't install the menu module without applying the other patch.
#49
Committed to CVS HEAD. Thanks.
#50
fabulous, thanks!
Of course, now that I'm starting to use this API for book module, I found a minor bug in the code to delete a menu link. See attached patch.
#51
Committed. Thanks.
#52
system_update_6020() needs to be rewritten. Short form: You cannot assume that .schema files contain the right thing during update functions. Long form: http://drupal.org/node/144765#comment-245805.
I'll write a patch, but not at midnight. Since this is now a bugfix, it does not need to get done before 6/1 (right?).
#53
The update is at http://drupal.org/node/147657
#54