Make WordPress Core

Opened 7 months ago

Last modified 2 months ago

#59390 new defect (bug)

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

Reported by: davidbinda's profile david.binda Owned by:
Milestone: 6.6 Priority: normal
Severity: normal Version: 5.9
Component: Menus Keywords: has-patch changes-requested
Focuses: rest-api Cc:

Description

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.

Attachments (1)

59390.diff (702 bytes) - added by david.binda 7 months ago.

Download all attachments as: .zip

Change History (11)

@david.binda
7 months ago

#1 @SergeyBiryukov
7 months ago

  • Component changed from General to Menus
  • Focuses rest-api added
  • Milestone changed from Awaiting Review to 6.4
  • Version set to 5.9

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

#2 @mukesh27
7 months ago

  • Keywords has-patch added

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

Last edited 7 months ago by mukesh27 (previous) (diff)

#3 @nicolefurlan
6 months ago

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.

#4 @nicolefurlan
6 months ago

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?

This ticket was mentioned in Slack in #core by oglekler. View the logs.


6 months ago

#6 @oglekler
6 months ago

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

#7 @SergeyBiryukov
6 months ago

  • Keywords changes-requested added

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.

#8 @nicolefurlan
6 months ago

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.

#9 @oglekler
6 months ago

  • Milestone changed from 6.4 to 6.5

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

#10 @swissspidy
2 months ago

  • Milestone changed from 6.5 to 6.6
Note: See TracTickets for help on using tickets.