Add object parameter to bp_core_fetch_avatar_no_grav

[dcavins]

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!

[boonebgorges]

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? :)

[dcavins]

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]

Removes extract() from bp_core_fetch_avatar()

[boonebgorges]

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?

[dcavins]

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.)

[boonebgorges]

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.

[dcavins]

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!

[dcavins]

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]

Changes bp_core_fetch_avatar(); adds tests.

[boonebgorges]

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.

[boonebgorges]

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.