WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 6 months ago

#40953 new defect (bug)

Empty values are handled inconsistently between wpdb->get_results() and wpdb->get_col()

Reported by: DrewAPicture Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 0.71
Component: Database Keywords: dev-feedback
Focuses: performance Cc:

Description (last modified by johnjamesjacoby)

As outlined and discussed yesterday in Slack here, here, and 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.

Change History (3)

#1 @DrewAPicture
6 months ago

  • Description modified (diff)

#2 @DrewAPicture
6 months ago

Request for insight posted on the ezSQL repo.

#3 @johnjamesjacoby
6 months ago

  • Description modified (diff)

In my research, all of our tests still pass if we remove the && $values[$x] !== '' bit from get_var() and nothing visibly obviously breaks. This may be because we don't test this specifically, recent improvements to charset & collation safety, or some 14 year-old secret protection that shouldn't be removed that lacks adequate documentation.

Note: See TracTickets for help on using tickets.