Make WordPress Core

Opened 12 years ago

Last modified 5 years ago

#20275 new defect (bug)

wp_update_nav_menu hook is not fired when nav menu item is auto-added

Reported by: inderpreet99's profile inderpreet99 Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Menus Keywords: has-patch dev-feedback
Focuses: Cc:

Description

WP calls the action hook 'wp_update_nav_menu' when a navigation menu is updated. For example, go to Site Design -> Custom Menus -> Click the "Save Menu" button on a menu.

Next to the Menu title, the Custom Menus provide the user to "Automatically add new top-level pages". When this option is checked, and a new top-level page is created, the 'wp_update_nav_menu' hook is not fired. So the attached nav-menu-hooks.diff patch fixes that.

I would also suggest that we do not use the same hook 'wp_update_nav_menu' twice (as it is currently being done) with different amount of arguments, because this causes a PHP Warning

PHP Warning:  Missing argument 2 for x_save_footer() in /var/www/branches/x.trunk/wp-content/themes/x/functions.php on line 815, referer: http://x.com/x/wp-admin/nav-menus.php

for code:

add_action('wp_update_nav_menu', 'x_save_footer', 10, 2);

So, the hook call at nav-menu.php:255 has 2 parameters and the call at nav-menus.php:383 has 1 parameter.

As a fix, the action hook in nav-menu.php should be named something else, like 'wp_update_nav_menu_object' (because it is updating the object), as done in the attached nav-menu-hooks.diff patch. This will also prevent the same hook from being fired twice when updating the nav menus.

Attachments (1)

nav-menu-hooks.diff (561 bytes) - added by inderpreet99 12 years ago.
nav-menu.php: new hook and hook name change

Download all attachments as: .zip

Change History (7)

@inderpreet99
12 years ago

nav-menu.php: new hook and hook name change

#1 follow-up: @SergeyBiryukov
12 years ago

Renaming an existing hook isn't a good idea, some plugins are using it. I've found two:

wp-super-cache/trunk/wp-cache-phase2.php
add-descendants-as-submenu-items/trunk/add-descendants-as-submenu-items.php

#2 in reply to: ↑ 1 @mark8barnes
12 years ago

  • Cc mark@… added
  • Type changed from enhancement to defect (bug)

Replying to SergeyBiryukov:

Renaming an existing hook isn't a good idea, some plugins are using it. I've found two:

wp-super-cache/trunk/wp-cache-phase2.php
add-descendants-as-submenu-items/trunk/add-descendants-as-submenu-items.php

But surely having these two actions named the same is a bug, not a design decision. The two hooks fire at completely different times in the save menu process - the first fires when the menu itself is updated, the second fires when the menu items are updated by javascript.

Incidentally, trying to monitor a menu by adding a hook to discover a menu change is way harder than it ought to be - partly for the reason above, and partly because when javascript is disabled, no hooks are in place.

#3 @HumanNic
12 years ago

  • Cc HumanNic added

#4 @rachelbaker
11 years ago

  • Cc rachelbaker added

#5 @chriscct7
9 years ago

  • Keywords dev-feedback added

#6 @inderpreet99
8 years ago

  • Version 3.3.1 deleted

Like @mark8barnes, we should rename the hooks since they're being called at completely different times. This is a bug. It's a pretty important hook, so I think it should be fixed.

Although I don't know why @mark8barnes suggests that javascript is firing one of the hooks. I've disabled JS and I do not see that happening.

If we're not comfortable with renaming the hooks (due to those two plugins), we could just add two new hooks with unique names. This would allow the developers to target a specific hook.

PS: Since it's been 4 years, the hooks have moved around. They are now located in wp-admin/includes/nav-menu.php:1048 and wp-includes/nav-menu.php:346.

Note: See TracTickets for help on using tickets.