Make WordPress Core

Opened 7 years ago

Closed 4 years ago

Last modified 4 years ago

#42469 closed defect (bug) (fixed)

WP_Query found_posts inconsistent data type.

Reported by: presslabs's profile PressLabs Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: Query Keywords: has-unit-tests has-patch has-dev-note
Focuses: Cc:


Attachments (5)

42130.patch (746 bytes) - added by PressLabs 7 years ago.
42469.patch (746 bytes) - added by PressLabs 7 years ago.
42469.1.patch (789 bytes) - added by PressLabs 7 years ago.
42469.2.diff (1.5 KB) - added by birgire 7 years ago.
42469.3.diff (2.9 KB) - added by birgire 7 years ago.

Download all attachments as: .zip

Change History (13)

7 years ago

7 years ago

#1 @PressLabs
7 years ago

You can remove the patch that has the mismatching ticket number, my bad.

#2 @birgire
7 years ago

Thanks for the patch @PressLabs.

The database query returns a string, so this seems a valid concern.

I checked the other query classes and they also convert it to integer.

In WP_Comment_Query we see:

$found_comments_query = apply_filters( 'found_comments_query', 'SELECT FOUND_ROWS()', $this );
$this->found_comments = (int) $wpdb->get_var( $found_comments_query );

and in WP_Site_Query we have:

$found_sites_query = apply_filters( 'found_sites_query', 'SELECT FOUND_ROWS()', $this );
$this->found_sites = (int) $wpdb->get_var( $found_sites_query );

Similarly in WP_User_Query:

$this->total_users = (int) $wpdb->get_var( apply_filters( 'found_users_query', 'SELECT FOUND_ROWS()' ) );

I would suggest making it more readable and change:

$this->found_posts = intval( $wpdb->get_var( apply_filters_ref_array( 'found_posts_query', array( 'SELECT FOUND_ROWS()', &$this ) ) ) );


$found_posts_query = apply_filters_ref_array( 'found_posts_query', array( 'SELECT FOUND_ROWS()', &$this ) );

$this->found_posts = (int) $wpdb->get_var( $found_posts_query );

in WP_Query.

7 years ago

#3 @PressLabs
7 years ago

I'm unsure what the stance is on the matter, but do you in core enforce type casting on apply_filter or is it too defensive and out of scope.

I'm asking because for me it would be reasonable if I got an int I should return an int but in the PHP world things grow hazy on the matter.

Should I cast also the found_posts filter a couple of lines below, as well?

7 years ago

7 years ago

#4 @birgire
7 years ago

  • Keywords dev-feedback has-unit-tests has-patch added

I added a type test in 42469.2.diff and removed some extra tabs.

There are some cases in core with (int) apply_filters( ... ); so I don't think that's a problem with that structure. It sounds logical to convert the output from the found_posts filter to integer as well, as the definition of the found_posts property says it's of type int:

 * The amount of found posts for the current query.
 * If limit clause was not used, equals $post_count.
 * @since 2.1.0
 * @var int
public $found_posts = 0;

I added the int cast on the found_posts filter in 42469.3.diff, with an extra test.

But the general concern is if anyone out there is assuming that found_posts is a string, for example:

if( '1' === $query->found_posts ) {
   // ...

So I tagged the ticket with dev-feedback.

#5 @ocean90
7 years ago

  • Version trunk deleted

#6 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.5
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#7 @SergeyBiryukov
4 years ago

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

In 48328:

Query: Make sure the found_posts property of WP_Query is always an integer, to match the documented type.

This makes the property consistent with similar properties of other classes:

  • WP_Comment_Query::$found_comments
  • WP_Network_Query::$found_networks
  • WP_Site_Query::$found_sites
  • WP_User_Query::$total_users

Props birgire, PressLabs.
Fixes #42469.

#8 @desrosj
4 years ago

  • Keywords has-dev-note added; dev-feedback removed

This received a call out in the Miscellaneous Developer Changes in 5.5 dev note:

Note: See TracTickets for help on using tickets.