Allow custom menus to be deleted

pwolanin - July 4, 2007 - 03:41
Project:Drupal
Version:6.x-dev
Component:menu system
Category:bug report
Priority:normal
Assigned:pwolanin
Status:closed
Description

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.

#1

pwolanin - July 5, 2007 - 18:11
Status:active» needs work

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).

AttachmentSizeStatusTest resultOperations
delete_menu_1.patch5.97 KBIgnoredNoneNone

#2

RobRoy - July 6, 2007 - 02:05

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

pwolanin - July 7, 2007 - 00:27
Status:needs work» needs review

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.

AttachmentSizeStatusTest resultOperations
delete_menu_2.patch6.85 KBIgnoredNoneNone

#4

pwolanin - July 7, 2007 - 00:53

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.

AttachmentSizeStatusTest resultOperations
delete_menu_3.patch7.32 KBIgnoredNoneNone

#5

webernet - July 7, 2007 - 14:51
Status:needs review» needs work

Some entries are being left behind in menu_links.

#6

pwolanin - July 7, 2007 - 15:14
Assigned to:Anonymous» pwolanin
Status:needs work» needs review

now with code to delete links to the menu overview page for the menu being deleted.

AttachmentSizeStatusTest resultOperations
delete_menu_4.patch7.88 KBIgnoredNoneNone

#7

pwolanin - July 7, 2007 - 15:16

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

pwolanin - July 7, 2007 - 16:11

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.

AttachmentSizeStatusTest resultOperations
delete_menu_5.patch9.32 KBIgnoredNoneNone

#9

pwolanin - July 7, 2007 - 16:32

capitalization fix from webernet.

AttachmentSizeStatusTest resultOperations
delete_menu_6.patch9.37 KBIgnoredNoneNone

#10

webernet - July 7, 2007 - 16:38
Status:needs review» reviewed & tested by the community

Tested, code looks good.

#11

webernet - July 9, 2007 - 18:16

Rerolled after http://drupal.org/node/151055 was committed.

AttachmentSizeStatusTest resultOperations
patch_150.txt9.24 KBIgnoredNoneNone

#12

Gábor Hojtsy - July 12, 2007 - 10:58
Status:reviewed & tested by the community» needs review

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

pwolanin - July 12, 2007 - 11:17

@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

pwolanin - July 12, 2007 - 11:51

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.

AttachmentSizeStatusTest resultOperations
delete_menu_7.patch9.51 KBIgnoredNoneNone

#15

webernet - July 15, 2007 - 17:26
Status:needs review» reviewed & tested by the community

Tested again, still works as expected.

#16

webernet - July 23, 2007 - 20:23

Rerolled to fix offset.

AttachmentSizeStatusTest resultOperations
patch_155.txt9.38 KBIgnoredNoneNone

#17

Gábor Hojtsy - July 25, 2007 - 14:46
Status:reviewed & tested by the community» fixed

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

Anonymous - August 8, 2007 - 15:04
Status:fixed» closed
 
 

Drupal is a registered trademark of Dries Buytaert.