[wp-trac] [WordPress Trac] #40953: Empty values are handled inconsistently between wpdb->get_results() and wpdb->get_col()

[WordPress Trac]

#40953: Empty values are handled inconsistently between wpdb->get_results() and
wpdb->get_col()
--------------------------+------------------------------
 Reporter:  DrewAPicture  |       Owner:
     Type:  defect (bug)  |      Status:  new
 Priority:  normal        |   Milestone:  Awaiting Review
Component:  Database      |     Version:  0.71
 Severity:  normal        |  Resolution:
 Keywords:  dev-feedback  |     Focuses:  performance
--------------------------+------------------------------
Description changed by DrewAPicture:

Old description:

> As outlined and discussed yesterday in Slack
> [https://wordpress.slack.com/archives/C02RQBWTW/p1496821047585923 here],
> [https://wordpress.slack.com/archives/C02RQBWTW/p1496844209230845 here],
> and [https://wordpress.slack.com/archives/C02RQBWTW/p1496850305693715
> here], `wpdb` treats empty stored values differently in the
> `get_results()` and `get_col()` methods. This is because of the use of
> `get_var()` inside of `get_col()`, which defaults to null for empty
> values.
>
> For example, let's say you're running a query like `SELECT rate FROM
> sometable` through `get_results()`. With the default parameters and empty
> values for the column, you'd get something like the following:
>
> {{{
> array(2) {
>   [0]=>
>   object(stdClass)#1734 (1) {
>     ["rate"]=>
>     string(0) ""
>   }
>   [1]=>
>   object(stdClass)#1735 (1) {
>     ["rate"]=>
>     string(0) ""
>   }
> }
> }}}
>
> If you ran that same query through `get_col()`, you'd instead get an
> array of `null` values:
>
> {{{
> array(4) {
>   [0]=>
>   NULL
>   [1]=>
>   NULL
> }
> }}}
>
> This seems oddly inconsistent. And writing tests for the workaround is
> annoying in that creates the need to understand the core workaround in
> the future.
>
> Now, this code goes all the way back to [112], so changing the default
> behavior is not even on the table.
>
> Some solutions brainstormed with @boonebgorges and @johnjamesjacoby
> include:
>
> * A global flag to check against, i.e. `wpdb_get_col_force_strings( true
> );`
> * A global flag in the form of a constant
> * A settable `wpdb`-level flag
> * A new argument for `get_col()` to selectively change the behavior.
>
> The global flag ideas are attractive because they cover the entire DB
> stack: whether you're using the abstraction layers like `get_posts()`,
> `WP_Query`, or any of the other query classes, it ''just works'' all the
> way down the line.
>
> The settable `wpdb` flag is attractive only if you're really working with
> direct queries like we are in our custom table query classes. The same
> goes for a new argument in `get_col()`, though both could be implemented
> higher up the stack in the form of arguments or filters.
>
> I think a good first step here would be to try to benchmark performance
> for all of the listed options, just to see what we're looking at. The
> global flag choices seem like they could be the least impactful.
>
> In the short term, our workaround for AffiliateWP will probably be to
> create a wrapper for `get_results()` that simply plucks the values out so
> we can maintain consistency, but I'm not a big fan of writing and
> maintaining core workarounds in perpetuity.
>
> Whichever way we go in core, this is something that we should probably
> address. Who knows how many workarounds there are currently in the wild
> to fix this.

New description:

 As outlined and discussed yesterday in Slack
 [https://wordpress.slack.com/archives/C02RQBWTW/p1496821047585923 here],
 [https://wordpress.slack.com/archives/C02RQBWTW/p1496844209230845 here],
 and [https://wordpress.slack.com/archives/C02RQBWTW/p1496850305693715
 here], `wpdb` treats empty stored values differently in the
 `get_results()` and `get_col()` methods. This is because of the use of
 `get_var()` inside of `get_col()`, which defaults to null for empty
 values.

 For example, let's say you're running a query like `SELECT rate FROM
 sometable` through `get_results()`. With the default parameters and empty
 values for the column, you'd get something like the following:

 {{{
 array(2) {
   [0]=>
   object(stdClass)#1734 (1) {
     ["rate"]=>
     string(0) ""
   }
   [1]=>
   object(stdClass)#1735 (1) {
     ["rate"]=>
     string(0) ""
   }
 }
 }}}

 If you ran that same query through `get_col()`, you'd instead get an array
 of `null` values:

 {{{
 array(4) {
   [0]=>
   NULL
   [1]=>
   NULL
 }
 }}}

 This seems oddly inconsistent. And writing tests for the workaround is
 annoying in that creates the need to understand the core workaround in the
 future.

 Now, this code goes all the way back to [112], so changing the default
 behavior is not even on the table.

 Some solutions brainstormed with @boonebgorges and @johnjamesjacoby
 include:

 * A global flag to check against, i.e. `wpdb_get_col_force_strings( true
 );`
 * A global flag in the form of a constant
 * A settable `wpdb`-level flag
 * A new argument for `get_col()` to selectively change the behavior.

 The global flag ideas are attractive because they cover the entire DB
 stack: whether you're using the abstraction layers like `get_posts()`,
 `WP_Query`, or any of the other query classes, it ''just works'' all the
 way down the line.

 The settable `wpdb` flag is attractive only if you're really working with
 direct queries like
 [https://github.com/AffiliateWP/AffiliateWP/blob/master/includes/abstracts
 /class-db.php#L179 we are] in our custom table query classes. The same
 goes for a new argument in `get_col()`, though both could be implemented
 higher up the stack in the form of arguments or filters.

 I think a good first step here would be to try to benchmark performance
 for all of the listed options, just to see what we're looking at. The
 global flag choices seem like they could be the least impactful.

 In the short term, our workaround for AffiliateWP will probably be to
 create a wrapper for `get_results()` that simply plucks the values out so
 we can maintain consistency, but I'm not a big fan of writing and
 maintaining core workarounds in perpetuity.

 Whichever way we go in core, this is something that we should probably
 address. Who knows how many workarounds there are currently in the wild to
 fix this.

--

--
Ticket URL: <https://core.trac.wordpress.org/ticket/40953#comment:1>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform