WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 10 months ago

#38904 new enhancement

Add action hook in class-walker-nav-menu-edit.php

Reported by: timersys Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.7
Component: Menus Keywords: has-patch dev-feedback
Focuses: administration Cc:

Description

The class-walker-nav-menu-edit.php is lack for a simple action hook that let authors add their own fields to the admin menu.
In the last week I found 4 different users complaining their plugins/themes are not compatible ( Ubermenu, Geotargeting Pro, Salient theme and another theme I can't remember) because every of them are replacing the whole walker.
The whole menu ecosystem could simple work by adding a hook ( patch added ) and makes totally sense.

We already have filters/actions for settings wp_setup_nav_menu_item , wp_update_nav_menu_item so I don't see a reason of why not having a hook to add the gui of this fields without modifying the whole walker.

Related to https://core.trac.wordpress.org/ticket/18584 but could solve lot of issues until that ticket from 5 years got solved and merged.

Attachments (4)

#38904.diff (2.6 KB) - added by timersys 2 years ago.
38904.diff (2.2 KB) - added by welcher 2 years ago.
38904.2.diff (1.1 KB) - added by welcher 2 years ago.
38904.3.diff (2.7 KB) - added by timersys 2 years ago.
38904.3

Download all attachments as: .zip

Change History (16)

@timersys
2 years ago

#1 @welcher
2 years ago

Thanks for the patch!

There was a change on line 73 in the diff that seems out of place with the intent of the patch.

I think we may need a more descriptive name for the action. add_menu_settings is too generic. Perhaps something like, admin_nav_menu_item_below_settings?.

Patch updated to address the above, some documentation standards and to generate from the root of the repo instead of /src

Last edited 2 years ago by welcher (previous) (diff)

@welcher
2 years ago

#2 @welcher
2 years ago

  • Focuses administration added
  • Keywords has-patch dev-feedback added

@welcher
2 years ago

#3 @welcher
2 years ago

  • Keywords reporter-feedback added; dev-feedback removed

I took another look at the original patch and there were a fair amount of markup changes that had nothing to do with adding the action. I've reverted those and uploaded a new patch containing just the new action.

@timersys can you review it and be sure it's still what you were wanting to do?

#4 @timersys
2 years ago

@welcher it works perfectly. Not sure what I did with the original patch that got messed. Glad you catch it!
We just need to instruct plugin authors to use the hook instead of replacing the entire class now :)

#5 @welcher
2 years ago

  • Keywords dev-feedback added; reporter-feedback removed

Thanks for checking! We just need someone to commit this now :)

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


2 years ago

#7 @adamsilverstein
2 years ago

Related: #27280, #14414, #18889 and #18584.

Last edited 2 years ago by adamsilverstein (previous) (diff)

#8 @adamsilverstein
2 years ago

@timersys & @welcher - thanks for the bug report and patch.

My concern with the current patch is it is so narrowly focused & application/need specific. While it solves the reported use case, what if plugins want to add something to a submenu or alter the classes added to menus? We might need many hooks to address every use case. I think this is the reason the general approach of replacing the walker has been used, and was the resolution for #13342.

A more generic filter applied to the return of start_el might be worth considering (as suggested in #27280). This would still let you add menu items (although it would require a bit more care to find the correct insertion point) without replacing Walker_Nav_Menu_Edit entirely.

What do you think @timersys, would that still be useful?

Last edited 2 years ago by adamsilverstein (previous) (diff)

#9 follow-up: @timersys
2 years ago

HI @adamsilverstein , thanks for looking into it. Adding the filter at the end of the generated HTML looks ugly and dirty imo.
I would rather prefer to add filters and hooks everywhere to cover as much use cases as possible. Is not what wordpress does anyway :)

Adding just that hook will make lot of people happier. And I don't mean just developers. I found lots of complaints from users because their plugins are not compatible between each other because they simply override the walker.

#10 in reply to: ↑ 9 @adamsilverstein
2 years ago

Replying to timersys:

just that hook will make lot of people happier. And I don't mean just developers. I found lots of complaints from users because their plugins are not compatible between each other because they simply override the walker.

This is a primary reason hooks are preferable! What other actions do you think would be worth considering as part of this change?

@timersys
2 years ago

38904.3

#11 @timersys
2 years ago

I added a couple of filters that should cover most cases. But I guess once developers actually start using this, with more feedback we will be able to tune it up.

#12 @crstnio
10 months ago

I want to build multiple small plugins to extend menu items. But currently, this is not possible.

We need hooks inside Walker_Nav_Menu_Edit. The more, the better.

Note: See TracTickets for help on using tickets.