Fix: Validate rest_route Query Var to Ensure String Type

[geekofshire]

This PR introduces an early return validation in rest_api_loaded( to ensure that the rest_route query variable is always a string. If the rest_route is not a string, the function now returns a WP_Error, allowing for better error handling and maintaining proper REST API response behaviour.

Changes:

• Added an early return if rest_route is not a string.
• Replaced wp_die() with a WP_Error response to improve flexibility.

Trac ticket: https://core.trac.wordpress.org/ticket/62932

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props geekofshire, peterwilsoncc.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[peterwilsoncc]

I've added a note inline.

It would be helpful to add a unit test to ensure wp_die() is called if the route is an array. For testing this patch, I used the following.

/**
 * @ticket 62932
 */
public function test_rest_route_throws_wp_die_if_passed_an_array() {
	$this->expectException( 'WPDieException' );
	$GLOBALS['wp']->query_vars['rest_route'] = array( 'foo' => 'bar' );
	rest_api_loaded();
}

If you haven't got experience with adding unit tests, I'm happy to provide assistance.

[geekofshire]

@peterwilsoncc I have added the unit-test in f14f658. Let me know if that looks good or needs some change

Edit: Not sure why majority of tests are failing

[peterwilsoncc]

I'm not sure why but this is causing seemingly unrelated tests to fail, so I will need to take a closer look at it. I hope to do so in the next week or so.

[peterwilsoncc]

@geekofshire I think I've figured it out and it's on me, sorry 🫢

Calling rest_api_loaded() in the test suite is hitting the line define( 'REST_REQUEST', true ); which will affect the rest of the test suite. I suspect the failed tests are interacting with the constant in some way (or the functions they call).

Unfortunately that makes rest_api_loaded() largely untestable (@TimothyBJacobs can you confirm) so we might need to rely on manual testing of the code.

[geekofshire]

Hello, @peterwilsoncc Do we have any update on this on testing this change and since we need to manually test this out should I remove the unit test? Thank You!

[peterwilsoncc]

@geekofshire I think there are two options:

• remove the test
• move the logic check triggering wp_die() above define( 'REST_REQUEST', true );

I'm happy with either option, which do you prefer?

[geekofshire]

@peterwilsoncc I think we should move the logic above define( 'REST_REQUEST', true ) because if the rest_route query var isn't a string, there's no need to define REST_REQUEST and initialize the server. The process should stop immediately.

Let me know if this sounds good so I can update the PR.

[peterwilsoncc]

@geekofshire That sounds good, thanks.

[geekofshire]

@peterwilsoncc Updated the approach please check once, the 2 tests are failing due to some connection issues/server config issue.

[peterwilsoncc]

This looks great, thanks.

Just one more note about making the test clearer. If you are happy with that change, I am happy to commit it.

[github-actions]

A commit was made that fixes the Trac ticket referenced in the description of this pull request.

SVN changeset: 59886
GitHub commit: e7ce9bb

This PR will be closed, but please confirm the accuracy of this and reopen if there is more work to be done.