WordPress.org

Make WordPress Core

Opened 14 months ago

Closed 7 weeks ago

#23627 closed enhancement (fixed)

Allow plugins to short-circuit wp_nav_menu, good for performance

Reported by: kasparsd Owned by: helen
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.5.1
Component: Menus Keywords: has-patch
Focuses: template Cc:

Description

We already have short-circuit filters for widgets, options, links that all can be used for retrieving items from non-persistent data store. It would make a lot of sense if this feature was also available to wp_nav_menu which is particularly DB heavy.

My suggestion is to add pre_wp_nav_menu right after wp_nav_menu_args, like so:

	$args = wp_parse_args( $args, $defaults );
	$args = apply_filters( 'wp_nav_menu_args', $args );
	$args = (object) $args;

	// Allow plugins to short-circuit menu output
	$nav_menu = apply_filters( 'pre_wp_nav_menu', '', $args );

	if ( ! empty( $nav_menu ) && $args->echo )
		echo $nav_menu;
	elseif ( ! empty( $nav_menu ) )
		return $nav_menu;

	// Get the nav menu based on the requested menu
	$menu = wp_get_nav_menu_object( $args->menu );

Attachments (4)

add-pre_wp_nav_menu-filter.23627.diff (663 bytes) - added by kasparsd 14 months ago.
Patch
pre_wp_nav_menu-2.23627.diff (686 bytes) - added by kasparsd 2 months ago.
Use WP coding standards, return on echo.
pre_wp_nav_menu-3.23627.diff (857 bytes) - added by kasparsd 2 months ago.
Added documentation and used strict comparison for the short-circuit.
23627.diff (1.1 KB) - added by DrewAPicture 2 months ago.
hook docs tweaks

Download all attachments as: .zip

Change History (21)

comment:1 kasparsd14 months ago

  • Keywords has-patch added

comment:2 kasparsd14 months ago

Here is a link to wp_nav_menu.

comment:3 kasparsd14 months ago

Wrote a blog post explaining my reasoning behind this filter http://konstruktors.com/blog/wordpress/4594-filter-pre-wp-nav-menu/

comment:4 kasparsd2 months ago

  • Keywords dev-feedback added

comment:5 Rarst2 months ago

+1, must have - menus are slooow.

For those who need it right now, there is a roundabout way to make this happen by forcing core to see no menus at all, which triggers custom callback. See implementation in my Fragment Cache plugin https://bitbucket.org/Rarst/fragment-cache/src/tip/php/class-menu-cache.php

comment:6 nacin2 months ago

  • Focuses template added
  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to 3.9

Sounds good to me. Note that we always require braces around if statements now.

Rarst, anything we can do to speed up menus? I have not profiled this area of core in quite a while.

comment:7 Rarst2 months ago

Rarst, anything we can do to speed up menus?I have not profiled this area of core in quite a while.

For the scope of my plugin I just carved whole menu out into async generation, so hadn't paid much attention to what goes on inside.

If there is interest I will poke through it some time.

comment:8 kasparsd2 months ago

I just noticed that there is a bug in the patch -- it will continue executing the function even after echoing the filtered output. There should be a return after echo. I'll make a new patch that uses braces as suggested by @nacin.

kasparsd2 months ago

Use WP coding standards, return on echo.

comment:9 follow-up: nacin2 months ago

A few other things I'd suggest:

  • Returning false or '' should cause the function to short-circuit. Only returning null should allow execution to continue. (While I prefer this, core has historically reversed false and null. Either works.)
  • The filter will need documentation.

kasparsd2 months ago

Added documentation and used strict comparison for the short-circuit.

comment:10 in reply to: ↑ 9 kasparsd2 months ago

Replying to nacin:

A few other things I'd suggest:

Thanks for the suggestions, @nacin!

I have added a modified patch. Not sure how to describe the default filter argument/value null in the PHPDoc.

DrewAPicture2 months ago

hook docs tweaks

comment:11 follow-up: DrewAPicture2 months ago

Replying to kasparsd:

Replying to nacin:

A few other things I'd suggest:

Thanks for the suggestions, @nacin!

I have added a modified patch. Not sure how to describe the default filter argument/value null in the PHPDoc.

For the null, I usually try to just describe what the value basically is. And you can just use a docs-specific variable, maybe something like $output, e.g.:

* @param string|null $output Nav menu output to short-circuit with. Default null.

I've made that adjustment and others to the hook doc in 23627.diff.

Notably, in the description I avoided describing why it's filterable, rather describing what's filterable. Typically we'll use a long description to outline the *why* or reasoning if it's unclear in the short description -- which I added in the patch.

I also moved the @see tag out of an in-line parameter description.

comment:12 in reply to: ↑ 11 kasparsd2 months ago

Replying to DrewAPicture:

Looks great, Drew! Thank you very much.

comment:13 follow-up: Rarst2 months ago

Nitpicking on patch - why check for null twice? Also yoda condition.

if ( null !== $nav_menu  ) {

	if ( $args->echo ) {
		echo $nav_menu;
		return;
	}

	return $nav_menu;
}

comment:14 in reply to: ↑ 13 kasparsd2 months ago

Replying to Rarst:

Nitpicking on patch - why check for null twice? Also yoda condition.

I personally find the single level indentation more readable. Agreed about Yoda.

comment:15 ircbot7 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.

comment:16 nacin7 weeks ago

I agree with Rarst, as does helen (see IRC). She's handling commit.

comment:17 helen7 weeks ago

  • Owner set to helen
  • Resolution set to fixed
  • Status changed from new to closed

In 27386:

Add the ability to short-circuit wp_nav_menu() via the pre_wp_nav_menu hook. props kasparsd, DrewAPicture, Rarst. fixes #23627.

Note: See TracTickets for help on using tickets.