Add 'wp_nav_menu_item_custom_fields' hook in Walker_Nav_Menu_Edit::start_el()

[MikeSchinkel]

This is admittedly related to several other tickets, most notably of #38904 and #18584.

However this ticket is very limited in scope as both those expanded in scope until they stalled — one of them is 8 years old — but there is a "Let's not let perfect be the enemy of good" solution that has been proposed, is working in at least two plugins, and that we could implement trivially today.

The plugins using this of course cannot be used together because of this functionality not being in core and thus each has to declare their own version of a menu walker class modeled after Walker_Nav_Menu_Edit.

So I propose we add the following patch to the next version of WordPress and then deal with all the other use-cases and concerns later, especially because there is proven working code that uses this action hook in the wild and adding it would allow those plugin vendors to drop their custom Walkers and then their plugins would be compatible with each other.

https://core.trac.wordpress.org/attachment/ticket/18584/nav-menu.patch

Given that WordPress generally views user-experience as the highest priority, and it is rather likely that users interested in enhancing menus would try more than one menuing plugin at the same time it would seem that adding this action hook should be a no-brainer given it will minimize the number of problems end-users experience when they try to use more than one of these menuing plugins.

[MikeSchinkel]

Simple patch to include just the do_action hook

[MikeSchinkel]

Simple patch to include just the do_action hook

[MikeSchinkel]

The second patch was an accidental duplication.

[MikeSchinkel]

Per @desrosj at the 2019 WCATL contributor day event I did some research to find out how many current WordPress plugins are:

1. Extending Walker_Nav_Menu_Edit
2. Replacing Walker_Nav_Menu_Edit by using a 'wp_edit_nav_menu_walker' hook
3. Already implementing a 'wp_nav_menu_item_custom_fields' hook

Based on ​downloading of all of the latest versions of plugins from the WordPress repo I have come up with the following metrics:

1. 72 plugins extend Walker_Nav_Menu_Edit
2. 156 plugins use the 'wp_edit_nav_menu_walker' hook
3. 60 already implement the 'wp_nav_menu_item_custom_fields' hook

I am attaching the files where I got these numbers.

Here are links to gists of the raw data:

1. ​Extends `Walker_Nav_Menu_Edit`
2. ​Uses the `'wp_edit_nav_menu_walker'` hook
3. ​Implements the `'wp_nav_menu_item_custom_fields'` hook

The problems replacing Walker_Nav_Menu_Edit causes is that it ensures no two plugins that do so can be used together — with some obvious caveats, but those do not change the fundamental problem.

I think it is reasonable to assume that if someone is downloading plugins to enhance their menus chances are there are trying to use more than one such plugin. The fact that these plugins are mutually exclusive means that all those people will run into incompatibilities, and ones they are likely not even be able to understand what is going wrong. This seems destined to ensure a bad UX which is the opposite of the WordPress ethos.

Since the primary concern that @desrosj mentioned to not add this hook is the potential for issues, it would seem that so much use in-the-wild proves that the hook can be used without (at least existing) issues. And adding the hook can eliminate the incompatibility between any of plugins that update to use the new hook instead of still replacing Walker_Nav_Menu_Edit.

So I am hoping that makes this patch a no-brainer?

[MikeSchinkel]

Shows list of plugins that extend Walker_Nav_Menu_Edit

[MikeSchinkel]

Shows list of plugins that implementing the 'wp_nav_menu_item_custom_fields' hook

[MikeSchinkel]

Shows list of plugins that use the 'wp_edit_nav_menu_walker' hook

[helgatheviking]

YESSSSSSS! Glad to see this get picked back up and really hope it gets merged in.

[birgire]

The plugins search on wpdirectory.net is also very helpful.

Here's the list of the 60 plugins that use the wp_nav_menu_item_custom_fields hook:

​https://wpdirectory.net/search/01DAY87H5P9AAFY90DGWZWWHHP

Nice to see a joint effort there among these plugins to address this.

There seems to be a strong case for adding it into core.

As this hook is not for the Customizer, is the plan to handle that separately or in the other existing tickets?

Looking at 47056.2.patch​ it would benefit from adding the inline docs.

​Here's an example from one such plugin (Theme my login). I based the inline title description from this one.

Some plugins seems to add the nav menu ID to the hook, like ​here and ​here.

47056-3.diff​ adds inline docs and adds the navigational menu ID as an hook argument.

[MikeSchinkel]

@birgire Nice! Thanks.

[helgatheviking]

As this hook is not for the Customizer, is the plan to handle that separately or in the other existing tickets?

Please don't wait for a Customizer-ready solution to add this hook. That's already been provided as a reason to not add this hook that we've been waiting on now for 8 years.

[MikeSchinkel]

"Please don't wait for a Customizer-ready solution to add this hook. That's already been provided as a reason to not add this hook that we've been waiting on now for 8 years."

Exactly. The entire point of this ticket is to not let perfect be the enemy of good and to add a hook we know is needed as it is already used by many plugins in the wild in mutually incompatible ways.

Let's give plugin developers the tools needed to fix the current known UX problem, and address the perfect in future tickets. 8 years is long enough to wait.

[helgatheviking]

Thanks Mike! As one of the plugin developers using the wp_nav_menu_item_custom_fields I really appreciate you pushing this.

[MikeSchinkel]

Replying to birgire:

Some plugins seems to add the nav menu ID to the hook, like ​here and ​here.

Just a follow up on this. Looking at core source, that parameters is commented ​to be unused, which is why I did not include in my patch.

I personally don't have an issue to include it, if some plugin actually does pass it, but I visually tracied the code I don't see how it would pass it. I think that might have been a legacy parameter that is no longer used.

But if it is in use, okay my me to include. OTOH if it does not, would be a shame to include it.

[birgire]

Replying to MikeSchinkel:

Replying to birgire:

Some plugins seems to add the nav menu ID to the hook, like ​here and ​here.

Just a follow up on this. Looking at core source, that parameters is commented ​to be unused, which is why I did not include in my patch.

I personally don't have an issue to include it, if some plugin actually does pass it, but I visually tracied the code I don't see how it would pass it. I think that might have been a legacy parameter that is no longer used.

But if it is in use, okay my me to include. OTOH if it does not, would be a shame to include it.

Thanks for bringing that up. I noticed this when looking at the inline docs for the patch, but my understanding was that it meant that it was not used within the method (it could be my misunderstanding, but I will have to look better into that). (Maybe these lines could be better explained in another doc ticket)

What I also had in mind was that if we would e.g. only use 3 input arguments for the new hook, then plugins might get PHP notices for callbacks that expect 4 or 5 inputs.

But we should definitely look better into that.

PS:

For reference, here is the inline documentation for the related start_el() methods.

One would expect same types for the inputs (e.g. the $args), but I guess it might need to be rechecked and synced e.g. in another ticket.

https://core.trac.wordpress.org/browser/tags/5.2/src/wp-admin/includes/class-walker-nav-menu-edit.php#L44

class Walker_Nav_Menu_Edit extends Walker_Nav_Menu {

        // ... cut ...

	/**
	 * Start the element output.
	 *
	 * @see Walker_Nav_Menu::start_el()
	 * @since 3.0.0
	 *
	 * @global int $_wp_nav_menu_max_depth
	 *
	 * @param string $output Used to append additional content (passed by reference).
	 * @param object $item   Menu item data object.
	 * @param int    $depth  Depth of menu item. Used for padding.
	 * @param array  $args   Not used.
	 * @param int    $id     Not used.
	 */
	public function start_el( &$output, $item, $depth = 0, $args = array(), $id = 0 ) {

https://core.trac.wordpress.org/browser/tags/5.2/src/wp-includes/class-walker-nav-menu.php#L104

class Walker_Nav_Menu extends Walker {

        // ... cut ...

	/**
	 * Starts the element output.
	 *
	 * @since 3.0.0
	 * @since 4.4.0 The {@see 'nav_menu_item_args'} filter was added.
	 *
	 * @see Walker::start_el()
	 *
	 * @param string   $output Used to append additional content (passed by reference).
	 * @param WP_Post  $item   Menu item data object.
	 * @param int      $depth  Depth of menu item. Used for padding.
	 * @param stdClass $args   An object of wp_nav_menu() arguments.
	 * @param int      $id     Current item ID.
	 */
	public function start_el( &$output, $item, $depth = 0, $args = array(), $id = 0 ) {

https://core.trac.wordpress.org/browser/tags/5.2/src/wp-includes/class-wp-walker.php#L79

class Walker {

        // ... cut ...

	/**
	 * Start the element output.
	 *
	 * The $args parameter holds additional values that may be used with the child
	 * class methods. Includes the element output also.
	 *
	 * @since 2.1.0
	 * @abstract
	 *
	 * @param string $output            Used to append additional content (passed by reference).
	 * @param object $object            The data object.
	 * @param int    $depth             Depth of the item.
	 * @param array  $args              An array of additional arguments.
	 * @param int    $current_object_id ID of the current item.
	 */
	public function start_el( &$output, $object, $depth = 0, $args = array(), $current_object_id = 0 ) {}

Here are some relevant lines from wp_get_nav_menu_to_edit() function:

$walker_class_name = apply_filters( 'wp_edit_nav_menu_walker', 'Walker_Nav_Menu_Edit', $menu_id );

https://core.trac.wordpress.org/browser/tags/5.2/src/wp-admin/includes/nav-menu.php#L1015

$result .= walk_nav_menu_tree( array_map( 'wp_setup_nav_menu_item', $menu_items ), 0, (object) array( 'walker' => $walker ) );

https://core.trac.wordpress.org/browser/tags/5.2/src/wp-admin/includes/nav-menu.php#L1049

We should also checkout the relevant display_element() methods.

[MikeSchinkel]

@birgire Good catch on those additional references that I obviously missed. It does seem that additional parameter is likely used.

[adamsilverstein]

Let's get this committed for 5.4 - it is a small, non destructive, helpful hook.

[audrasjb]

@adamsilverstein do you think it can be committed in time for 5.4 beta 1 in the next few days? Otherwise, I think it's better to move it back to Future release :)

[helgatheviking]

As Mike mentioned, this issue dates back 8 years. So if there's any chance to get this into 5.4, I really hope it will make it.

[MikeSchinkel]

For further reference I attended WordCamp contributor day specifically so I could collaborate on this patch since that was recommended to me on Slack. I got great feedback on it while there from @desrosj, then on this ticket, and I followed up numerous times via Slack in public and via DM.

Just want to say it is certainly is not a great experience to spend free time attend contributor day and then get so little traction on such a simple ticket that have no objections for including into core. Also not a great way to motivate someone to participate in future contributor day eithers. :-(

(Just giving feedback on how having a good ticket ignored can be demotivating. Do with the feedback as you will.)

[SergeyBiryukov]

In 47190:

Menus: Introduce wp_nav_menu_item_custom_fields action that fires just before the move buttons of a nav menu item in the menu editor.

Props MikeSchinkel, birgire, sebastian.pisula, desrosj, helgatheviking.
Fixes #47056. See #38904, #18584.

[MikeSchinkel]

Replying to SergeyBiryukov:

Thank you!

[adamsilverstein]

Thanks @SergeyBiryukov and apologies for the slow response as I've been out of the office for the last week and a half. I do plan to revisit everything I own for 5.4 in the next few days.

[celloexpressions]

Equivalent action for the nav menu item customize control template

[celloexpressions]

It is irresponsible to enable plugin authors to use a new hook, but only for users using a particular version of an interface. Including a customizer version of this hook is a requirement for adding the hook at all. We actually could have done that when that interface shipped in 4.3, but chose not to so that there was parity with wp-admin.

I agree with others that it is well past time to fix this issue. Rather than revert and try again, I would suggest a simple action with no parameters in class-wp-customize-nav-menu-item-control. This allows plugins to add markup to the JavaScript template for menu items, and manage any custom fields in JavaScript. Note that menu-item-specific data is added to the markup template in JavaScript, so the PHP hook doesn't have or need parameter context. See 47056-customize.diff​.

[SergeyBiryukov]

Replying to celloexpressions:

Including a customizer version of this hook is a requirement for adding the hook at all.

Indeed, thanks for bringing that up!

[SergeyBiryukov]

In 47350:

Menus: Introduce wp_nav_menu_item_custom_fields_customize_template action that fires at the end of the form field template for nav menu items in the customizer.

This brings parity with the wp_nav_menu_item_custom_fields action added in [47190] for the Menus screen.

Props celloexpressions.
Fixes #47056.

[SergeyBiryukov]

Marking this as needs-dev-note to avoid potential confusion like in #49500.

[birgire]

Thanks @audrasjb for dev notes:

​https://make.wordpress.org/core/2020/02/25/wordpress-5-4-introduces-new-hooks-to-add-custom-fields-to-menu-items/

[helgatheviking]

Hey y'all.... the dev notes on wp_nav_menu_item_custom_fields_customize_template could use some more detail as currently there's no docu on how to make that hook functional.

I've figured out how to display the extra fields, but without at least a point in the right direction it's unclear how to get the data for the menu item to the output. The other fields are using {{ data.something }} but I can't find out to insert properties to the data object as there's no filter for the WP_Customize_Nav_Menu_Item_Control::json() function (or it's parent).

It's also unclear how to send the data back to the DB... I expect something in the wp.customizer scripts, but can't yet find where.

I'm willing to write a tutorial on this, but going to need a point in the right direction.