WordPress.org

Make WordPress Core

Opened 22 months ago

Last modified 6 weeks ago

#21195 new feature request

get_avatar_url

Reported by: pathawks Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 2.5
Component: General Keywords: has-patch
Focuses: Cc:

Description

There should be a separate function to just get an avatar URL, versus getting the image wrapped in an <img> tag. This function should pass the avatar URL through a filter before returning it.
get_avatar would then call this function, rather than generating a Gravatar URL on its own.

This way, a plugin can grab a user's avatar image without relying on regex to extract the image from the <img> tag, and in a way that is compatible with plugins that replace Gravatar.

Attachments (8)

21195-get_avatar_url.diff (2.8 KB) - added by batmoo 21 months ago.
21195.3.diff (12.7 KB) - added by mdawaffe 18 months ago.
21195-ut.3.diff (20.3 KB) - added by mdawaffe 18 months ago.
21195-param.diff (1.5 KB) - added by cliffseal 14 months ago.
Add parameter to get_avatar
21195.diff (12.8 KB) - added by pento 6 months ago.
21195.2.diff (13.1 KB) - added by pento 6 months ago.
21195.4.diff (13.2 KB) - added by pento 6 months ago.
21195.5.diff (13.1 KB) - added by pento 6 months ago.

Download all attachments as: .zip

Change History (44)

comment:1 c3mdigital22 months ago

  • Type changed from defect (bug) to feature request
  • Version set to 2.5

Wouldn't the get_avatar filter work for this?

comment:2 pathawks22 months ago

Get get_avatar filter works great when you want the full <img> tag.
I'm after a function & filter that applies only to the image URL.

I realize that I could regex the image URL from the <img> tag, but what if a plug-in has replaced get_avatar() in such a way that it no longer returns images? What if I want to include avatars as background with CSS rather than in an <img> tag?

comment:3 Ipstenu22 months ago

This stems from http://wordpress.org/extend/ideas/topic/why-is-avatar-stuff-in-pluggablephp

Honestly I don't know why you'd want to get the avatar image and not put it in an image tag. What are you doing with it if not displaying it? The whole point of the avatar is to show it, and for that you'll always need the img tags.

Which I asked in the other thread too :)

comment:4 toscho22 months ago

  • Cc info@… added

@Ipstenu You could use the avatar as a background image for example.

comment:5 readysetawesome21 months ago

  • Cc readysetawesome added

+1

This is an issue any time you want to use avatar images outside the context of displaying avatars in an HTML page, which is IMO a reasonable thing to want to do. The use case necessitates really ugly parsing code, and seems like it could be much more cleanly handled with some core code changes.

comment:6 batmoo21 months ago

  • Cc batmoo@… added

comment:7 batmoo21 months ago

  • Keywords has-patch added

Patch breaks URL-building piece of get_avatar into a new function called get_avatar_url.

comment:8 pathawks21 months ago

Beautiful!

comment:9 greenshady20 months ago

  • Cc justin@… added

comment:10 mdgl20 months ago

This would be useful also for people who just want to add standard HTML attributes to the <img> tag for a gravatar, for example "title" (for tooltip), "id" or further "class" names.

You could argue, however that these simple cases should be catered for directly by get_avatar() with additional optional parameters in the same way as "alt".

To do this kind of thing at the moment you need to either replace the function entirely or parse the returned <img> tag in a filter. Both methods are messy and error prone.

mdawaffe18 months ago

comment:12 follow-up: mdawaffe18 months ago

attachment:21195.3.diff refreshes the patch and adds some functionality we've found useful at WordPress.com.

attachment:21195-ut.3.diff provides new test coverage for get_avatar() and get_avatar_url(). All test pass for the attached patch in PHP5.3 and PHP5.4. I don't have a handy way of testing under PHP5.2 right now.

  • Get avatar by user_id, user (new), email address, MD5 hash (new), comment, or post (new).
    • Getting by MD5 is handy when you have a hash but no the email address.
    • Getting by user or post is not strictly necessary, but provides a little extra context to plugins hooking in to various hooks.
  • Support for all Gravatar parameters:
    • size
    • default (404, retro, monsterid, wavatar, identicon, mysteryman, blank, gravatar logo, and URL)
    • force_default
    • rating
  • pre_get_avatar_url, get_avatar_url, pre_get_avatar, get_avatar filters (see #21930)
    • The first three of which can modify the $args parameter on the fly (it's passed by reference to the filter) allowing plugins to modify default behavior without needing to duplicate code.
  • Uses set_url_scheme()
  • Updates to latest Gravatar supported subdomains.
  • Argument to add classes to the returned img element.
  • Argument to force the return of the img element, ignoring the show_avatars option.
  • Fixes some edge case bugs
    • Non integer and non-positive sizes
    • Negative user IDs
    • Comments by users who's accounts have since been deleted

Implementation Notes

With the patched filters, I don't think get_avatar_url() needs to be pluggable.

Both functions now accept a single $args array parameter instead of many parameters. This makes it easier for plugins to extend the functionality or replace it with a service that doesn't have the same set of parameters.

In the attached patch, get_avatar() respects the show_avatars option (as it does currently) unless the new force_display argument is true. In contrast, get_avatar_url() ignores the show_avatars option and always returns a URL if it can.

If the default avatar is a URL, get_avatar() currently appends the requested size to the default URL as an s={$size} query string parameter. In the patch, this behavior is removed since such a parameter is not needed (and is ignored) by Gravatar when serving default images. In theory, that removal could break some avatar-substituting plugins. I'm ambivalent: removing it cleans up the code a bit, but we can add it back.

If get_avatar() can't figure out an email address to hash, it currently uses unknown@gravatar.com, yielding URLs like: http://www.gravatar.com/avatar/ad516503a11cd5ca435acc9bb6523536?s=96. The attached patch instead does not include any hash in the URL for these cases: http://www.gravatar.com/avatar/?s=96.

If get_avatar() can't figure out an email address to hash but has a default image URL, it currently uses that URL as the src of the img: src="http://example.com/default.jpg". The attached patch instead sends that URL through Gravatar so that it can be sized correctly: src="http://www.gravatar.com/avatar/?s=96&d=http://example.com/default.jpg", which could break some URLs (since the provided default URL must be accessible to Gravatar for the image to be served). I chose this behavior since it matches what happens when we can find an email address to hash, but when that hash has no associated Gravatar.

Getting a URL by MD5 as implemented requires you to pass "{$md5}@md5.gravatar.com" in the first parameter. I did this because some hashes will pass the is_numeric() test. Who can say? Maybe someday a WordPress site will have more than 10 nonillion user accounts :)

In addition to the URL returned by get_avatar_url(), get_avatar() needs some other information calculated by get_avatar_url(): the sanitized avatar size, whether a potential avatar match was found, and potentially more by/for the various filters. Rather than calculating things twice or adding an intermediary function, get_avatar_url()'s args array accepts a 'processed_args' item, which should be passed by reference. get_avatar_url() fills that reference with the processed args/flags, and get_avatar() reads it.

I chose to implement this "out variable" as $args['processed_args'] rather than get_avatar_url( $id_or_email, $args, &$processed_args ) for selfish reasons: versions of Jetpack have shipped with code like:

get_avatar_url( 1, 96, '' );

which will generate a Fatal Error if get_avatar_url()'s signature has a reference as the third parameter.

mdawaffe18 months ago

comment:13 follow-up: cliffseal14 months ago

I was working with this today, but went a little different route by adding an extra parameter to the get_avatar function allowing you to just return the URL. If this has been explored already, forgive me, but I couldn't find any discussion on it. Thoughts?

(Excuse my failure to space properly, please.)

Version 0, edited 14 months ago by cliffseal (next)

cliffseal14 months ago

Add parameter to get_avatar

comment:14 in reply to: ↑ 13 ; follow-up: pathawks14 months ago

Replying to cliffseal:

I was working with this today, but went a little different route by adding an extra parameter to the get_avatar function allowing you to just return the URL.

The biggest problem I see with this approach is that, because get_avatar is pluggable, some plugins replace the entire function.

If get_avatar has been plugged with a version that does not support returning just the URL when given an extra paramater, it will break anything that relies on that behavior.

Additionally, this seems inconsistent with how functions like wp_get_attachment_image and wp_get_attachment_image_src work, with one function returning the whole <img> tag and another returning just the URL.

comment:15 in reply to: ↑ 14 ; follow-up: cliffseal14 months ago

Replying to pathawks:

If get_avatar has been plugged with a version that does not support returning just the URL when given an extra paramater, it will break anything that relies on that behavior.

I wasn't aware this would happen. It doesn't occur in any of my tests and seems unintuitive, but it's quite possible you know something I don't.

Additionally, this seems inconsistent with how functions like wp_get_attachment_image and wp_get_attachment_image_src work, with one function returning the whole <img> tag and another returning just the URL.

True, but neither of those functions are pluggable, so I'm not sure if the correlation should determine the method.

Regardless, sharing what worked for me here. I'll be pleased with whatever the best solution is.

comment:16 in reply to: ↑ 15 ; follow-up: pathawks14 months ago

Replying to cliffseal:

Replying to pathawks:

If get_avatar has been plugged with a version that does not support returning just the URL when given an extra paramater, it will break anything that relies on that behavior.

It doesn't occur in any of my tests

Of course it doesn't; you're working with a modified version of get_avatar :)

The problem comes when a plugin has replaced get_avatar completely. In that case, no updates that have been made to core's get_avatar will be reflected.
In this case, get_avatar would never return just a URL, because it was merely designed to replace the legacy get_avatar.

Now, the problem comes when a plugin expects to be able to retrieve a URL from get_avatar. As you can see, it wouldn't be possible in this senario. If the new plugin is expecting just a URL to be returned and instead is given an <img> tag, Bad Things will happen. If the plugged get_avatar is passed more parameters than it knows what to do with, Bad Things might happen.

comment:17 in reply to: ↑ 16 ; follow-up: cliffseal14 months ago

Replying to pathawks:

Of course it doesn't; you're working with a modified version of get_avatar :)

Just to be super clear here: no, I'm not. I kept the current version of get_avatar in pluggable.php and overrode it in a plugin for testing.

I think I see what you're saying, though. Plugins conflicts could occur if one plugin is overriding the function and another plugin is trying to call the updated version of the function—but, it would seem any decent plugin author would check for this error before proceeding.

comment:18 in reply to: ↑ 17 pathawks14 months ago

Replying to cliffseal:

Replying to pathawks:

Of course it doesn't; you're working with a modified version of get_avatar :)

Just to be super clear here: no, I'm not. I kept the current version of get_avatar in pluggable.php and overrode it in a plugin for testing.

Sorry, should have been more clear. WordPress is using a modified version of get_avatar, at any rate.

It would seem any decent plugin author would check for this error before proceeding.

If that's the case, then what would be the point in changing things?
If I will have to, not only provide a way to extract the URL from the returned <img> tag in these circumstances, but also check to see if the output from get_avatar is a URL or a tag, we actually end up with far more code than what we would currently need.

No, it seems that, if anything changes, a new function should be added called get_avatar_url or whatever, and get_avatar should be left alone. I see absolutely no advantage from trying to shim totally different functionality into an existing function, which currently does it's job well enough.

comment:19 DrewAPicture14 months ago

  • Cc xoodrew@… added

comment:20 rodrigosprimo10 months ago

  • Cc rodrigosprimo@… added

comment:21 in reply to: ↑ 12 pathawks9 months ago

Replying to mdawaffe:

I have packaged mdawaffe's patches into a plugin, if that makes it easier for others to test.

Last edited 9 months ago by pathawks (previous) (diff)

comment:22 rmccue9 months ago

I've included this as a separate function in my JSON API plugin; this is ripe for core inclusion in some form. I think a separate function would be a better choice here.

comment:23 alex-ye9 months ago

  • Cc nashwan.doaqan@… added

pento6 months ago

comment:24 pento6 months ago

attachment:21195.diff updates mdawaffe's patch to apply cleanly against trunk.

pento6 months ago

comment:25 follow-up: pento6 months ago

attachment:21195.2.diff​ updates the inline docs to more closely resemble the new standard.

comment:26 in reply to: ↑ 25 rmccue6 months ago

Replying to pento:

attachment:21195.2.diff​ updates the inline docs to more closely resemble the new standard.

I want me some of this.

I think your case statements should be indented a block, as with the } in the default PHPDoc.

comment:27 follow-up: wonderboymusic6 months ago

I use the same style for cases, which is the PEAR standard and the style that validates in jslint

pento6 months ago

comment:28 follow-up: pento6 months ago

attachment:21195.4.diff​ fixes the case and PHPDoc indentation.

Looking through a handful of core files, this seems to be the correct indentation for case statements (thought there's variation over whether there should be a space before the : or not).

comment:29 in reply to: ↑ 27 rmccue6 months ago

Replying to wonderboymusic:

I use the same style for cases, which is the PEAR standard and the style that validates in jslint

We should codify this in the standard. Here's the stats for *.php:

  • ^(\t+)switch.*\n\1case => 17 matches
  • ^(\t+)switch.*\n\1\tcase => 278 matches

And for `*.js*:

  • ^(\t+)switch.*\n\1case => 2 matches (both in nav-menu.js)
  • ^(\t+)switch.*\n\1\tcase => 18 matches
Last edited 6 months ago by rmccue (previous) (diff)

comment:30 in reply to: ↑ 28 rmccue6 months ago

Replying to pento:

attachment:21195.4.diff​ fixes the case and PHPDoc indentation.

Looking through a handful of core files, this seems to be the correct indentation for case statements (thought there's variation over whether there should be a space before the : or not).

For *.php:

  • case .*[^ ]: (no whitespace) => 2339 matches
  • case .* : (whitespace) => 286 matches

(This is less of a standard though; it's more about the context. We might want to standardise this too though.)

comment:31 follow-up: dd326 months ago

function get_avatar( $id_or_email, $args = null )

I don't think we can change the signature like that since it's a pluggable function, adding optional parameters is OK if the calling code can deal with it not being respected, but we should attempt to limit any in/out behaviour of the existing function.

Moving the entire contents of the function to a get_avatar_url() function is perfectly fine though, just have to remember that anyone who has a pluggable get_avatar() at present will suddenly have their functions alterations not respected.

comment:32 in reply to: ↑ 31 mdawaffe6 months ago

Replying to dd32:

I don't think we can change the signature like that since it's a pluggable function

The patched function can accept the old signature, but I see what you're saying. If we change the signature, new calls in core and plugins will use the new signature, which will break for sites which have a plugged version with the old signature.

pento6 months ago

comment:33 pento6 months ago

attachment:21195.5.diff

  • Changes get_avatar() back to the old signature, with a new optional fifth $args argument, and changes the PHPDoc to match
  • Re-adds some arguments that were accidentally removed from the PHPDoc for get_avatar()
  • Fixes a PHP warning when there's no email hash available

comment:34 rachelbaker5 months ago

  • Cc rachel@… added

comment:36 ircbot6 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by GaryJ. View the logs.

Note: See TracTickets for help on using tickets.