Fixes the warning if null is given to prepare_items_query()

[apermo]

As discribed in the track ticket, it is possible that prepare_items_query() can receive a null value as $prepared_args which results in a warning. This fixes this issue.

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

AI disclosure:

AI used for commit message.

---

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 apermo, dmsnell, westonruter.

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.

[dmsnell]

I’ve checked the menu-items and attachment REST controllers and they call the posts controller, so it looks like this is comprehensive.

The purpose of the existing default value appears to be to make the code flow smoothly when no $prepared_args are sent, but this only triggers when the function is called without the argument, not when the argument itself is NULL.

To that end I could see two different ways of approaching this, both of which are rather subjective options and roughly equivalent to what you’ve proposed:

1. Combine all of the nullable/optional assignments inside the function.

function prepare_items_query( $prepared_args = null, $request = null ) {
	$prepared_args = $prepared_args ?? array();
...

2. Provide a second check inside the foreach

foreach ( $prepared_args ?? array() ...

The second option was my initial preference, but I don’t like that it splits the method’s understanding of the argument. Your if approach leaves that understanding in tact, but the first option, of combining the optional/nullable values, harmonizes what I suspect was the original intention behind this function.

Granted, as the existing type is written, the problem identified here is in the call sites of prepare_items_query(). Since these are protected methods, the other option for this PR would be to fix the bug where it appears, e.g.

$args       = apply_filters( "rest_{$this->post_type}_query", $args, $request );
$query_args = $this->prepare_items_query( $args ?? array(), $request );

This current fix removes the code which triggers the warning, but it leaves the type error in the codebase, the error being outside of this method and in the places that call it.

[dmsnell]

@westonruter what are your thoughts on this? I don’t love seeing us guard against type errors where the parameter types assert that those errors cannot happen, but this is getting filter output and people end up sending the wrong values anyway (a challenge with all this typing business)

we could change the $prepared_args type to ?array which accounts for probably all reasonable expectations here, but I tossed in 36530f0 to update the call sites and avoid passing any invalid data types (make sure that Core itself doesn’t propagate type errors deeper).

if we change the arg type it seems dishonest because the intent was always an array, but then if we were to do that we would end up with only a single type-check inside the function and callers need not worry about it. that would be a bit cleaner in my opinion.

protected function prepare_items_query( $prepared_args, $request = null ) {
    $prepared_args = is_array( $prepared_args ) ? $prepared_args : array();

otherwise we can merge this code. it’s protecting against an existing actual bug. either of the commits resolves the known cases. the first commit guards plugin code which might call this method directly, the second guards against plugin code which might return the wrong data type from the filter.

[dmsnell]

In order to see the tests pass I have merged trunk, which includes a fix for broken end-to-end tests.

[westonruter]

@dmsnell I'm glad to see this. I think your addition is the key one (which I iterated on a bit more in e54f258). The return value of apply_filters() cannot be trusted. If that along addresses the issue, it would seem fine to revert the change to the prepare_items_query methods, which I also amended in 35606cf + 15f956a to account for other values being passed in, for example an empty string.

With that in place, I'd ideally want to add the type hints to the methods and eliminate the default values, since the params are “always” provided:

diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php
index 720f122aed..83467201ba 100644
--- a/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php
+++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php
@@ -1208,15 +1208,12 @@ class WP_REST_Posts_Controller extends WP_REST_Controller {
 	 *
 	 * @since 4.7.0
 	 *
-	 * @param array           $prepared_args Optional. Prepared WP_Query arguments. Default empty array.
-	 * @param WP_REST_Request $request       Optional. Full details about the request.
+	 * @param array           $prepared_args Prepared WP_Query arguments. Default empty array.
+	 * @param WP_REST_Request $request       Full details about the request.
 	 * @return array Items query arguments.
 	 */
-	protected function prepare_items_query( $prepared_args = array(), $request = null ) {
+	protected function prepare_items_query( array $prepared_args, WP_REST_Request $request ): array {
 		$query_args = array();
-		if ( is_array( ! $prepared_args ) ) {
-			$prepared_args = array();
-		}
 
 		foreach ( $prepared_args as $key => $value ) {
 			/**
diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php
index cc483edacf..20a050a6e8 100644
--- a/src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php
+++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php
@@ -546,15 +546,12 @@ class WP_REST_Revisions_Controller extends WP_REST_Controller {
 	 *
 	 * @since 5.0.0
 	 *
-	 * @param array           $prepared_args Optional. Prepared WP_Query arguments. Default empty array.
-	 * @param WP_REST_Request $request       Optional. Full details about the request.
+	 * @param array           $prepared_args Prepared WP_Query arguments. Default empty array.
+	 * @param WP_REST_Request $request       Full details about the request.
 	 * @return array Items query arguments.
 	 */
-	protected function prepare_items_query( $prepared_args = array(), $request = null ) {
+	protected function prepare_items_query( array $prepared_args, WP_REST_Request $request ): array {
 		$query_args = array();
-		if ( is_array( ! $prepared_args ) ) {
-			$prepared_args = array();
-		}
 
 		foreach ( $prepared_args as $key => $value ) {
 			/** This filter is documented in wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php */

However, this does not account for subclasses which may be calling this protected method. So if we did that, we should do so early in a release cycle to ensure plugins which extend these classes are passing in the proper values as we've done in this PR with type checking the filter return values. But since a subclass may be also using a filter for the $prepared_args param being passed in to prepare_items_query() then it seems safest to add the type checking inside method as well since we didn't add the type hint in the first place back in 5.0.

[github-actions]

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

SVN changeset: 61773
GitHub commit: 105c7aa

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

[dmsnell]

Thanks @westonruter — I had a commit prepared but had been stalled by some flakey PHP Unit Tests and was trying to wait until they passed. Appreciate the teamwork.

[westonruter]

@dmsnell oh! Sorry if I stepped on your toes. 🤝