Make WordPress Core

Opened 8 years ago

Last modified 2 years ago

#37344 new defect (bug)

Side effects on 'nav_menu_item_args' filter when appending to arguments

Reported by: dgwyer's profile dgwyer Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.4
Component: Menus Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

The 'nav_menu_item_args' filter allows you to filter the arguments for a single nav menu item which works well when you are overwriting arguments.

When you try to append an argument the original arguments object is updated. This means that the next menu item passes in the updated object to the 'nav_menu_item_args' filter which leads to unexpected side effects.

For demonstration purposes here's how to reproduce the issue.

function format_menu_item_args( $args, $item, $depth ) {
	$args->before .= rand(0,9);
	return $args;
}
add_filter( 'nav_menu_item_args', 'format_menu_item_args', 10, 3);

What you should see before every menu item is a random integer between 0-9. What happens instead is the integer from the previous menu item is passed into the next. So by the time the 'nth' menu item is processed you get 'n' integers outputted whereas only one integer per menu item should be outputted.

I have added a patch to solve this by casting the $args object to an array early in start_el(). If left as an array this would break code already using the 'nav_menu_item_args' filter as an $args object is passed into the filter. Also code further down start_el() would need updating to use array rather than object notation. So, to get around this I've casted $args to an object again.

This is a new object and so there won't be any issues with referencing the original nav menu args object. Plus, existing code using the 'nav_menu_item_args' filter will still work fine.

See attached screenshots for views of a nav menus before and after the patch is applied.

Note: The default type for $args in start_el() is already specified as an empty array but it's actually an object that's being passed in.

Attachments (4)

37344.patch (952 bytes) - added by dgwyer 8 years ago.
menu_before.png (4.4 KB) - added by dgwyer 8 years ago.
menu_after.png (4.2 KB) - added by dgwyer 8 years ago.
37344-2.patch (1.0 KB) - added by campusboy1987 6 years ago.
Fix headers

Download all attachments as: .zip

Change History (11)

@dgwyer
8 years ago

@dgwyer
8 years ago

@dgwyer
8 years ago

#1 @ocean90
8 years ago

  • Keywords has-patch added
  • Version changed from trunk to 4.4

#2 @welcher
7 years ago

  • Keywords needs-refresh added; has-patch removed

@dgwyer thanks reporting! The patch didn't apply cleanly for me can you refresh it using the root of the project?

#3 @dgwyer
7 years ago

I haven't time to add an updated patch right now, but you should be able to apply the changes manually as it's only one additional line of code, plus a minor modification to another line.

@campusboy1987
6 years ago

Fix headers

#4 @campusboy1987
6 years ago

  • Keywords has-patch added; needs-refresh removed

@dgwyer thanks for the bug! Faced the same task and came to this ticket.

Patch refreshed, @ocean90, @welcher can you please try?

#5 @dgwyer
6 years ago

@campusboy1987 Thanks for updating the patch!

#6 @SergeyBiryukov
5 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

#7 @smerriman
2 years ago

Just stumbled across this bug myself. The original post says it works fine if you're overwriting args - just not appending them.

This isn't true - it also fails for overwriting args. For example, if you overwrite an argument based on testing something in $item, it will overwrite the argument for all later menu items even if they fail that test.

The filter doesn't really seem usable at all for this reason.

Note: See TracTickets for help on using tickets.