WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 5 weeks ago

#37344 new defect (bug)

Side effects on 'nav_menu_item_args' filter when appending to arguments

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

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 3 years ago.
menu_before.png (4.4 KB) - added by dgwyer 3 years ago.
menu_after.png (4.2 KB) - added by dgwyer 3 years ago.
37344-2.patch (1.0 KB) - added by campusboy1987 18 months ago.
Fix headers

Download all attachments as: .zip

Change History (10)

@dgwyer
3 years ago

@dgwyer
3 years ago

@dgwyer
3 years ago

#1 @ocean90
3 years ago

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

#2 @welcher
2 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
2 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
18 months ago

Fix headers

#4 @campusboy1987
18 months 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
18 months ago

@campusboy1987 Thanks for updating the patch!

#6 @SergeyBiryukov
5 weeks ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release
Note: See TracTickets for help on using tickets.