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

pwolanin’s picture

Status: Active » Needs work
StatusFileSize
new5.97 KB

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

RobRoy’s picture

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.

pwolanin’s picture

Status: Needs work » Needs review
StatusFileSize
new6.85 KB

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.

pwolanin’s picture

StatusFileSize
new7.32 KB

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.

webernet’s picture

Status: Needs review » Needs work

Some entries are being left behind in menu_links.

pwolanin’s picture

Assigned: Unassigned » pwolanin
Status: Needs work » Needs review
StatusFileSize
new7.88 KB

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

pwolanin’s picture

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

pwolanin’s picture

StatusFileSize
new9.32 KB

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.

pwolanin’s picture

StatusFileSize
new9.37 KB

capitalization fix from webernet.

webernet’s picture

Status: Needs review » Reviewed & tested by the community

Tested, code looks good.

webernet’s picture

StatusFileSize
new9.24 KB

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

gábor hojtsy’s picture

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.

pwolanin’s picture

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

pwolanin’s picture

StatusFileSize
new9.51 KB

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.

webernet’s picture

Status: Needs review » Reviewed & tested by the community

Tested again, still works as expected.

webernet’s picture

StatusFileSize
new9.38 KB

Rerolled to fix offset.

gábor hojtsy’s picture

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.

Anonymous’s picture

Status: Fixed » Closed (fixed)