Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#5958 closed enhancement (fixed)

Add object parameter to bp_core_fetch_avatar_no_grav

Reported by: dcavins Owned by: boonebgorges
Milestone: 2.2 Priority: low
Severity: minor Version: 2.1
Component: Media Keywords: has-patch needs-unit-tests
Cc: david.cavins@…

Description

I'd like to add a second parameter to bp_core_fetch_avatar_no_grav so that you can choose when you want to use Gravatars. For instance, I'd like to skip the Gravatar check for group avatars since they're extremely unlikely to turn anything up. (The "email address" used for groups is {$item_id}-{$object}@{bp_get_root_domain()} )

Adding the $object parameter means I can tune the filter to only apply to groups.

function no_gravatars_for_groups( $no_grav, $object ) {
  if ( $object == 'group' )
    $no_grav = true;

  return $no_grav;
}
add_filter( 'bp_core_fetch_avatar_no_grav', 'no_gravatars_for_groups', 10, 2 );

Thanks!

Attachments (3)

5958.patch (560 bytes) - added by dcavins 6 years ago.
5958.2.patch (14.4 KB) - added by dcavins 6 years ago.
Removes extract() from bp_core_fetch_avatar()
5958.3.patch (18.5 KB) - added by dcavins 6 years ago.
Changes bp_core_fetch_avatar(); adds tests.

Download all attachments as: .zip

Change History (14)

@dcavins
6 years ago

#1 @boonebgorges
6 years ago

How about if we pass all the args?

apply_filters( 'bp_core_fetch_avatar_no_grav', $no_grav, $params )

This would be much more flexible, and seems much better to me.

The problem with this is that we extract() these params near the top of the function, and do some mods on the extracted values before they get passed to the filter. So to make this work, we'd probably want to do the following: (a) Stop extracting; (b) all the modifications that take place to the variables should happen to the $param values instead; (c) pass the function $args to the filter in addition to the $params.

What do you think? Can you write this patch? :)

#2 @DJPaul
6 years ago

  • Keywords needs-refresh added; has-patch removed
  • Milestone changed from Awaiting Review to 2.2

#3 @dcavins
6 years ago

  • Cc david.cavins@… added
  • Keywords has-patch added; needs-refresh removed

Here's a pass at handling the function as @boonebgorges suggested. I removed extract() in favor of modifying the $params array. I like that when $params is passed via a filter, you get the updated and so-far-so-filtered array rather than the original parsed array. I also moved some code around for clarity (I hope!) and was able to simplify some bits.

@dcavins
6 years ago

Removes extract() from bp_core_fetch_avatar()

#4 follow-up: @boonebgorges
6 years ago

  • Keywords needs-unit-tests added

This looks good at a glance, but: can I be a real pain in the butt and ask for one or two unit tests before pulling the trigger? I don't think we need exhaustive unit tests for bp_core_fetch_avatar() just for the purposes of this patch - it would be a huge job - but I would like tests that demonstrate that each previously extracted parameter is getting correctly parsed to the end of the function. dcavins, could you have a go at this?

#5 in reply to: ↑ 4 @dcavins
6 years ago

Replying to boonebgorges:

I would like tests that demonstrate that each previously extracted parameter is getting correctly parsed to the end of the function.

Yep, I'm happy to add unit tests. Can you clarify what you mean in the above quote? Maybe checking that the arguments being passed out of the later filters are what we expect them to be? (That's how I checked my own work.)

#6 follow-up: @boonebgorges
6 years ago

Maybe checking that the arguments being passed out of the later filters are what we expect them to be? (That's how I checked my own work.)

That's not what I meant - I was thinking more that you'd try to generate an avatar that used all non-default parameters, and make sure the output matched what you'd expect for those params. However, your suggestion of testing what's getting passed to the filter is probably quite a bit easier, and would be fine for these purposes.

#7 in reply to: ↑ 6 @dcavins
6 years ago

Replying to boonebgorges:
That's what I was thinking: ask for a non-standard avatar and make sure those parameters survive the code I wrote. :)

I'll add a check to the output as well. Thanks for your comments!

#8 @dcavins
6 years ago

I'm adding a new patch that adds tests. The tests check the $params as passed out of the main filter as well as the html output for both gravatar and no-gravatar requests. I'm a little skeptical of the maintainability of the gravatar portion of the test, since it was necessary to recreate quite a bit of code from the original function. Please let me know if there are other test cases that you'd like me to cover; I'm happy to write more tests.

@dcavins
6 years ago

Changes bp_core_fetch_avatar(); adds tests.

#9 @boonebgorges
6 years ago

In 9124:

Simplify default argument parsing in bp_core_fetch_avatar().

  • Refrain from setting a couple of unnecessary variables.
  • Remove comments, which are redundant with the docblock.

Props dcavins.
See #5958.

#10 @boonebgorges
6 years ago

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

In 9125:

Don't extract() in bp_core_fetch_avatar(), and pass params to 'bp_core_fetch_avatar_no_grav' hook.

Passing the modified parameters to the hook required moving away from the use
of extract(). See #5698.

Props dcavins.
Fixes #5958.

#11 @DJPaul
4 years ago

  • Component changed from API - Avatars to Media
Note: See TracTickets for help on using tickets.