WP_REST_Comments_Controller::check_read_post_permission() counts an uncountable value

[johnbillion]

From #40109.

In WP_REST_Comments_Controller::check_read_post_permission(), count() is called on $request['post'], but the value of this variable isn't always countable. This causes a warning in PHP 7.2.

From some testing, $request['post'] can be an integer, an empty array, an array of integers, or null. All of these types need to be accounted for in this method's logic.

[ayeshrajans]

To replicate the count($request['post']), here are the variable types that return (int) 1 for count($request['post']) calls:

• true
• false
• 'foobar' (any string)
• array(1)
• any object. Even an empty stdClass object returns int 1 for count() calls.

Ref: ​https://3v4l.org/1mkY5

[ayeshrajans]

src changes

[ayeshrajans]

Please see the patch above.
The !empty($request['post']) call there prevents false-y values from continuing. false, null, empty strings, and empty arrays should be thrown off here. This leaves us with the post ID either being a single string/int value, or an array of values. Earlier, count() returned 1 for both count(12345) and count(array(12345)). Now, because the !empty() call has thrown off false-y values, we only need to check there is one array element only if the $request['post'] value is an array.

--

The warnings thrown from this particular count() call is count(): Parameter must be an array or an object that implements Countable.
Build log: ​https://travis-ci.org/WordPress/wordpress-develop/jobs/258395218 (find for /home/travis/build/WordPress/wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-comments-controller.php:1514). There are 30 instances of it.

The patch above fixes those issues.
Build log: ​https://travis-ci.org/Ayesh/wordpress-develop/jobs/258563149
The build still fails because of the other errors, but all the 39 errors reported for this count() call are gone.
As for the unit tests, \WP_Test_REST_Comments_Controller::test_get_items_with_password_with_multiple_post covers a case of multiple post ID REST call with a password. I'm not sure if we can write a more meaningful test, but I think we can check if Boolean, arrays with wrong-password post IDs, and other misc variable types work (it should throw a 401 or 400 error).

[johnbillion]

Thanks for looking into this, @ayeshrajans!

I'll leave this with one of the REST API component maintainers to review.

[johnbillion]

For clarification: This method currently causes the PHP 7.2 build to fail due to the use of count() with an uncountable value. We want to avoid counting an uncountable value so PHP 7.2 doesn't throw an error.

What I think we need is somebody to say, yes, we have all of the possible types for this variable covered in the tests so we can apply the patch from @ayeshrajans.

[schlessera]

According to ​http://phpcheatsheets.com/test/array_tests/ , we get the following rules for the result of count():

• is_array() => count()
• null => 0,
• <everything else> => 1

[kadamwhite]

This looks good from our point of view, I got stalled trying to implement tests but the patch works and covers the values I'm aware of.

[johnbillion]

In 41735:

REST API: Avoid counting an uncountable type when checking read permissions for comment posts.

This avoids deprecated notices from showing in PHP 7.2 and above.

Props ayeshrajans
Fixes #41457

[ocean90]

#40741 was marked as a duplicate.