WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 3 weeks ago

#42860 closed defect (bug) (fixed)

PHP 7.2 warning - Parameter must be an array or an object that implements Countable in /wp-includes/class-wp-query.php on line 3035

Reported by: lisota Owned by: jorbin
Milestone: 4.9.3 Priority: normal
Severity: normal Version: trunk
Component: Query Keywords: fixed-major
Focuses: Cc:

Description

Upgrading to PHP 7.2 results in the following PHP warning from class-wp-query.php

PHP Warning: count(): Parameter must be an array or an object that implements Countable in /wp-includes/class-wp-query.php on line 3035

Attachments (4)

42860-1.patch (555 bytes) - added by ayeshrajans 2 months ago.
Tests: https://travis-ci.org/Ayesh/wordpress-develop/builds/314423062
42860.patch (531 bytes) - added by janak007 4 weeks ago.
Added array check and return the post counts
42860-1.2.patch (539 bytes) - added by janak007 4 weeks ago.
Added array check and return the post counts
42860.diff (1.9 KB) - added by jorbin 3 weeks ago.

Download all attachments as: .zip

Change History (28)

#1 @tw2113
2 months ago

For what it's worth, the line in question is

$this->found_posts = count( $this->posts ); inside function set_found_posts()

#2 @johnbillion
2 months ago

  • Keywords reporter-feedback added

Thanks for the report.

  • Are you able to determine the source of the error (ie. where the query is called from)?
  • Does it still occur with all your plugins deactivated?
  • What's the value of $this->posts at this point?

Thanks

#3 @johnbillion
2 months ago

  • Milestone changed from Awaiting Review to 4.9.2

#4 @ayeshrajans
2 months ago

It looks like the $this->posts variable was left default null, causing the warning when called with count().
Attached a patch that might solve it, but we should really be waiting for the reporter's information. A backtrace can explain a lot.

#6 follow-up: @dd32
2 months ago

After looking over this, I can only assume that there's a caching plugin in play which has set $this->request to null or '' which causes $this->posts to be set to the same thing instead of an array as expected.

The correct fix here is probably to ensure that $this->posts is always set to an array, even when $this->request is emptied out.

#7 in reply to: ↑ 6 @SergeyBiryukov
5 weeks ago

Replying to dd32:

The correct fix here is probably to ensure that $this->posts is always set to an array, even when $this->request is emptied out.

The comment in WP_Query::set_found_posts() specifically allows for $this->posts to not be an array though:

// Bail if posts is an empty array. Continue if posts is an empty string,
// null, or false to accommodate caching plugins that fill posts later.

I think we should just check if is_array() before calling count().

#8 @lisota
5 weeks ago

  • Version changed from 4.9.1 to trunk

Let me see if I can grab a backtrace. I rolled out PHP 7.2 on our production site, and experienced a number of errors related to 7.2, including this one, and promptly rolled back.

#9 @dd32
5 weeks ago

  • Milestone changed from 4.9.2 to 4.9.3

Bumping to 4.9.3 due to 4.9.2s release

#10 @josephscott
4 weeks ago

I agree with doing an array check first, could be something very simple:

            $this->found_posts = 0;
            if ( is_array( $this->posts ) ) {
                $this->found_posts = count( $this->posts );
            }

@janak007
4 weeks ago

Added array check and return the post counts

@janak007
4 weeks ago

Added array check and return the post counts

This ticket was mentioned in Slack in #core by desrosj. View the logs.


4 weeks ago

#12 @desrosj
4 weeks ago

  • Keywords needs-unit-tests added

#13 @jorbin
4 weeks ago

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

In 42581:

Query: Fix warning on counting non countable

Adds tests to continue the behavior for both null and strings.

See https://wiki.php.net/rfc/counting_non_countables for information on the PHP change.

Fixes #42860.
Props janak007 and ayeshrajans for initial patches.

#14 @jorbin
4 weeks ago

  • Keywords fixed-major added; reporter-feedback needs-unit-tests removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

#15 @dd32
4 weeks ago

In 42585:

Tests: Skip newly added tests from [42581] on PHP 5.2 as they require ReflectionMethod.

See #42860.

#16 follow-up: @SergeyBiryukov
3 weeks ago

  • Keywords needs-patch added; fixed-major removed

[42581] doesn't look complete to me. Per comment:7, an empty string or false should be treated the same as null.

#17 @SergeyBiryukov
3 weeks ago

To clarify a bit, [42581] does keep the previous behavior, I'm just not sure it's correct. If $this->posts is false or an empty string, I'd expect found_posts to be 0.

#18 in reply to: ↑ 16 ; follow-up: @jorbin
3 weeks ago

Replying to SergeyBiryukov:

[42581] doesn't look complete to me. Per comment:7, an empty string or false should be treated the same as null.

That's not how count works. It would be a breaking change to set it as 0 in those instances.

From http://php.net/manual/en/function.count.php#refsect1-function.count-returnvalues

Returns the number of elements in array_or_countable. When the parameter is neither an array nor an object with implemented Countable interface, 1 will be returned. There is one exception, if array_or_countable is NULL, 0 will be returned.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


3 weeks ago

@jorbin
3 weeks ago

#20 @jorbin
3 weeks ago

The above patch reorganizes the tests to include an empty string and false

#21 in reply to: ↑ 18 @SergeyBiryukov
3 weeks ago

  • Keywords fixed-major added; needs-patch removed

Replying to jorbin:

It would be a breaking change to set it as 0 in those instances.

Right, on second thought I agree breaking back compat is not worth it. Let's add 42860.diff then.

#22 @jorbin
3 weeks ago

In 42594:

Query: Improve tests for set_found_posts dealing with non arrays

Use a data provider and include tests for false and ''.

Previous: [42581] [42585]

See #42860

This ticket was mentioned in Slack in #core by jorbin. View the logs.


3 weeks ago

#24 @jorbin
3 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 42597:

Query: Fix warning on counting non countable

Merges [42581], [42585], and [42594] to the 4.9 branch.

Adds tests to continue the behavior for both null and strings. Skip the tests on PHP 5.2 as they require ReflectionMethod.

See https://wiki.php.net/rfc/counting_non_countables for information on the PHP change.

Fixes #42860.
Props dd32 for test skipping and janak007 and ayeshrajans for initial patches.

Note: See TracTickets for help on using tickets.