WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 4 months ago

#47056 reviewing enhancement

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

Reported by: MikeSchinkel Owned by: audrasjb
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 5.2
Component: Menus Keywords: has-patch dev-feedback
Focuses: Cc:
PR Number:

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

47056.patch (837 bytes) - added by MikeSchinkel 7 months ago.
Simple patch to include just the do_action hook
47056.2.patch (837 bytes) - added by MikeSchinkel 7 months ago.
Simple patch to include just the do_action hook
extending-Walker_Nav_Menu_Edit.txt (6.8 KB) - added by MikeSchinkel 7 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 7 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 7 months ago.
Shows list of plugins that use the 'wp_edit_nav_menu_walker' hook
47056-3.diff (1.2 KB) - added by birgire 7 months ago.

Download all attachments as: .zip

Change History (21)

@MikeSchinkel
7 months ago

Simple patch to include just the do_action hook

@MikeSchinkel
7 months ago

Simple patch to include just the do_action hook

#1 @MikeSchinkel
7 months ago

The second patch was an accidental duplication.

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

Shows list of plugins that extend Walker_Nav_Menu_Edit

@MikeSchinkel
7 months ago

Shows list of plugins that implementing the 'wp_nav_menu_item_custom_fields' hook

@MikeSchinkel
7 months ago

Shows list of plugins that use the 'wp_edit_nav_menu_walker' hook

#3 @helgatheviking
7 months ago

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

@birgire
7 months ago

#4 follow-up: @birgire
7 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
7 months ago

@birgire Nice! Thanks.

#6 @helgatheviking
7 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
7 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 7 months ago by MikeSchinkel (previous) (diff)

#8 @helgatheviking
7 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
7 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
7 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 7 months ago by birgire (previous) (diff)

#11 @MikeSchinkel
7 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.


6 months ago

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


5 months ago

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


4 months ago

#15 @audrasjb
4 months ago

  • Owner set to audrasjb
  • Status changed from new to reviewing
Note: See TracTickets for help on using tickets.