Skip to:
Content

BuddyPress.org

Opened 9 months ago

Closed 9 months ago

Last modified 9 months ago

#8037 closed defect (bug) (fixed)

WP 5.1 `wp_parse_id_list()` check breaks `parent_id` parsing

Reported by: boonebgorges Owned by: boonebgorges
Milestone: 4.2.0 Priority: normal
Severity: normal Version:
Component: Groups Keywords:
Cc:

Description

In [11095], when hierarchical groups were introduced, we exploited the behavior of wp_parse_id_list() that converted false to 0. This no longer happens: https://core.trac.wordpress.org/ticket/43977 https://core.trac.wordpress.org/changeset/44546

The logic at https://buddypress.trac.wordpress.org/browser/tags/4.1.0/src/bp-groups/classes/class-bp-groups-group.php?marks=1206#L1204 needs changing.

Attachments (1)

8037.diff (771 bytes) - added by boonebgorges 9 months ago.

Download all attachments as: .zip

Change History (6)

@boonebgorges
9 months ago

#1 @boonebgorges
9 months ago

@dcavins - Since you originally wrote this code, could you please have a look at 8037.diff and check that I've captured the intended behavior of false here?

#2 @dcavins
9 months ago

Hi @boonebgorges. I'm pretty sure that "intended behavior" is a mild overstatement. :)

The logic as originally written was

  • parent_id that is null => don't take the parent_id into account for the query.
  • parent_id that's not null => pass the parameter to wp_parse_id_list() to sort it out.

The note in the code was just to remind users that passing parent_id === false would be equivalent (because of a quirk in wp_parse_id_list()) to passing a parent_id of 0 (and the query would return top-level groups only).

Your patch maintains the behavior as it was, quirks and all, but whether it's the best, most predictable behavior I can't say. The code note for the parameter says "Optional. Array or comma-separated list of group IDs. Results will be limited to children of the specified groups. Default: null." So we don't really make any promises about what false might do. Honestly, converting false to 0 seems to make less sense than treating false like null, except it would change BP_Groups_Group::get()'s behavior.

Thanks for catching this change!

#3 @boonebgorges
9 months ago

Thanks for looking, @dcavins!

I'm also not sure what the "best" behavior is, but I'd prefer to be conservative and stick with the current behavior.

#4 @boonebgorges
9 months ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 12326:

Improve parent_id parsing in BP_Groups_Group.

The previous logic depended on behavior in wp_parse_id_list() that
will change in WP 5.1.

Fixes #8037.

#5 @boonebgorges
9 months ago

In 12327:

Improve parent_id parsing in BP_Groups_Group.

The previous logic depended on behavior in wp_parse_id_list() that
will change in WP 5.1.

Merges [12327] to the 4.0 branch.

Fixes #8037.

Note: See TracTickets for help on using tickets.