Filtering Results when set to false or 0 does not filter results

[alexstandiford]

This seems to be happening because of a vague check in Query::parse_where.

...
if (  ! empty( $this->query_vars[ $column->name ] ) ) {
...

Both 0, and false both cause empty to return true. This causes parse_where to skip parsing a filter by a value set to 0 or false.

[alexstandiford]

In testing, I was able to fix this by adding a more explicit check, but there's probably a better way to go about this.

...
if ( ! empty( $this->query_vars[ $column->name ] ) || is_bool( $this->query_vars[$column->name] ) || 0 === $this->query_vars[$column->name] ) {
...

[JJJ]

What are you setting to false? Can you include a code snippet or example?

[alexstandiford]

Wow, my description issue was terrible, sorry about that. I can't seem to figure out why this was a problem for me now.

If I stumble on this again, I will open a new issue with a proper description on what's going on.

edit decided to leave open since @nosegraze mentioned seeing this, too.

[ashleyfae]

@alexstandiford Not sure if this was your original issue but I have seen something that maybe sounds similar in RCP. I wanted to query for results where a value was equal to 0 explicitly. As an example:

We have a column in RCP for auto_renew that can be set to 0 or 1. I recall that I was originally unable to use the query() method to return all memberships that had auto_renew set to 0 to due that use of empty() you highlighted.

[alexstandiford]

@nosegraze This is exactly what I'm referring to, and what I experienced. I can't seem to duplicate it.

Do you remember what you did to get around this?

[ashleyfae]

@alexstandiford Yeah I basically just did exactly what you did, but without the boolean check. Just added a check against 0. https://github.com/restrictcontentpro/restrict-content-pro/blob/master/includes/database/engine/class-query.php#L1112

if ( ! empty( $this->query_vars[ $column->name ] ) || 0 === $this->query_vars[ $column->name ] ) {

It feels super hacky, so it's not great. I don't remember exactly, but I suspect isset() wasn't an option because all query vars were set to an empty string or null or something.

[JJJ]

I definitely understand the problem now.

Maybe we should check query_var_originals instead of query_vars here. That way isset() would work, allowing for empty string non-null values as well?

This would be the first/only time we'd be using it though. That array is intentionally not filtered, so there would not be (and probably should not be) a way to override it like you can currently with query_vars.

Ideally, we'd find a way to make query_wars work. That probably means rethinking how they're parsed to avoid needing to define every single type and case. Perhaps instead of an empty string (''), we come up with a better default starting value? (I only used an empty string originally because it didn't require any additional validation or preparation for the MySQL query to be valid.)

[alexstandiford]

Tripped over this bug again today, this time I had enough knowledge to make sense of the issue, and what @jjj was proposing. From what I can tell, this works beautifully.

A PR will follow momentarily.

(Edit: just noticed @jjj already submitted. I'll review that instead)

[JJJ]

Problem solved via #35

Closing this out. Thank you everyone 💫