WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 6 months ago

#38904 assigned enhancement

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

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

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 3 years ago.
38904.diff (2.2 KB) - added by welcher 3 years ago.
38904.2.diff (1.1 KB) - added by welcher 3 years ago.
38904.3.diff (2.7 KB) - added by timersys 3 years ago.
38904.3

Download all attachments as: .zip

Change History (19)

@timersys
3 years ago

#1 @welcher
3 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 3 years ago by welcher (previous) (diff)

@welcher
3 years ago

#2 @welcher
3 years ago

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

@welcher
3 years ago

#3 @welcher
3 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
3 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
3 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.


3 years ago

#7 @adamsilverstein
3 years ago

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

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

#8 @adamsilverstein
3 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 3 years ago by adamsilverstein (previous) (diff)

#9 follow-up: @timersys
3 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
3 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
3 years ago

38904.3

#11 @timersys
3 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
18 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.

#13 @MikeSchinkel
6 months ago

  • Keywords needs-refresh added

Hi @adamsilverstein - I am running into this very same problem with the [If Menu plugin](https://wordpress.org/plugins/if-menu/) a client started using on their site.

What is the reason this stalled 2 years ago, and is there anything we can do to put onto the radar for next release?

I would question if the hook names in the patch are consistent with the rest of the hook names in nav-menu.php of if they should be updated. And if someone needs to have a go at updating this patch I can certainly do so, assuming there is some reasonable chance we can get in a near future version of WP.

#14 @adamsilverstein
6 months ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

@MikeSchinkel Thanks for the ping, I will review this for the 5.3 cycle. Do you feel the latest patch is sufficient, or would additional hooks help? In terms of the naming, what would you suggest for the hook names?

#15 @MikeSchinkel
6 months ago

@adamsilverstein Thanks for the reply.

After reviewing this and the ticket that this is somewhat a duplicate of I created a new ticket #47056 with a single goal of just getting one hook added that 3rd party plugins are already modifying the Walker class the patch suggested in #18584 and the one my new ticket asks for an up or down vote to include.

IOW, rather than get stalled on the perfect set of hooks I am hoping we can move forward with one we know has been used "in the wild" because there is a know need for it, and then deal with the others as edge cases when they come up?

Note: See TracTickets for help on using tickets.