Incorrect method / function args used in bin/tests/core/tests/rest-api/wpRestMenuItemsController.php

[david.binda]

The Tests_REST_WpRestMenuItemsController::check_get_menu_item_response method accepts 2 arguments, however 3 are passed to the method in https://core.trac.wordpress.org/browser/trunk/tests/phpunit/tests/rest-api/wpRestMenuItemsController.php?rev=56549#L885

It feels like the Tests_REST_WpRestMenuItemsController::check_menu_item_data method was meant to be used here.

[SergeyBiryukov]

Good catch, introduced in [52079] / #40878.

[mukesh27]

@SergeyBiryukov, you beat me to it! I was just about to add the ticket and changeset entry for when it was introduced. 😅

[nicolefurlan]

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://core.trac.wordpress.org/attachment/ticket/59390/59390.diff

Environment

• OS: macOS 13.4.1
• PHP: 8.2.11
• WordPress: trunk

Actual Results

• ✅ Issue resolved with patch.

Additional Notes

• PHPUnit tests continue to pass after applying the patch.

[nicolefurlan]

With just over a week left until 6.4 RC1, do we think this ticket will make it? I submitted a passing test report last week – do we think this ticket is ready for commit?

[oglekler]

@audrasjb you are this component maintainer, please take a look, it should be ready for commit.

[SergeyBiryukov]

Thanks for the patch!

I'm a bit confused here. The Tests_REST_WpRestMenuItemsController::check_menu_item_data() method requires four arguments, so unless I'm missing something, passing only three would cause a fatal error, and the test only passes accidentally because that branch is never hit.

This could use some more investigation.

[nicolefurlan]

I think you're absolutely right @SergeyBiryukov.

@davidbinda could you take a look at https://core.trac.wordpress.org/ticket/59390#comment:7?

@audrasjb pinging you as well as the component maintainer to see if you have any input on this.

[oglekler]

Better safe than sorry, I am moving this to the next milestone to get everyone time to check the patch properly.

[hellofromTonya]

Hmm, this one is not a straightforward swap. It will need investigation to go back into the PR/patch history to determine the intent of the parent check.

I've added to my TODO list. Moving it out of the 6.6 milestone, for now, as fixing a test is not bound to a milestone. As soon as there's a clear understanding, the fix can be committed.