Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#41457 closed defect (bug) (fixed)

WP_REST_Comments_Controller::check_read_post_permission() counts an uncountable value

Reported by: johnbillion's profile johnbillion Owned by: johnbillion's profile 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 7 years ago.
src changes

Download all attachments as: .zip

Change History (12)

#1 @ayeshrajans
7 years 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 7 years ago by ayeshrajans (previous) (diff)

@ayeshrajans
7 years ago

src changes

#2 @ayeshrajans
7 years 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 7 years ago by ayeshrajans (previous) (diff)

#3 @johnbillion
7 years 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.


7 years ago

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


7 years ago

#6 @johnbillion
7 years 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.


7 years ago

#8 @schlessera
7 years 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
7 years 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
7 years 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
7 years ago

#40741 was marked as a duplicate.

Note: See TracTickets for help on using tickets.