Add short-circuit filter to `wp_setup_nav_menu_item`

[david.binda]

While looking into a memory consumption of navigation menus on some blogs with huge menus, I have noticed that to decorate a nav menu item via wp_setup_nav_menu_item function generates a lot of data.

To decorate a nav menu item of post_type type, for instance, requires the original object to be fetched in order to grab it's permalink and title only.

However, in case there's a menu consisting of multiple posts and/or pages with large content, the whole original object is much larger than the data which are actually needed.

IMHO, in case of a WordPress install with a persistent object cache backend, it would be beneficial to cache the fully decorated nav menu item (output of the wp_setup_nav_menu_item) in order to bring that from cache on subsequent requests instead of decorating the nav menu item over and over.

On a site which I've been investigating, the difference in between original objects fetched from memcache in order to decorate the nav menu item vs. obtaining the already decorated and cached nav menu item was 300M vs. 3.19 (yes, the nav menu is huge, consisting of ~1000 items, mostly posts).

However, I don't think that caching the decorated nav menu is something what core should do by default (eg.: it does not make sense w/o persistent object cache backend), so I wonder if it would make sense to add a short-circuit filter to the wp_setup_nav_menu_item function which would allow plugins to implement the caching layer.

[mukesh27]

Hi there!

The Patch need to update with document. Properly indent

 * @param object|null $modified_menu_item Modified menu item. Default null.
 * @param object      $menu_item          The menu item to modify.

[david.binda]

Hey @mukesh27 ! Thanks for pointing that out. I have fixed that in 56577.2.diff

[ironprogrammer]

Removing refresh keyword, as this was addressed with attachment:56577.2.diff​.

[ironprogrammer]

This filter has a well-defined use case; adding to 6.3 for consideration and further discussion.

It would be great if this patch also included testing steps and unit tests, so I've added those keywords as well.

[andizer]

Added unit tests

[andizer]

It has been a while since I wrote a unittest for WordPress, but I've made an attempt. Please let me know if I've done it the right way.

[azaozz]

56577.2.diff and 56577.unittests.patch look good imho (the second needs some coding standards fixes). Thinking this is ready.

[azaozz]

In 55867:

Menus: Add a short-circuit filter to wp_setup_nav_menu_item().

Props: davidbinda, ironprogrammer, andizer.
Fixes: #56577.

[TobiasBg]

Should the filter hook get a @since 6.3.0?

[SergeyBiryukov]

In 55868:

Docs: Add a @since tag for the pre_wp_setup_nav_menu_item filter.

Includes moving the unit test next to the other wp_setup_nav_menu_item() tests and using the MockAction class to confirm that the filter runs.

Follow-up to [55867].

Props TobiasBg.
Fixes #56577.

[azaozz]

Replying to SergeyBiryukov:

Thanks for adding the @since tag.

Not sure why the description was reverted though. It's not a big deal but generally using the same word (Returning) in different contexts in the same sentence is considered poor usage of the English language. Imho the edited description is better and clearer.

using the MockAction class to confirm that the filter runs.

Not sure why the unit test was refactored. Also not sure what is being tested now. The original test was testing the output of the wp_setup_nav_menu_item() function after the filter was used. The current test doesn't seem to be testing anything?

Think the unit test should be fixed to test the output of wp_setup_nav_menu_item() after the filter was used.

[SergeyBiryukov]

Replying to azaozz:

Not sure why the description was reverted though.

The description was adjusted for consistency with all other short-circuit filters in core. There are at least 20 instances of this pattern in core files:

Returning a ... value will (effectively) short-circuit the ..., returning that value instead.

Perhaps it's not ideal and could be improved further, but I find it helpful when documentation uses the same exact sentence structure in the same context.

Not sure why the unit test was refactored. Also not sure what is being tested now.

The test was also adjusted a bit for consistency with existing tests:

• Wordpress.org looks OK in test_orphan_nav_menu_item(), where it gets replaced with WordPress.org, but here it seemed unrelated to the purpose of the test and just looked weird to me :)
• Newer tests tend to use MockAction::get_call_count() or MockAction::get_args() as a consistent approach to ensure hooks are called in expected places with the expected parameters, instead of attaching callbacks with random data. In this case, I was following [55256] testing the pre_wp_load_alloptions filter, but this pattern also exists in many other tests.

The original test was testing the output of the wp_setup_nav_menu_item() function after the filter was used. The current test doesn't seem to be testing anything?

The current test ensures that wp_setup_nav_menu_item() applies the pre_wp_setup_nav_menu_item filter exactly once. I agree though that when testing the output specifically, it might be preferable to use a custom callback instead.

Think the unit test should be fixed to test the output of wp_setup_nav_menu_item() after the filter was used.

Fair enough, happy to fix that :)

[SergeyBiryukov]

56577.3.diff​ updates the test to verify the actual output of wp_setup_nav_menu_item() with the filter applied.

[azaozz]

Replying to SergeyBiryukov:

The description was adjusted for consistency with all other short-circuit filters in core. There are at least 20 instances of this pattern in core files:

Ahh, I see. Makes sense. Thinking we can look at updating/fixing the docs for all of these at some point, seems they can be a bit better.

56577.3.diff looks good imho, thanks for updating the test!

[ugyensupport]

Description

DesAdd short-circuit filter to wp_setup_nav_menu_item

Environment

• WordPress: 6.2.2
• PHP: 8.2.0
• Server: Apache/2.4.54 (Unix) mod_fastcgi/mod_fastcgi-SNAP-0910052141 OpenSSL/1.0.2u mod_wsgi/3.5 Python/2.7.18
• Database: mysqli (Server: 5.7.39 / Client: mysqlnd 8.2.0)
• Browser: Chrome 114.0.0.0 (macOS)
• Theme: Twenty Twenty-Three 1.1
• MU-Plugins: None activated
• Plugins:
  • WordPress Beta Tester 3.4.1

Steps to Reproduce

1. Test 56577.3.diff

Expected Results

1. ✅ https://core.trac.wordpress.org/attachment/ticket/56577/56577.3.diff Patch Fixes perfectly

[mukesh27]

Removing performance keyword as discuss in today's bug scrub.

[SergeyBiryukov]

In 55979:

Tests: Update the test for pre_wp_setup_nav_menu_item filter.

This ensures that not only is the filter applied in wp_setup_nav_menu_item(), but also the actual output is tested.

Follow-up to [55867], [55868].

Props azaozz, ugyensupport.
Fixes #56577.