WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#30257 closed defect (bug) (wontfix)

wpdb::get_var() returns NULL for existing values that are empty strings

Reported by: tyxla Owned by:
Milestone: Priority: normal
Severity: normal Version: 0.71
Component: Database Keywords: has-patch
Focuses: Cc:

Description

I have noticed that using $wpdb->get_var() returns NULL in both of these cases:

  • When there is no such value in the database
  • When there is a value, but it is an empty string

This appears to be intended and is explicitly defined on the following line: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/wp-db.php#L1870

In my opinion it does not make sense for an empty string to be represented as NULL, as there is a distinguishable difference between NULL and empty string.

Attachments (1)

wp-db.php.patch (412 bytes) - added by tyxla 6 years ago.
The modification that I suggest for fixing this issue.

Download all attachments as: .zip

Change History (9)

@tyxla
6 years ago

The modification that I suggest for fixing this issue.

#1 @tyxla
6 years ago

  • Type changed from enhancement to defect (bug)

#2 @jdgrimes
6 years ago

  • Keywords has-patch added

This behavior goes all the way back to when EZ SQL was added in [112]. That means that there is 11 years of code built on top of this, and expecting this behavior. There are even places in core that use get_row() instead to get around this "feature". It might too late to turn back now.

#3 @jdgrimes
6 years ago

The workaround in get_option() was added in [3023] (then removed in [3025], added in [3026], removed in [3068], but restored in [3071]). There was no mention of altering get_var() instead in #1859.

#4 @tyxla
6 years ago

Modifying the get_var() will not affect the get_row() calls that @jdgrimes has indicated - these calls will keep working the same way as they have nothing to do with get_var().

I've gone through the entire core and searched for any occurrences of get_var(), and I did not find any single call that depends on the fact that empty strings are considered NULL.

Taking as granted the fact that in PHP NULL == '' is true, the only way to take advantage of the current way get_var() is working is to test its result by using the value and type comparison operator (!== or ===).

I was able to find only one occurrence of such check in the core: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/link-template.php#L1574 but will continue working the same way, as it simply modifies NULL with '', so the result is '', it will keep working the same way like it does now.

Finally, considering all of the above notes I can conclude that I did not find any plausible downsides of having this modification implemented into the core.

#5 @jdgrimes
6 years ago

Thanks for taking the time to do some more research, @tyxla. I realize that this won't affect the get_row() calls, I just wanted to document that because it is related. I wanted to see if there was any pertinent discussion regarding get_var() in the corresponding ticket, but as I said above there wasn't.

I think this would be a good change—I just wanted to see where the code came form and whether there was any reason for it.

It's possible that there are plugins that are relying on this behavior, but even so, I'm not sure that the change could really break anything badly. I guess that's something we should still consider though.

#6 @pento
6 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed
  • Version changed from trunk to 0.71

Thanks for the bug report, tyxla!

I agree that this behaviour isn't ideal, and it's clearly inconsistent with the behaviour of the other get_* methods (except for get_col(), which calls get_var()).

However, due to the age of this behaviour, and the potential to introduce subtle bugs into plugins that expect it, this isn't really a thing we can fix.

If you run into any other behaviour you think should be fixed, please don't hesitate to open a new ticket - we're always happy to discuss these things, even if they're not something we can change. :-)

#7 @nacin
6 years ago

This odd quirk in get_var() is also why we use get_row() for a single field in many other places, such as get_option(), get_site_option(), get_metadata(), etc.

I bet we could change it and few would notice... but @pento is also right that it would break a lot.

#8 @tyxla
6 years ago

I understand, although this was not what I expected to be the conclusion here.

Guess we have to stick with using get_col() and get_row(). Anyway, fixing that issue was worth a shot, at least I rest calm that I tried. :)

Last edited 6 years ago by tyxla (previous) (diff)
Note: See TracTickets for help on using tickets.