Skip to:
Content

BuddyPress.org

Opened 3 years ago

Closed 3 years ago

#7202 closed defect (bug) (fixed)

BP_XProfile_Query::find_compatible_table_alias uses field_id instead of field

Reported by: thomaslhotta Owned by: boonebgorges
Milestone: 2.7 Priority: normal
Severity: normal Version: 2.2
Component: Extended Profile Keywords: has-patch commit
Cc:

Description

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?

Attachments (3)

7202.01.patch (787 bytes) - added by r-a-y 3 years ago.
7202.unit-test.patch (1.3 KB) - added by r-a-y 3 years ago.
Updated unit test so compatible alias is used
7202.diff (3.1 KB) - added by boonebgorges 3 years ago.

Download all attachments as: .zip

Change History (14)

#1 @DJPaul
3 years ago

@boonebgorges I think you wrote this.

#2 @boonebgorges
3 years ago

  • Owner set to r-a-y
  • Status changed from new to assigned

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

#3 @DJPaul
3 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 2.6.3

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.

#4 @DJPaul
3 years ago

@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?

#5 @r-a-y
3 years ago

  • Keywords needs-unit-tests removed
  • Version changed from 2.6.1.1 to 2.2

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.

Last edited 3 years ago by r-a-y (previous) (diff)

@r-a-y
3 years ago

#6 @r-a-y
3 years ago

  • Keywords has-patch added
  • Milestone changed from 2.6.3 to 2.7

@r-a-y
3 years ago

Updated unit test so compatible alias is used

@boonebgorges
3 years ago

#7 @boonebgorges
3 years ago

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.

#8 @r-a-y
3 years ago

  • Keywords commit added

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!

#9 @boonebgorges
3 years ago

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

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

#10 @boonebgorges
3 years ago

  • Owner changed from r-a-y to boonebgorges

#11 @boonebgorges
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

Note: See TracTickets for help on using tickets.