Make WordPress Core

Opened 20 months ago

Closed 10 months ago

Last modified 10 months ago

#56577 closed enhancement (fixed)

Add short-circuit filter to `wp_setup_nav_menu_item`

Reported by: davidbinda's profile david.binda Owned by: azaozz's profile azaozz
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: Menus Keywords: needs-testing-info has-patch commit add-to-field-guide
Focuses: Cc:

Description

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.

Attachments (4)

56577.diff (1010 bytes) - added by david.binda 20 months ago.
56577.2.diff (1019 bytes) - added by david.binda 20 months ago.
56577.unittests.patch (1.3 KB) - added by andizer 11 months ago.
Added unit tests
56577.3.diff (688 bytes) - added by SergeyBiryukov 10 months ago.

Download all attachments as: .zip

Change History (23)

@david.binda
20 months ago

#1 @mukesh27
20 months ago

  • Keywords needs-refresh added

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.

#2 @david.binda
20 months ago

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

Last edited 20 months ago by david.binda (previous) (diff)

#3 @krupalpanchal
20 months ago

  • Keywords has-patch added

#4 @ironprogrammer
12 months ago

  • Keywords needs-refresh removed

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

#5 @ironprogrammer
12 months ago

  • Keywords needs-unit-tests needs-testing-info added
  • Milestone changed from Awaiting Review to 6.3

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
11 months ago

Added unit tests

#6 @andizer
11 months ago

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.

#7 @azaozz
11 months ago

  • Keywords needs-unit-tests removed

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

#8 @azaozz
11 months ago

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

In 55867:

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

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

#9 @TobiasBg
11 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Should the filter hook get a @since 6.3.0?

#10 follow-up: @SergeyBiryukov
11 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#11 in reply to: ↑ 10 ; follow-up: @azaozz
11 months ago

  • Keywords needs-patch 2nd-opinion added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#12 in reply to: ↑ 11 ; follow-up: @SergeyBiryukov
10 months ago

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 :)

#13 @SergeyBiryukov
10 months ago

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

#14 @SergeyBiryukov
10 months ago

  • Keywords has-patch added; needs-patch removed

#15 in reply to: ↑ 12 @azaozz
10 months ago

  • Keywords commit added; 2nd-opinion removed

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!

#16 @ugyensupport
10 months ago

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

#17 @mukesh27
10 months ago

  • Focuses performance removed

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

#18 @SergeyBiryukov
10 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#19 @stevenlinx
10 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.