Skip to:
Content

Opened 8 years ago

Closed 8 years ago

#2733 closed defect (bug) (fixed)

groups_join_group() parameter fixes

Reported by: r-a-y Owned by:
Milestone: 1.5 Priority: normal
Severity: Version: 1.2.6
Component: Groups Keywords: has-patch
Cc: r-a-y-@…

Description

When passing an external $user_id or $group_id, groups_join_group() doesn't accurately pass the parameters properly.

1) groups_join_group() checks if $bp->current_group exists, but it should be checking if a $group_id is passed

2) groups_record_activity() in groups_join_group() needs to pass the $user_id so it accurately records activity for users who aren't logged in

Patch is against 1.2.6.

Attachments (1)

2733.001.patch (895 bytes) - added by r-a-y 8 years ago.

Download all attachments as: .zip

Change History (8)

@r-a-y
8 years ago

#1 @DJPaul
8 years ago

  • Version changed from 1.2.6 to 1.3

Thanks for the patch, r-a-y. I was about to put this into trunk then started wondering why $bp->groups->current_group is set here at all. We're only using it for the group permalink and name.

If, on a group page, someone called this function without a different group id (i.e. not the current group's ID), we'd end up reassigning $bp->groups->current_group and the sky would fall. Instead of re-assigning the global, can anyone see any issue with assigning the group object to a new local variable and using that in the arguments to groups_record_activity()?

Also bumping to 1.3 as too late for 1.2.7.

#2 @DJPaul
8 years ago

  • Version changed from 1.3 to 1.2.6

#3 @DJPaul
8 years ago

s/without/with

#4 @r-a-y
8 years ago

  • Cc r-a-y-@… added

My initial patch removed $bp->groups->current_group with a local variable, but I decided to put it back for legacy purposes (if any).

I'm assuming that the initial intent behind $bp->groups->current_group was that it'd be only used from a group page or loop.

#5 @r-a-y
8 years ago

Maybe:

if ( $group_id && !$bp->groups->current_group )

#6 @DJPaul
8 years ago

The patch I'm about to commit builds off yours, but switches this to use a local object. If $bp->groups->current_group is already set (i.e. we're in the context of a groups loop, as you suggest), we'll use that rather than create a new database object.

#7 @djpaul
8 years ago

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

(In [3408]) Use the correct group ID and user ID in groups_join_group(). Fixes #2733, props r-a-y.

Note: See TracTickets for help on using tickets.