Closed (fixed)
Project:
Drupal core
Version:
6.x-dev
Component:
menu system
Priority:
Normal
Category:
Bug report
Assigned:
Reporter:
Created:
4 Jul 2007 at 03:41 UTC
Updated:
8 Aug 2007 at 15:04 UTC
Jump to comment: Most recent file
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.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | patch_155.txt | 9.38 KB | webernet |
| #14 | delete_menu_7.patch | 9.51 KB | pwolanin |
| #11 | patch_150.txt | 9.24 KB | webernet |
| #9 | delete_menu_6.patch | 9.37 KB | pwolanin |
| #8 | delete_menu_5.patch | 9.32 KB | pwolanin |
Comments
Comment #1
pwolanin commentedUugh - 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).
Comment #2
RobRoy commentedI 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.
Comment #3
pwolanin commentedThis 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.
Comment #4
pwolanin commentedafter 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.
Comment #5
webernet commentedSome entries are being left behind in menu_links.
Comment #6
pwolanin commentednow with code to delete links to the menu overview page for the menu being deleted.
Comment #7
pwolanin commentedAlso- 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).
Comment #8
pwolanin commentedfound 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.
Comment #9
pwolanin commentedcapitalization fix from webernet.
Comment #10
webernet commentedTested, code looks good.
Comment #11
webernet commentedRerolled after http://drupal.org/node/151055 was committed.
Comment #12
gábor hojtsyAren'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.
Comment #13
pwolanin commented@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.
Comment #14
pwolanin commentedActually, 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.
Comment #15
webernet commentedTested again, still works as expected.
Comment #16
webernet commentedRerolled to fix offset.
Comment #17
gábor hojtsyAlthough this is a feature patch, it is so basic functionality, that I agree that it should be in. So reviewed once more and committed.
Comment #18
(not verified) commented