BP_XProfile_Query::find_compatible_table_alias uses field_id instead of field

[thomaslhotta]

Hi,

was just playing around with the BP_XProfile_Query class and discovered this on line 518:

} elseif ( isset( $sibling['field_id'] ) && isset( $clause['field_id'] ) && $sibling['field_id'] === $clause['field_id'] ) {
    $compatible_compares = array( '!=', 'NOT IN', 'NOT LIKE' );
}

Shouldn't the array key be field instead of field_id for this to work?

[DJPaul]

@boonebgorges I think you wrote this.

[boonebgorges]

I wrote it for WordPress but I'm pretty sure @r-a-y ported it here :)

[DJPaul]

Our new code coverage report shows execution doesn't go inside this if statement (for our unit tests): ​https://codecov.io/gh/buddypress/BuddyPress/src/a99367360691ea789fdad9a681bce41e5bcfa819/src/bp-xprofile/classes/class-bp-xprofile-query.php#L519

If this has never worked since implementation, we should put it in 2.6.x since at least one developer (the ticket reporter) has encountered this issue in the wild.

[DJPaul]

@r-a-y I've confirmed this issue is valid but I can't figure out how to write a test demonstrating this failure before we fix it. Can you have a look sometime, please?

[r-a-y]

but I'm pretty sure @r-a-y ported it here :)

@boone committed the changes - r9178 :)

I do actually want to make a change here so BP_XProfile_Query extends BP_Recursive_Query to reduce code duplication. Meant to do this awhile ago, but completely forgot about it.

---

As for this issue, I've attached a unit test using 'relation' => 'AND', 'compare' => 'NOT IN'. Patch passes using either 'field' or 'field_id'. Meaning this isn't important enough to fix in a point release.

I'm not sure if the table aliases are already optimized and whether line 519 in BP_XProfile_Query is necessary or not.

Would appreciate some feedback from Boone here.

Update: I checked the SQL clauses after the patch and @thomaslhotta is right. The JOIN is removed. I say we fix this in BP 2.7 instead.

I haven't added an assert to see if the INNER JOIN is removed, but I could.

[r-a-y]

Updated unit test so compatible alias is used

[boonebgorges]

The unit test in 7202.unit-test.patch​ is useful, but if I'm looking correctly, it passes without the patch. The bug described here has to do with too many table joins, not with incorrect results.

7202.diff​ adds a test against the SQL string.

[r-a-y]

As noted in comment:5, unit-test.patch was to confirm that this wasn't a big enough problem to fix in a point release and that I didn't do a test for the INNER JOIN omission.

7202.diff looks good to me!

[boonebgorges]

Meaning this isn't important enough to fix in a point release.

Derp, my eyes must have glazed over that sentence :) Sorry about that.

[boonebgorges]

In 11018:

In BP_XProfile_Query, fix variable name typo that prevented table JOINs from shared between meta query clauses.

Introduced in [9178].

Props thomaslhotta, r-a-y.
Fixes #7202.