Add object cache to wp_nav_menu() items

[Clorith]

Leveraging $menu_items in the wp_nav_menu() function call with the Object Cache shows a drop in queries by 2 for each consecutive call for a specific theme_location

Might need a bit more intense testing to ensure it doesn't negatively impact anything else using the function, but from a front facing side of things it has s ofar not showed any negative effect in my tests.

[Clorith]

There seems to be an object cache for the default fallback, so if your theme_location isn't found, when nobody picked a menu location in the admin-ui, caching occurs as it should.

If someone has defined the menu location and it matches theme_location, there is no caching done pre-patch.

The two queries being ran with each iteration of wp_nav_menu() as mentioned;

SELECT
	tr.object_id 
FROM 
	wp_term_relationships AS tr 
INNER JOIN 
	wp_term_taxonomy AS tt 
		ON tr.term_taxonomy_id = tt.term_taxonomy_id 
WHERE 
	tt.taxonomy IN ('nav_menu') 
AND 
	tt.term_id IN ('2') 
ORDER BY 
	tr.object_id ASC

and

SELECT 
	wp_posts.* 
FROM 
	wp_posts 
WHERE 
	1=1 
AND 
	wp_posts.ID IN (35,36,38,39,50,51,118,130,131,132,133,134) 
AND 
	wp_posts.post_type = 'nav_menu_item' 
AND 
	((wp_posts.post_status = 'publish')) 
ORDER BY 
	wp_posts.menu_order ASC 

[SergeyBiryukov]

Please use tabs rather than spaces for indentation (per ​WordPress Coding Standards).

Do we need to clear this cache somewhere in case of using a persistent object cache?

[Clorith]

Fixed indentations to follow coding standard

[nacin]

I'm not entirely sure I see the bug here. Or rather, I'm not sure the solution is the best way to reduce queries.

The queries I see are in wp_get_nav_menu_object(), which uses get_term() or get_term_by() (one of which is cached, the other will be in #21760); and in wp_get_nav_menu_items(), which uses get_objects_in_term(), which could be switched to a higher-level function that performs caching first.

get_posts() and get_terms() have their own caches to some extent. The get_posts() call could be moved to a _prime_post_caches() call instead.

I don't think we need to blindly cache menus; deeper improvements could go a long way. (As Sergey said, these would changes would require cache invalidation.)

[chriscct7]

Closing as maybelater given the lack of interest over the past 2 years

[soulseekah]

Implementation + tests

[soulseekah]

Added delete invalidation tests

[soulseekah]

Maybe now? :)

@campusboy1987 pointed out that the same wp_nav_menu() calls (for example in twentysixteen header.php and footer.php) generate a duplicate query.

With help from @versusbassz @campusboy1987 @denisco this patch was put together.

1. Added an item cache around the get_posts() call inside wp_get_nav_menu_items().
2. Invalidation is inside wp_update_nav_menu_item()
3. Some tests, including invalidation.

Thanks :)

[pento]

This ticket needs reviewing and a decision.

[mukesh27]

@spacedmonkey did a lot of object cache related work for 6.1. Do you have time available for this ticket? Or did you get a chance to work on this ticket? If you have some time to review this ticket, please have a look at it. If you think I can be of any help, please let me know.

cc. @peterwilsoncc.

[johnbillion]

This ticket crossed my mind last week because the wp_cache_flush_group() function is now in place. It facilitates caching nav menus separately for each URL in a group that can now be flushed easily when necessary (as long as the cache driver supports it).

However I think the main blocker for doing this in core is handling dynamic menu items and handling filters on the nav menu hooks that can adjust the output. It's fine on a client site where you know there is no dynamism in the nav menus, but it can't be guaranteed otherwise.

[spacedmonkey]

A lot of work has been done on caching and caching priming in 6.0/6.1. I am not sure that this ticket is valid anymore. Taxonomy and posts queries are cached in object caching now. This was fixed upstream. @Clorith and @mukesh27 can you do some profiling against trunk see if this ticket is valid?

[spacedmonkey]

I would also recommend looking into - ​https://github.com/spacedmonkey/advanced-nav-cache

[spacedmonkey]

@Clorith Can you confirm that this ticket is still valid? Caching was added to both WP_Query and WP_Term_Query, so it is likely there lower level caches will cover this.

I believe we need mark this as #invalid, but I want to hear from others on this ticket.

[Clorith]

Oh how I wish I was better at making tickets back in the day, I can only apologize for the lack of clarity and information in this one.

I do agree that the underlying cache solutions are probably a better approach now though, and with the improvements done there recently, I can't immediately replicate my original thoughts at least, so I think we could probably close this ticket, and if someone can recall what I was thinking in my days of youth, and it turns out this isn't fully resolved, we can always reopen it later.

[spacedmonkey]

This ticket now seems invalid, I am plan to close it out, if anyone disagree, let me know.

[spacedmonkey]

#36225 was marked as a duplicate.

[mukesh27]

As per the discussion at https://core.trac.wordpress.org/ticket/27120#comment:22, no one disagrees. Therefore, I am going to close this. Thank you!