WordPress.org

Make WordPress Core

Opened 16 months ago

Closed 6 months ago

Last modified 7 days ago

#47056 closed enhancement (fixed)

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

Reported by: MikeSchinkel Owned by: audrasjb
Milestone: 5.4 Priority: normal
Severity: normal Version: 5.2
Component: Menus Keywords: has-patch has-dev-note
Focuses: Cc:

Description

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.

Attachments (7)

47056.patch (837 bytes) - added by MikeSchinkel 16 months ago.
Simple patch to include just the do_action hook
47056.2.patch (837 bytes) - added by MikeSchinkel 16 months ago.
Simple patch to include just the do_action hook
extending-Walker_Nav_Menu_Edit.txt (6.8 KB) - added by MikeSchinkel 15 months ago.
Shows list of plugins that extend Walker_Nav_Menu_Edit
implementing-wp_nav_menu_item_custom_fields.txt (8.7 KB) - added by MikeSchinkel 15 months ago.
Shows list of plugins that implementing the 'wp_nav_menu_item_custom_fields' hook
uses-wp_edit_nav_menu_walker.txt (21.8 KB) - added by MikeSchinkel 15 months ago.
Shows list of plugins that use the 'wp_edit_nav_menu_walker' hook
47056-3.diff (1.2 KB) - added by birgire 15 months ago.
47056-customize.diff (876 bytes) - added by celloexpressions 6 months ago.
Equivalent action for the nav menu item customize control template

Download all attachments as: .zip

Change History (35)

@MikeSchinkel
16 months ago

Simple patch to include just the do_action hook

@MikeSchinkel
16 months ago

Simple patch to include just the do_action hook

#1 @MikeSchinkel
16 months ago

The second patch was an accidental duplication.

#2 @MikeSchinkel
15 months ago

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
15 months ago

Shows list of plugins that extend Walker_Nav_Menu_Edit

@MikeSchinkel
15 months ago

Shows list of plugins that implementing the 'wp_nav_menu_item_custom_fields' hook

@MikeSchinkel
15 months ago

Shows list of plugins that use the 'wp_edit_nav_menu_walker' hook

#3 @helgatheviking
15 months ago

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

@birgire
15 months ago

#4 follow-up: @birgire
15 months ago

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.

#5 @MikeSchinkel
15 months ago

@birgire Nice! Thanks.

#6 @helgatheviking
15 months ago

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.

#7 @MikeSchinkel
15 months ago

"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.

Last edited 15 months ago by MikeSchinkel (previous) (diff)

#8 @helgatheviking
15 months ago

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

#9 in reply to: ↑ 4 ; follow-up: @MikeSchinkel
15 months ago

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.

#10 in reply to: ↑ 9 @birgire
15 months ago

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.

Last edited 15 months ago by birgire (previous) (diff)

#11 @MikeSchinkel
15 months ago

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

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


14 months ago

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


14 months ago

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


12 months ago

#15 @audrasjb
12 months ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

#16 @adamsilverstein
8 months ago

  • Milestone changed from Awaiting Review to 5.4

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

#17 @audrasjb
6 months ago

@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 :)

#18 @helgatheviking
6 months ago

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.

#19 @MikeSchinkel
6 months ago

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

#20 follow-up: @SergeyBiryukov
6 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

#21 in reply to: ↑ 20 @MikeSchinkel
6 months ago

Replying to SergeyBiryukov:

Thank you!

#22 @adamsilverstein
6 months ago

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
6 months ago

Equivalent action for the nav menu item customize control template

#23 follow-up: @celloexpressions
6 months ago

  • Keywords dev-feedback removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#24 in reply to: ↑ 23 @SergeyBiryukov
6 months ago

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!

#25 @SergeyBiryukov
6 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#26 @SergeyBiryukov
6 months ago

  • Keywords needs-dev-note added

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

#27 @birgire
6 months ago

  • Keywords has-dev-note added; needs-dev-note removed

#28 @helgatheviking
7 days ago

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.

Note: See TracTickets for help on using tickets.