Posted by pwolanin on July 4, 2007 at 3:41am
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | menu system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | pwolanin |
| Status: | closed (fixed) |
Issue Summary
Follow on to the most recent big menu system patch, http://drupal.org/node/151583
That patch does not include a mechanism for custom menus to be deleted. Being able to delete custom menus (and handling and system module items in those menus) seems like pretty basic functionality.
Comments
#1
Uugh - I thought I had this done, but it turns out that there is no reasonable or simple way to tell block module to delete the block corresponding to the menu. Looks like it needs this issue or something similar: http://drupal.org/node/148531
unless it's ok to directly delete items from the {blocks} table.
Also, on IRC chx says he does not like the idea of being able to delete any menu that is not empty of items (links).
#2
I for one don't want to have to delete every link from a menu just to delete the menu. Only admins should have this power and we should assume if you have edit menus perm or whatever that you should know what's what.
#3
This patch runs the queries to clear out the {blocks} and {block_roles} tables directly.
Also fixes on more spot in menu.module to use the new listing function in menu.inc.
#4
after follow-up with Karoly on IRC - stronger warning messages in the confirm form to let the admin know how many items will be deleted by deleting the menu.
#5
Some entries are being left behind in menu_links.
#6
now with code to delete links to the menu overview page for the menu being deleted.
#7
Also- if you want to test this patch note that it declares a new router path so force a menu rebuild by enabling or disabling a module (such as Ping).
#8
found another bug, of sorts - drupal_access_denied should not be called from within a form builder function, or wackiness results (two pages are rendered on the screen). This patch wraps both menu deletion and menu item deletion confirm form callbacks in a separate function that checks the access before calling drupal_get_form.
Also add message and watchdog notice as with menu link deletion.
#9
capitalization fix from webernet.
#10
Tested, code looks good.
#11
Rerolled after http://drupal.org/node/151055 was committed.
#12
Aren't the user defined menus prefixed with 'menu-', so they are easily distinguished? Or am I mistaken? This patch introduces a different way to distinguish between custom and built-in menus. Some explanation on this would be great.
#13
@Gabor - that is an interesting point - it's true that all the menu-module created menus are now prefixed, but it seemed it would be useful to provide a standard list of system-defined menus that any/all modules could access and rely upon. Note that some of the code being replaced in menu module is a hard-coded list of these menus. In addition, since other modules could create menus using a different prefixing scheme, it seems more robust to me to have a definitive list rather than searching for the prefix.
#14
Actually, it would be most robust to check both that the menu name is not system-defined AND that is is in {menu_custom}, I think.
Note that the callback for the confirm form does (already) check that the menu exists in {menu_custom} since a menu must be loaded or the menu system will return "Not found". So the only place to add this to be extra careful is in the confirm submit function.
#15
Tested again, still works as expected.
#16
Rerolled to fix offset.
#17
Although this is a feature patch, it is so basic functionality, that I agree that it should be in. So reviewed once more and committed.
#18