Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds local avatar support #29

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

BE-Webdesign
Copy link
Contributor

This was tested using https://wordpress.org/plugins/wp-user-avatar/,
which has 200,000 active installs. This will handle local avatars that
are using get_avatar() for functionality. Solves #14, for most cases.
There are likely edge cases where this may not work, but for the most
part, I imagine this will add support for local avatars.

This was tested using https://wordpress.org/plugins/wp-user-avatar/,
which has 200,000 active installs.  This will handle local avatars that
are using get_avatar() for functionality.  Solves Automattic#14, for most cases.
There are likely edge cases where this may not work, but for the most
part, I imagine this will add support for local avatars.
@georgestephanis
Copy link
Member

  1. If we did this, it would probably be better to just regex out the image src.
  2. But, we shouldn't do this, as core has a nifty function called get_avatar_url() since 4.2.0 -- and this patch should just call that.

@BE-Webdesign
Copy link
Contributor Author

BE-Webdesign commented Mar 24, 2016

Thanks for the info. I will check that out and revise accordingly

@BE-Webdesign
Copy link
Contributor Author

BE-Webdesign commented Mar 29, 2016

I checked out get_avatar_url() and it never calls any of the hooks relating to avatars, which plugins are using to create local avatars. It just ends up returning false for any locally set avatars. Could you please elaborate more on how you propose we use get_avatar_url() and what I am overlooking.

As far as DOM parsing vs. regex it is probably a sound idea to use regex, since it is only an image tag being parsed but I always personally feel safer using DOM as it is less error prone.

If you could expand a bit on your thoughts about using get_avatar_url(), it would be greatly appreciated. Once that is settled, I will switch this over to regex and things should be good to go!

@georgestephanis
Copy link
Member

get_avatar_url() calls get_avatar_data() which applies two filters -- get_avatar_url and get_avatar_data on its data just before it returns.

    /**
     * Filter the avatar URL.
     *
     * @since 4.2.0
     *
     * @param string $url         The URL of the avatar.
     * @param mixed  $id_or_email The Gravatar to retrieve. Accepts a user_id, gravatar md5 hash,
     *                            user email, WP_User object, WP_Post object, or WP_Comment object.
     * @param array  $args        Arguments passed to get_avatar_data(), after processing.
     */
    $args['url'] = apply_filters( 'get_avatar_url', $url, $id_or_email, $args );

    /**
     * Filter the avatar data.
     *
     * @since 4.2.0
     *
     * @param array  $args        Arguments passed to get_avatar_data(), after processing.
     * @param mixed  $id_or_email The Gravatar to retrieve. Accepts a user_id, gravatar md5 hash,
     *                            user email, WP_User object, WP_Post object, or WP_Comment object.
     */
    return apply_filters( 'get_avatar_data', $args, $id_or_email );

Any plugin that adds an option for local avatars should be using one or both of these filters to make WordPress aware of them. And then any plugin that uses get_avatar_url() or get_avatar_data() as functions will 'catch' the new avatars passed in by the first plugin.

@BE-Webdesign
Copy link
Contributor Author

BE-Webdesign commented Mar 30, 2016

Right, I agree that is the better implementation and how a plugin should interact with avatars. However, this https://wordpress.org/plugins/wp-user-avatar/, which has 200,000 active installs, uses the hook get_avatar. (probably should have made that more clear)

Would it be acceptable to use get_avatar_url() first and then if the avatar is not already found at that point use get_avatar(), as a sort of backwards compatibility kind of thing? If not, I'll switch it to only using get_avatar_url().

@georgestephanis
Copy link
Member

Honestly, I have zero interest in adding workarounds to o2 to support outdated plugins that choose not to use filters that have been available and known for about a year now (since 4.2), regardless of the size of their user base. If a plugin is doing it wrong, we shouldn't also do it wrong just to follow suit.

Merging master to keep up to date.
…avatar-support

# Conflicts:
#	js/collections/users.js
Changed to use only get_avatar_url().  This is the proper function to
use and agreement was reached for not adding backwards compatibility for
get_avatar hook.
@BE-Webdesign
Copy link
Contributor Author

BE-Webdesign commented Apr 1, 2016

Let me know if there are other changes to be made. Thank you for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants