WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 9 months ago

Last modified 3 months ago

#41457 closed defect (bug) (fixed)

WP_REST_Comments_Controller::check_read_post_permission() counts an uncountable value

Reported by: johnbillion Owned by: johnbillion
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.7
Component: Comments Keywords: needs-unit-tests has-patch needs-testing
Focuses: rest-api Cc:

Description

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.

Attachments (1)

41457-src.patch (979 bytes) - added by ayeshrajans 11 months ago.
src changes

Download all attachments as: .zip

Change History (12)

#1 @ayeshrajans
11 months ago

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

I'll try to see what I can do, and may be submit a patch with additional tests.

Last edited 11 months ago by ayeshrajans (previous) (diff)

@ayeshrajans
11 months ago

src changes

#2 @ayeshrajans
11 months ago

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).

Last edited 11 months ago by ayeshrajans (previous) (diff)

#3 @johnbillion
11 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

Thanks for looking into this, @ayeshrajans!

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

This ticket was mentioned in Slack in #core-restapi by johnbillion. View the logs.


11 months ago

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


11 months ago

#6 @johnbillion
11 months ago

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.

This ticket was mentioned in Slack in #core-restapi by ayesh. View the logs.


11 months ago

#8 @schlessera
9 months ago

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

#9 @kadamwhite
9 months ago

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.

#10 @johnbillion
9 months ago

  • Owner set to johnbillion
  • Resolution set to fixed
  • Status changed from new to closed

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

#11 @ocean90
3 months ago

#40741 was marked as a duplicate.

Note: See TracTickets for help on using tickets.