[wp-trac] [WordPress Trac] #56577: Add short-circuit filter to `wp_setup_nav_menu_item`

[WordPress Trac]

#56577: Add short-circuit filter to `wp_setup_nav_menu_item`
-------------------------------------------------+-------------------------
 Reporter:  david.binda                          |       Owner:  azaozz
     Type:  enhancement                          |      Status:  reopened
 Priority:  normal                               |   Milestone:  6.3
Component:  Menus                                |     Version:
 Severity:  normal                               |  Resolution:
 Keywords:  needs-testing-info needs-patch 2nd-  |     Focuses:
  opinion                                        |  performance
-------------------------------------------------+-------------------------

Comment (by SergeyBiryukov):

 Replying to [comment:11 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 :)

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/56577#comment:12>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform