Make WordPress Core

Opened 4 months ago

Last modified 4 months ago

#42469 new defect (bug)

WP_Query found_posts inconsistent data type.

Reported by: PressLabs Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Query Keywords: dev-feedback has-unit-tests has-patch
Focuses: Cc:


Attachments (5)

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

Download all attachments as: .zip

Change History (10)

4 months ago

4 months ago

#1 @PressLabs
4 months ago

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

#2 @birgire
4 months 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.

4 months ago

#3 @PressLabs
4 months 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?

4 months ago

4 months ago

#4 @birgire
4 months 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
4 months ago

  • Version trunk deleted
Note: See TracTickets for help on using tickets.