WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 4 months ago

Last modified 2 months ago

#21195 closed feature request (fixed)

get_avatar_url

Reported by: pathawks Owned by:
Milestone: 4.2 Priority: normal
Severity: normal Version: 2.5
Component: General Keywords:
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 (20)

21195-get_avatar_url.diff (2.8 KB) - added by batmoo 3 years ago.
21195.3.diff (12.7 KB) - added by mdawaffe 3 years ago.
21195-ut.3.diff (20.3 KB) - added by mdawaffe 3 years ago.
21195-param.diff (1.5 KB) - added by cliffseal 2 years ago.
Add parameter to get_avatar
21195.diff (12.8 KB) - added by pento 19 months ago.
21195.2.diff (13.1 KB) - added by pento 19 months ago.
21195.4.diff (13.2 KB) - added by pento 19 months ago.
21195.5.diff (13.1 KB) - added by pento 19 months ago.
21195.6.diff (16.0 KB) - added by pathawks 10 months ago.
Refresh patch
21195.7.diff (15.9 KB) - added by pathawks 10 months ago.
Refresh
21195.patch (1005 bytes) - added by rachelbaker 9 months ago.
Current approach used in the JSON REST API
21195.8.diff (16.3 KB) - added by pento 8 months ago.
21195.9.diff (16.6 KB) - added by DrewAPicture 8 months ago.
docs pass 2
21195.10.diff (24.9 KB) - added by pento 8 months ago.
21195.11.diff (24.9 KB) - added by pento 5 months ago.
21195.12.diff (24.9 KB) - added by pento 5 months ago.
21195.13.diff (27.1 KB) - added by pento 5 months ago.
21195.14.diff (25.9 KB) - added by pento 5 months ago.
21195.15.diff (39.5 KB) - added by pento 5 months ago.
21195.16.diff (1.1 KB) - added by valendesigns 5 months ago.

Download all attachments as: .zip

Change History (96)

comment:1 @c3mdigital3 years 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 @pathawks3 years 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 @Ipstenu3 years 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 @toscho3 years ago

  • Cc info@… added

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

comment:5 @readysetawesome3 years 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 @batmoo3 years ago

  • Cc batmoo@… added

comment:7 @batmoo3 years ago

  • Keywords has-patch added

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

comment:8 @pathawks3 years ago

Beautiful!

comment:9 @greenshady3 years ago

  • Cc justin@… added

comment:10 @mdgl3 years 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.

@mdawaffe3 years ago

comment:12 follow-up: @mdawaffe3 years 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.

@mdawaffe3 years ago

comment:13 follow-up: @cliffseal2 years 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?

Last edited 2 years ago by cliffseal (previous) (diff)

@cliffseal2 years ago

Add parameter to get_avatar

comment:14 in reply to: ↑ 13 ; follow-up: @pathawks2 years 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: @cliffseal2 years 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: @pathawks2 years 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: @cliffseal2 years 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 @pathawks2 years 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 @DrewAPicture2 years ago

  • Cc xoodrew@… added

comment:20 @rodrigosprimo2 years ago

  • Cc rodrigosprimo@… added

comment:21 in reply to: ↑ 12 @pathawks22 months ago

Replying to mdawaffe:

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

Version 0, edited 22 months ago by pathawks (next)

comment:22 @rmccue22 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-ye22 months ago

  • Cc nashwan.doaqan@… added

@pento19 months ago

comment:24 @pento19 months ago

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

@pento19 months ago

comment:25 follow-up: @pento19 months ago

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

comment:26 in reply to: ↑ 25 @rmccue19 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: @wonderboymusic19 months ago

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

@pento19 months ago

comment:28 follow-up: @pento19 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 @rmccue19 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 19 months ago by rmccue (previous) (diff)

comment:30 in reply to: ↑ 28 @rmccue19 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: @dd3219 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 @mdawaffe19 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.

@pento19 months ago

comment:33 @pento19 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 @rachelbaker18 months ago

  • Cc rachel@… added

comment:36 @ircbot15 months ago

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

comment:37 @wonderboymusic12 months ago

  • Keywords needs-refresh added
  • Milestone changed from Awaiting Review to Future Release

If someone refreshes this, we can look and possibly move to 4.0

comment:38 @rmccue12 months ago

For reference, here's how we do it in WP-API currently. (Basically, regex on HTML, bleh.)

@pathawks10 months ago

Refresh patch

@pathawks10 months ago

Refresh

comment:39 @pathawks10 months ago

  • Keywords needs-refresh removed

21195.7.diff Refreshes the patch.

What is next?

@rachelbaker9 months ago

Current approach used in the JSON REST API

comment:40 @rachelbaker9 months ago

  • Keywords 2nd-opinion added

comment:41 @SergeyBiryukov9 months ago

  • Keywords 2nd-opinion removed

The approach in 21195.patch looks more like a workaround. We don't want to add new pluggable functions, so 21195.7.diff seems like the way to go.

#22650 would benefit from this.

comment:42 @pathawks9 months ago

Because get_avatar is pluggable, we cannot safely assume that it will always even return an <img> tag. It could, for instance, return a <div> with a backgroud set to an avatar image, or something silly.

Because of the (albeit remote) possibility of this, running a regex on the return from get_avatar only works well when you are certain that nothing odd has plugged get_avatar. A safe assumption for WordPress.com, perhaps not a safe assumption for the rest of the world.

What would be the possible advantage of adding a new function to pluggable.php? Why not just send output (Gravatar URL) through a filter?

comment:43 @wonderboymusic8 months ago

  • Milestone changed from Future Release to 4.1

@pento, @mdawaffe, @SergeyBiryukov - one of you please remind yourself why you fell in love with get_avatar_url() in the first place

comment:44 @wonderboymusic8 months ago

#27336 was marked as a duplicate.

@pento8 months ago

comment:45 @pento8 months ago

  • Keywords needs-unit-tests added

That was a fun walk down memory lane. 21195.8.diff:

  • Updates the patch to apply cleanly against trunk
  • Re-adds some filter PHPDocs that had been removed
  • Adds new PHPDocs for new filters
  • Adds PHPDocs for new parameters to old filters
  • Fixes a bunch of space/tab indent issues
  • Fixes some minor coding style issues

It wouldn't kill us to put some unit tests on this, too. I'll do that sometime later today, if someone doesn't beat me to it.

@DrewAPicture - I couldn't find anything in the doc standards about marking when a new parameter was added, just a couple of examples that I managed to grep for the source. I went with the (since 4.1.0) style, please correct me if there's a better way. :-)

comment:46 @ircbot8 months ago

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

@DrewAPicture8 months ago

docs pass 2

comment:47 @DrewAPicture8 months ago

21195.9.diff brings the docs in-line with the standards, retaining most of the awesome background information supplied by @pento in .8.

@pento8 months ago

comment:48 @pento8 months ago

  • Keywords needs-unit-tests removed

21195.10.diff:

  • Adds unit tests for get_avatar() and get_avatar_url()
  • Fixes get_avatar_url() failing if it's passed the array version of a user/post/comment object
  • Fixes get_avatar_url()'s $args parameter not being passed as a reference
  • Fixes get_avatar_url() returning a broken URL if it couldn't figure out the email hash
  • Tweaks the docs just a little more

comment:49 @DrewAPicture7 months ago

#21930 was marked as a duplicate.

comment:50 @ocean906 months ago

  • Milestone changed from 4.1 to Future Release

comment:51 in reply to: ↑ description @bgerhardt14885 months ago

Replying to pathawks:

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.

@pento5 months ago

comment:52 @pento5 months ago

  • Milestone changed from Future Release to 4.2

21195.11.diff updates the patch to apply cleanly against trunk, and updates the @since version numbers.

@pento5 months ago

comment:53 @pento5 months ago

21195.12.diff replaces all of the apply_filters_ref_array() calls with apply_filter() - I was unable to find a difference between the behaviour of them.

comment:54 @slackbot5 months ago

This ticket was mentioned in Slack in #core by rachelbaker. View the logs.

@pento5 months ago

comment:55 @pento5 months ago

So, I realised the purpose of the apply_filters_ref_array() - to allow filters to change the $args.

Because references make everyone sad, 21195.13.diff shuffles all of the $args processing into a new get_avatar_data() function, with filters specifically for filtering the data. get_avatar_url() is now just a little wrapper around this function.

This has the added bonus of allowing us to remove the $processed_args references, as get_avatar() is able to call get_avatar_data(), and get all of the processed args directly.

@pento5 months ago

comment:56 @pento5 months ago

In 21195.14.diff:

  • Drop support for the array versions of WP_User / WP_Post objects - it's a new feature so we don't need to support the old way of doing things.
  • Moved the get_avatar_url filter to get_avatar_data().
  • Dropped $original_args from all of the filters, it didn't really have a practical purpose.
Last edited 5 months ago by pento (previous) (diff)

@pento5 months ago

comment:57 @pento5 months ago

Does anyone have opinions on 21195.15.diff, which deprecates the get_avatar() pluggable function, in favour of wp_get_avatar()? (Currently just a copy/pasta, but we could make this code a lot neater if we didn't need to care about function signatures, for starters.)

comment:58 @pento5 months ago

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

In 31107:

Add get_avatar_url(), for retrieving just the URL of an avatar, rather than the entire <img> tag that get_avatar() produces.

Unlike get_avatar(), get_avatar_url() is not pluggable. It can be extended/or modified through the new filters included.

Fixes #21195.

Props mdawaffe, pento, pathawks, DrewAPicture

@valendesigns5 months ago

comment:59 follow-up: @valendesigns5 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

When viewing options-discussion.php all generated avatars are replaced with your personal avatar. Patch 21195.16.diff fixes this by using the new $args array and setting force_default to true. I also removed some filters that were added to force avatars to display on options-discussion.php and replaced them by setting the force_display option to true.

Unfortunately, another side effect of the newly implemented get_avatar means that the Mr. WordPress avatar is no longer showing in comments on the front end because it does not have an email address attached to the comment. That was not the behavior previously and we should fix this regression.

comment:60 @obenland4 months ago

valendesigns, the next line after the avatar is retrieved, where the img src is being replaced, can also be removed.

Last edited 4 months ago by obenland (previous) (diff)

comment:61 in reply to: ↑ 59 ; follow-up: @nacin4 months ago

Replying to valendesigns:

When viewing options-discussion.php all generated avatars are replaced with your personal avatar. Patch 21195.16.diff fixes this by using the new $args array and setting force_default to true.

Why did the existing code break? preg_replace("/src='(.+?)'/", "src='\$1&amp;forcedefault=1'", $avatar);

I also removed some filters that were added to force avatars to display on options-discussion.php and replaced them by setting the force_display option to true.

One problem with the approach we have here is there's no real way of knowing if these arguments work for get_avatar().

I'm thinking about an alternate approach that looks something like this:

<?php
function wp_get_avatar( ... ) {
    if ( defined( 'PLUGGED_GET_AVATAR' ) && empty( $args['force_core_function'] ) ) {
        return get_avatar( ... );
    }
    // else, the rest of the function
}

# pluggable-deprecated.php
if ( function_exists( 'get_avatar' ) ) {
    define( 'PLUGGED_GET_AVATAR', true );
    _deprecated_function( 'get_avatar', ... ); // optional. need to pass a message of some sort.
} else {
    function get_avatar( ... ) {
        return wp_get_avatar();
    }
}

That way we can finally banish get_avatar(), use wp_get_avatar() everywhere in core, force it to use core functionality when we need it, but not break anything overriding get_avatar() deliberately. We could possibly use this as a template for deprecating, eventually, all pluggable functions. Any thoughts here?

Unfortunately, another side effect of the newly implemented get_avatar means that the Mr. WordPress avatar is no longer showing in comments on the front end because it does not have an email address attached to the comment. That was not the behavior previously and we should fix this regression.

Indeed, this is broken.

comment:62 @slackbot4 months ago

This ticket was mentioned in Slack in #core by nacin. View the logs.

comment:63 @obenland4 months ago

Why did the existing code break?

It went from src='' to src="".

comment:64 in reply to: ↑ 61 @jeremyfelt4 months ago

Replying to nacin:

We could possibly use this as a template for deprecating, eventually, all pluggable functions. Any thoughts here?

In the past, we've deprecated without making the promise that core will continue to use the plugged version [18513]. If all instances in core would use the force_core_function argument, it seems unnecessary to continue looking for a plugged get_avatar().

That said, I'm also not familiar with issues that may have arisen from previous deprecated pluggables. If there are reasons for core to use wp_get_avatar() without forcing, then the above code looks sane for maintaining back-compat.

Looking around, #19615 could also be a good test (mostly because it's open) for deprecating pluggable get_currentuserinfo().

comment:65 follow-up: @nacin4 months ago

In [18513]'s case, get_user_by() is also pluggable. Presumably, if one is plugging the user_by functions, they're plugging all of them.

Frankly, I care very little about the promise of most pluggable functions. The auth ones, sure. But then there's get_avatar(), a fairly high-level function that, if I had to take a guess, is commonly overridden to use something other than Gravatar. I wish it were never pluggable and that it simply had just the filter (which was introduced at the same time).

I don't know — maybe we can get away with ditching it, deprecating it, and just using a new wp_get_avatar(), and anyone who is currently overriding get_avatar() just gets screwed and told they should have been using the filter.

comment:66 in reply to: ↑ 65 @jeremyfelt4 months ago

Replying to nacin:

I don't know — maybe we can get away with ditching it, deprecating it, and just using a new wp_get_avatar(), and anyone who is currently overriding get_avatar() just gets screwed and told they should have been using the filter.

Would there be a wp_get_avatar() use in core where we wouldn't use force_core_function to override?

Though... providing the check for the pluggable at least announces the deprecation, even if core itself skips it. This could help push developers to transition more than just moving to wp_get_avatar() and leaving behind a deprecation notice that may never fire.

comment:67 @pento4 months ago

In 31152:

In get_avatar(), revert the <img> tag attributes to using single quotes, instead of double quotes. This behaviour was changed in [31107], but caused problems for code that attempted to parse the <img> tag.

See #21195

comment:68 @pento4 months ago

In 31153:

In get_avatar_data(), there's no need to return false if we couldn't find an avatar, as Gravatar can handle being given an empty email hash. This allows the default avatar to show when no email address is given.

See #21195

comment:69 @pento4 months ago

In 31154:

If get_avatar_data() is passed an empty value for the default avatar, we should be using the site's avatar_default option instead.

See #21195

comment:70 @dshanske4 months ago

Isn't this a chance, while reviewing get_avatar and get_avatar_url, to add built-in support for local avatars which was proposed as a feature plugin? I've always felt that polling an external service for local accounts seemed wrong.

comment:71 @pento4 months ago

  • Keywords has-patch removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Adding local avatar support is waaaaay outside of the scope of this ticket. :-)

The best place for that would be to get the proposed feature plugin up and running, then we can look at merging it later.

comment:72 @SergeyBiryukov3 months ago

In 31480:

Remove src from duplicate hook comments for get_avatar and get_avatar_data.

see #21195.

comment:73 @slackbot2 months ago

This ticket was mentioned in Slack in #core by jip. View the logs.

comment:74 follow-up: @pathawks2 months ago

This ticket was mentioned in Slack in #core by jip. View the logs.

Apparently it is impossible for a non-member to view the logs? Lovely.

comment:75 in reply to: ↑ 74 ; follow-up: @ocean902 months ago

Replying to pathawks:

Apparently it is impossible for a non-member to view the logs? Lovely.

You can be a member too, see https://make.wordpress.org/chat/.

comment:76 in reply to: ↑ 75 @pathawks2 months ago

Replying to ocean90:

You can be a member too, see https://make.wordpress.org/chat/.

Thanks missed that somehow.

Note: See TracTickets for help on using tickets.