Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 7 years ago

#4185 closed defect (bug) (fixed)

Calling groups_get_group() with a non-existent group id returns a BP_Groups_Group object populated with the provided ID

Reported by: chriskeeble Owned by: boonebgorges
Milestone: 1.8 Priority: normal
Severity: normal Version: 1.5.5
Component: Groups Keywords:
Cc:

Description

A number of BuddyPress plugins use checks for whether a group exists by ID using calls such as:

e.g. #1

if (!$group = groups_get_group(array('group_id' => $id))) {

e.g. #2

!groups_get_group(array('group_id' => $id))

If no group with the specified ID exists, should the BP_Groups_Group constructor set the ID property to null? It currently leaves it set to whatever value was passed in.

Ideally groups_get_group should return null (though this could be a breaking change?)

Change History (9)

#1 @DJPaul
8 years ago

  • Milestone changed from Awaiting Review to Future Release

Good call. I am setting this to future release milestone, but if feedback from other developers think we could make a change in 1.6 for this, we'll do that.

I agree it should return null, but I think we need a full development cycle (1.7) to properly test and see if this breaks anything. I personally think we could do your first suggestion for 1.6, and the null change for 1.7.

#2 @r-a-y
8 years ago

  • Keywords dev-feedback added

I'd say this is important to fix for BP 1.6 because plugin devs are being encouraged to use groups_get_group() instead of new BP_Groups_Group() for the new object caching capability. See #3770.

#3 @boonebgorges
8 years ago

This is not an issue with groups_get_group() but with the BP_Groups_Group class itself. If you instantiate the class like this

$group = new BP_Groups_Group( $nonexistent_group_id );

it'll return an object with all properties null except for 'id', which will be $nonexistent_group_id. groups_get_group() is just a wrapper, and doesn't affect this aspect of the behavior.

So I agree with DJPaul that we can't just change it - it's likely that there are plugins relying on this odd behavior. If we wait until after 1.6, then we can make a breaking change and publicize it widely. As it stands, we are too close to release.

#4 @r-a-y
8 years ago

Gotcha! It's not good to post when you're sleep-deprived :)

#5 @DJPaul
7 years ago

  • Keywords dev-feedback removed
  • Milestone changed from Future Release to 1.7

#6 @DJPaul
7 years ago

What type of response should we return? An empty BP_Groups_Group seems a bit pointless. A WP_Error or a null response, maybe?

#7 @johnjamesjacoby
7 years ago

  • Milestone changed from 1.7 to 1.8

Looks like we missed the 1.7 early boat again. Punting to 1.8.

#8 @boonebgorges
7 years ago

I don't think we should be returning null or false. $foo = new BP_Groups_Group() should return a BP_Groups_Group object, regardless of whether there's a corresponding group. See the behavior of WP_Post and WP_User. I'm willing to be persuaded that the wrapper function groups_get_group() ought to return null for non-existent group ids, but that's not the subject of this ticket: the OP was referring to plugins that are checking against BP_Groups_Group itself. Those plugins are simply doing it wrong.

We should, however, probably be setting the id property to 0 in this case. I'm not persuaded that this'll really break much of anything - what is a scenario in which it's important for BP_Groups_Group::id to be set for a non-existent group? I'm just going to make the change, and we'll see if any plugins are doing anything strange during the beta period.

#9 @boonebgorges
7 years ago

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

In 7134:

BP_Groups_Group::id should be 0 for non-existent groups

Fixes #4185

Props chriskeeble

Note: See TracTickets for help on using tickets.