WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 2 years ago

#28138 new defect (bug)

Updating menu item requires passing all of a menu item's data to wp_update_nav_menu_item()

Reported by: danielbachhuber Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Menus Keywords: has-patch has-tests
Focuses: Cc:

Description

When using wp_update_nav_menu_item() to updating an existing menu item, you need to pass all data associated with a menu item in order to have the data persist.

This is because the $defaults are set to default values, instead of the existing menu item values (ref).

If the menu item exists, the defaults should be set to the existing menu item values.

Discovered in https://github.com/wp-cli/wp-cli/pull/1182

Attachments (3)

28138.diff (2.0 KB) - added by celloexpressions 3 years ago.
If a menu item is existing, populate the defaults with the existing values.
28138.2.diff (5.0 KB) - added by ericmann 2 years ago.
Updated patch file, with tests
28138.3.diff (5.9 KB) - added by ericmann 2 years ago.
Alternative patch with is_array() check instead of implode()ing and re-explode()ing the array.

Download all attachments as: .zip

Change History (11)

@celloexpressions
3 years ago

If a menu item is existing, populate the defaults with the existing values.

#2 @celloexpressions
3 years ago

  • Keywords has-patch added; needs-patch removed

I think 28138.diff works for now, but it seems like there should be a better way to approach this. Updating an item could be a completely different process from creating one, with only the fields where data is passed being handled in an update. wp_update_nav_menu_item() is a pretty big mess; it could maybe use some refactoring.

I'd like to be able to only pass changed data in with the Menu Customizer; otherwise I'll likely end up doing something like 28138.diff before calling wp_update_nav_menu_item().

#3 @celloexpressions
2 years ago

  • Milestone changed from Future Release to 4.3
  • Version set to 3.0

Menu Customizer wraps this function with a function that does essentially what my patch here does. Let's fix this either before or on merge and remove the extra nonsense so that we can just call the function directly and also fix this for the admin page. See #32576.

#4 @obenland
2 years ago

@celloexpressions can you update your patch with some unit tests?

@ericmann
2 years ago

Updated patch file, with tests

#5 @ericmann
2 years ago

  • Keywords has-tests added; needs-unit-tests removed

Updated the patch and added some unit tests. I noticed, while writing the tests, a minor bug in the conditional. Since we're calling wp_setup_nav_menu_item() to populate the nav item, the class list is auto-converted into an array. This causes the later invocation of explode() to, well, explode when an existing item is updated. I've modified the patch slightly to re-convert the class list into a string upon retrieval so explode() behaves properly.

Alternatively, we could just run an is_array() check before sanitizing the class elements. Both patterns work, and I don't really think it matters. However, for the sake of clarity, I'm attaching a second patch with an array check in a moment. Feel free to go with either one (the tests are the same and pass regardless).

@ericmann
2 years ago

Alternative patch with is_array() check instead of implode()ing and re-explode()ing the array.

This ticket was mentioned in Slack in #core by ericmann. View the logs.


2 years ago

#7 @celloexpressions
2 years ago

Thanks for the unit tests @eicmann! I have no opinion as to which approach is better, but it sounds like the alternate one is.

This is no longer relevant to menus in the Customizer, which already have all of the data loaded before calling this function after being refactored. @westonruter would have a better idea about whether that needs to be the case or could be otherwise improved upon, but I'm starting to think we may want to punt this for now.

#8 @westonruter
2 years ago

  • Milestone changed from 4.3 to Future Release

That's right. In Menu Customizer, each nav_menu_item setting contains a fully-populated array of menu item properties as required by wp_setup_nav_menu_item(), so it is not relevant to Menus in the Customizer.

Note: See TracTickets for help on using tickets.