Wrong data type for several variables holding a wpdb query result

[tfrommen]

Just like in #32876, there are several situations where the result of $wpdb->get_var() is used as is (which is a string), and yet data type is said to be int. The individual situations include apply_filters() calls, function return values, and class properties, i.e., only non-local usage.

Please find the attached patch that takes care of that.
In one case I also adapted two conditionals using a former-string/null-now-int-value.

[boonebgorges]

tfrommen - Thanks for the patch. Some of these changes look more dangerous than others. For example, count_user_posts() will currently return '0' when zero records are found and null when there's an error. It's not too hard to imagine someone doing a strict check on the return value in order to test whether an error occurred. So this is a case where the logic should probably be something like:

$count = $wpdb->get_var( ... );
if ( ! is_null( $count ) ) {
    $count = intval( $count );
}

Even this has the potential to break a plugin that checks if ( '0' === count_user_posts( ... ) ), which again, is not that unreasonable given the fact that null is semantically different in this case.

I'm not opposed to making these kinds of changes in general, but I'm only comfortable handling them individually, with some thought given to the possibility of breakage in each case. In some cases, this might involve searching the plugin repo etc for potentially problematic use cases. Ideally, we'd also have unit tests for any change :-D

[tfrommen]

I would like to ask for how to proceed with this ticket.

I understand

1. handling the proposed changes individually is the way to go;
2. almost every change might break some specific plugin (or theme) doing strict comparisons (with a type that shouldn't even be there, though!).

But what are we going to do here?

Would you like individual tickets? Or would individual patches in this ticket be good already?

As to the possible breaking plugin/theme functionality. I guess @boonebgorges (or someone else) will slurp the plugins and check for usage of the changed action/filter parameters and function return values.
Once again, if a function currently returns string|null, while its PHPDoc states it would return int instead, of course, strict comparisons with both string and null will break. On the other hand, no one should actually be doing this (in the case with int being included in the annotation).

In the case where the annotation says int|null and the passed type is string|null instead, we should keep the null return, while casting the string to an int.

What are yout thoughts? Can I be of any help here?

Also, I don't really understand what (kind of) unit tests you would like to have here... My proposed changes are about data types. So would you want a couple of $this->assertTrue( is_int( some_func() ) );, or what do you have in mind?

[boonebgorges]

Or would individual patches in this ticket be good already?

Individual patches on this ticket should be fine.

I guess @boonebgorges (or someone else) will slurp the plugins and check for usage of the changed action/filter parameters and function return values.

Or you! I should note that this is not an exact science, and I understand that collateral damage may be unavoidable. The purpose of the plugin search is to get a sense - often, an informed intuition - about whether the amount of potential breakage is acceptable, given the gains.

The reasoning, in each case, should work like this:

• Is anyone actually doing strict comparison here? If no one is, then we can feel more comfortable making the change.
• In cases where we break strict checks, how bad will the break be? Some functions are likely to be used in mission-critical ways, while others are just for display, etc. If there's a possibility that we could, say, introduce a security vulnerability because of a type change, then we should probably not make the change.

I don't really understand what (kind of) unit tests you would like to have here.

I want tests that accurately describe behavior, so that these kinds of type-related ambiguities don't reoccur. So, for count_user_posts(), there'd be tests that show that both 0 and non-zero results are returned as ints. Your technique will work; a more PHPUnit-native way is:

$this->assertInternalType( 'int', count_user_posts( $user_id ) );

Alternatively, you could go through any existing count_user_posts() tests - in this case, there are some - and change assertEquals() to assertSame(). This has the same effect.

[Rarst]

Anecdotally I just had to fix a regression where strict check was being made against the type count_user_post() is supposed to return.

String return from a function that is supposed to return a number and is explicitly documented to return an integer number seems like a bug to fix to me.

[jdgrimes]

Related: #35404