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

[danielbachhuber]

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

[SergeyBiryukov]

Related: #14134, comment:11:ticket:16799, #22189.

[celloexpressions]

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

[celloexpressions]

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

[celloexpressions]

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.

[obenland]

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

[ericmann]

Updated patch file, with tests

[ericmann]

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]

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

[celloexpressions]

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.

[westonruter]

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.