Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 4 weeks ago

Last modified 2 weeks ago

#6749 closed enhancement (fixed)

bp_get_group_avatar() needs to work outside the loop

Reported by: slaFFik Owned by: espellcaste
Milestone: 10.0.0 Priority: normal
Severity: normal Version: 1.0
Component: Groups Keywords: has-patch has-unit-tests dev-reviewed
Cc:

Description

Currently bp_get_group_avatar() function doesn't check $groups_template global var, although using it extensively.
If you try to use this function not in a loop scope - it will generate lots of notices.

It makes it impossible to use outside of the loop more relevant bp_get_group_avatar() comparing to bp_core_fetch_avatar().

Attachments (5)

6749-1.diff (54.6 KB) - added by espellcaste 5 weeks ago.
6749-2.diff (55.1 KB) - added by espellcaste 5 weeks ago.
6749-2.suggestions.patch (29.5 KB) - added by imath 5 weeks ago.
6749-3.diff (38.8 KB) - added by espellcaste 5 weeks ago.
6749-4.diff (23.2 KB) - added by espellcaste 5 weeks ago.

Download all attachments as: .zip

Change History (21)

#1 @boonebgorges
6 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from Awaiting Review to Under Consideration

I agree that there should be a way to get a group avatar outside of a group loop.

I'm not 100% sure that bp_get_group_avatar() is the way to do it. Our -template.php functions are pretty inconsistent throughout BuddyPress. In some places, these functions accept an argument so that they can be used outside of the loop, and in other places they do not. Many of the bp-groups-template.php functions, for example, accept a $group object. It's easy for us to add this functionality to bp_get_group_avatar(), but this might also be a good time to take a moment and see whether we'd like to take a step toward greater consistency here. What if, for example, we introduced a function like this:

function bp_get_group_from_param( $group = false ) {
    global $groups_template;

    $group_obj = null;

    if ( $group instanceof BP_Groups_Group ) { // or is_object()
        $group_obj = $group;
    } elseif ( is_numeric( $group ) ) {
        $group_obj = groups_get_group( array( 'group_id' => $group ) );
        // maybe some error checking to make sure the group exists
    } elseif ( isset( $groups_template->group ) ) {
        $group_obj = $groups_template->group;
    }

    return $group_obj;
}

Then, within each template function:

function bp_get_group_avatar( $group = false ) {
    if ( ! $group = bp_get_group_from_param( $group ) ) {
        return '';
    }

    // Now we can assume $group is correct
}

This way, devs have the option of passing a group object, passing a group ID, or passing nothing and having the group determined from the group loop context.

#2 @r-a-y
6 years ago

The bp_get_group_from_param() function would be pretty awesome.

Also, check out a related, crappy patch here - ticket:6387#comment:6 - that only adds groups_get_current_group() as a fallback and doesn't do any group lookups.

#3 @DJPaul
5 years ago

  • Component changed from Tools - Warnings/Notices to Core

#4 @DJPaul
5 years ago

  • Milestone changed from Under Consideration to Awaiting Review

#5 @DJPaul
5 years ago

  • Component changed from Core to Groups
  • Keywords 2nd-opinion removed
  • Milestone changed from Awaiting Review to Future Release
  • Summary changed from bp_get_group_avatar() needs extra error checking to bp_get_group_avatar() needs to work outside the loop

#6 @espellcaste
2 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Contributions to 10.0.0
  • Owner set to espellcaste
  • Status changed from new to assigned
  • Version set to 9.0.0

I like @boonebgorges suggestions and I'll work on a patch. On the BP REST API and BP CLI, I opted to use bp_core_fetch_avatar precisely because I noticed you could not use bp_get_group_avatar properly outside of the loop.

#7 @espellcaste
5 weeks ago

  • Keywords has-patch has-unit-tests added; needs-patch removed
  • Type changed from defect (bug) to enhancement

This is the first patch, of a few more I'm sure, introducing the new helper function, bp_get_group_from_param, to grab a group using one of the identifiers: slug, ID, BP_Groups_Group which defaults to the current group in the BP_Groups_Template loop.

This is a preliminary patch which accounts to:

  • creating the helper function
  • updating a few functions to use this new function (more needs to be updated)
  • adding basic support to unit tests (support will be improved in future patches)

Some notes:

  • PHPDoc was updated to remove references of getting the ID from the current loop;
  • bp_group_is_visible was updated to check against a specific user ID;
  • bp_get_group_avatar was updated to pass other parameter to bp_core_fetch_avatar which were missing.

The goal is to gather preliminary feedback before diving even deeper. =P

@espellcaste
5 weeks ago

#8 @imath
5 weeks ago

  • Version changed from 9.0.0 to 1.0

Hi @espellcaste

Thanks a lot for your work and patch about this improvement. I'll look at it in detail asap. Here's some first thoughts:

  • what about bp_get_group_by( $field, $value ) like the WP get_user_by() function?
  • It seems weird to have the from_param suffix and being able to pass the entire Group object. I would probably have used a bp_get_group( $group_id_or_group_object ) and used bp_get_group_by( 'id', $value ) function inside it in case a Group ID was passed.

I've changed the version to 1.0 as this was the BuddyPress version when bp_get_group_avatar() was introduced.

#9 @espellcaste
5 weeks ago

I think those are good suggestions, particularly the bp_get_group_by one (other objects/components would benefit from having similar instances).

I'll update the patch while the rest of the diff is reviewed. :)

@espellcaste
5 weeks ago

#10 @espellcaste
5 weeks ago

Second patch with the code suggestions applied. Minor tweaks were made to the PHPDoc and unit tests. :)

#11 @imath
5 weeks ago

  • Keywords dev-reviewed added

Hi @espellcaste

Thanks a lot for your work on this second patch. It looks nice, you'll find some suggestions mainly about DocBlocks into the patch I suggest to use to compliment yours below.

NB: the patch also fixes an issue with how you edited bp_get_group_avatar(): the html argument needs to default to true otherwise the avatar image is not output, and we get instead the avatar URL (which can be retrieved using bp_get_group_avatar_url().

Once you've fixed the bp_get_group_avatar() issue and If you're confident with the patch, feel free to commit it.

#12 @espellcaste
5 weeks ago

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

In 13085:

We are introducing in this commit two new helper functions to get a group: bp_get_group_by and bp_get_group. The former allows for getting a group by id/ID or slug.
And the latter allows for getting a group by id/ID, group object, slug. And when used in the context of a Group loop built by the BP_Groups_Template class, it defaults to the Group being iterated on.

  • The new helper functions were applied to several functions in the Group component.
  • bp_group_is_visible was updated to check against a specific user ID. Previously, only the current logged in user was verified.
  • bp_get_group_avatar was updated to use the new functions and new arguments that are passed to bp_core_fetch_avatar were added.
  • PHPDoc were updated to reflect those changes.

Props imath, boonebgorges and DJPaul

Fixes #6749 (trunk)

#13 @espellcaste
5 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I mistakenly fixed this ticket where I had other patches to send.

@espellcaste
5 weeks ago

@espellcaste
5 weeks ago

#14 @espellcaste
5 weeks ago

6749-4.diff is a follow up patch. For the purposes of this ticket, that's the only patches I'm gonna suggest. :)

Last edited 5 weeks ago by espellcaste (previous) (diff)

#15 @espellcaste
4 weeks ago

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

In 13097:

Adding the newly created helper functions bp_get_group_by and bp_get_group for Groups related functions
and template files.

Also, adding several PHP fixes/tweaks. And unit tests.

Fixes #6749

#16 @espellcaste
2 weeks ago

In 13103:

Improving the group member count routine and the function helper.

The group member count routine was updated to avoid direct, uncached, SQL query and unnecessary cache refresh when a group's Members page was viewed.

is now being used to get the group member count which takes into account users' existence in the site,

the query is now cached and filterable.

was also updated to get the current group from if available.

Props imath
Fixes #7614 and see #6749

Note: See TracTickets for help on using tickets.