menu_link_maintain doesn't clear cache properly
j0hn-smith - July 4, 2008 - 08:37
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | menu system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
When using menu_link_maintain to edit a link ($op = 'update'), the menu cache is not cleared unless the menu item you're editing is the the 'navigation' menu. The reason is that menu_cache_clear isn't passed the correct menu_name as a parameter so uses 'navigation' by default.
A quick solution would be to patch menu_link_maintain to call menu_cache_clear_all instead.
Current code (menu.inc line 2040)
function menu_link_maintain($module, $op, $link_path, $link_title) {
switch ($op) {
...............
case 'update':
db_query("UPDATE {menu_links} SET link_title = '%s' WHERE link_path = '%s' AND customized = 0 AND module = '%s'", $link_title, $link_path, $module);
menu_cache_clear(); // clears cache for navigation menu when no parameter is passed in
break;
...............
}
}A solution (maybe not a good one)
function menu_link_maintain($module, $op, $link_path, $link_title) {
switch ($op) {
...............
case 'update':
db_query("UPDATE {menu_links} SET link_title = '%s' WHERE link_path = '%s' AND customized = 0 AND module = '%s'", $link_title, $link_path, $module);
menu_cache_clear_all(); // clears cache for all menus
break;
...............
}
}
#1
Yeah, I think I noticed this at one point recently but didn't open an issue.
This code is problematic since we don't know which menus we are saving to. The solution you suggest would work, but we would invalidate less of the cache maybe if we did something like:
case 'update':db_query("UPDATE {menu_links} SET link_title = '%s' WHERE link_path = '%s' AND customized = 0 AND module = '%s'", $link_title, $link_path, $module);
$result = db_query("SELECT menu_name FROM {menu_links} WHERE link_path = '%s' AND customized = 0 AND module = '%s'", $link_title, $link_path, $module);
while ($item = db_fetch_array($result)) {
menu_cache_clear($item['menu_name']);
}
break;
#2
so, chx agrees in IRC that this is the sensible approach
#3
A much better solution.
#4
just rolled the above into a patch. not really tested yet.
#5
Patch looks completely fine to me. I ran all tests, no regressions, and manually edited a menu item. Didn't yet look inside the cache_menu table to ensure it's actually deleting the correct records though.
#6
Spoke to pwolanin in irc, doesn't seem to be a way to reproduce this in core. Since it's pretty minor, going to call it RTBC.
#7
Nope, it doesn't work. Regression in aggregator module when changing a category name - menu items should change with it, but they don't with the patch applied.
#8
ok, that was a stupid bug - extra argument to db_query() was causing the SELECT query to fail.
Tested and seems to work as expected.
#9
This one works fine :)
#10
Committed to CVS HEAD. Moving to Drupal 6.
#11
Committed to 6.x. Thanks!
#12
Automatically closed -- issue fixed for two weeks with no activity.