$id_or_email parameter in get_avatar filter needs to be more concrete

[dikiy_forester]

In the get_avatar and pre_get_avatar filters there is a parameter $id_or_email which documented as:

The Gravatar to retrieve. Accepts a user_id, gravatar md5 hash, user email, WP_User object, WP_Post object, or WP_Comment object.

There might be anything, but how to get user from it?

So, in filter callback function, developer have to check all of these types to determine the actual user identifier.

Like here https://core.trac.wordpress.org/browser/tags/4.8.1/src/wp-includes/link-template.php?rev=41211#L3923-L3965

Would be much more better to have more concrete value type in $id_or_email parameter (for example user ID) passed to get_avatar and pre_get_avatar filters. Really.

[knutsp]

A user or user_id cannot always be passed to the filters, because there may not be registered user in case of a comment.

In case an email is encountered in this parameter, it might be a commenting user that isn't logged in. You have to decide whether you want to look it up or not, depending on your needs.

I use something like this

<?php
if ( is_numeric( $id_or_email ) && $id_or_email > 0 ) {
    $user = get_user_by ( 'id', $id_or_email );
} elseif ( is_email( $id_or_email ) ) {
    $user = get_user_by( 'email', $id_or_email );
} elseif ( $id_or_email instanceof WP_Comment && $id_or_email->user_id > 0 ) {
    $user = get_user_by( 'id', $id_or_email->user_id );
} elseif ( $id_or_email instanceof WP_User && $id_or_email->ID > 0 ) {
    $user = $id_or_email;
} else {
    $user = false;
}
if ( $user ) {
    // Do somthing based on the fact that we have a logged in user, or an email mapped to a user
} else {
    // Do something based on $id_or_email being the email or gravatar hash of a not logged in commenter
}

If get_avatar() should do something like this for you, should it look up the email to a user or not?

Given the nature / signature of this function I think the "concrete" user (or not) should be left to the plugin or theme to decide, and hence, the filter parameters are ok.

[dikiy_forester]

Given the nature / signature of this function I think the "concrete" user (or not) should be left to the plugin or theme to decide, and hence, the filter parameters are ok.

Can't agree that theme or plugin should duplicate the code from https://core.trac.wordpress.org/browser/tags/4.8.1/src/wp-includes/link-template.php?rev=41211#L3923 just to been able to process the user identifier (or determine that there is no logged in user):

<?php
        if ( is_object( $id_or_email ) && isset( $id_or_email->comment_ID ) ) {
                $id_or_email = get_comment( $id_or_email );
        }

        // Process the user identifier.
        if ( is_numeric( $id_or_email ) ) {
                $user = get_user_by( 'id', absint( $id_or_email ) );
        } elseif ( is_string( $id_or_email ) ) {
                if ( strpos( $id_or_email, '@md5.gravatar.com' ) ) {
                        // md5 hash
                        list( $email_hash ) = explode( '@', $id_or_email );
                } else {
                        // email address
                        $email = $id_or_email;
                }
        } elseif ( $id_or_email instanceof WP_User ) {
                // User Object
                $user = $id_or_email;
        } elseif ( $id_or_email instanceof WP_Post ) {
                // Post Object
                $user = get_user_by( 'id', (int) $id_or_email->post_author );
        } elseif ( $id_or_email instanceof WP_Comment ) {
                /**
                 * Filters the list of allowed comment types for retrieving avatars.
                 *
                 * @since 3.0.0
                 *
                 * @param array $types An array of content types. Default only contains 'comment'.
                 */
                $allowed_comment_types = apply_filters( 'get_avatar_comment_types', array( 'comment' ) );
                if ( ! empty( $id_or_email->comment_type ) && ! in_array( $id_or_email->comment_type, (array) $allowed_comment_types ) ) {
                        $args['url'] = false;
                        /** This filter is documented in wp-includes/link-template.php */
                        return apply_filters( 'get_avatar_data', $args, $id_or_email );
                }

                if ( ! empty( $id_or_email->user_id ) ) {
                        $user = get_user_by( 'id', (int) $id_or_email->user_id );
                }
                if ( ( ! $user || is_wp_error( $user ) ) && ! empty( $id_or_email->comment_author_email ) ) {
                        $email = $id_or_email->comment_author_email;
                }
        }

At least this block of code might be extracted from the get_avatar_data() to a separate function.

As of original ticket topic, I still think that parameter $id_or_email needs to be concretized and contain the user id if available OR gravatar hash otherwise. So, developer can determine if it's a numeric value - it's user ID, otherwise it's a hash.

[SergeyBiryukov]

Replying to dikiy_forester:

At least this block of code might be extracted from the get_avatar_data() to a separate function.

Sounds good.

As of original ticket topic, I still think that parameter $id_or_email needs to be concretized and contain the user id if available OR gravatar hash otherwise. So, developer can determine if it's a numeric value - it's user ID, otherwise it's a hash.

Changing the type of an existing parameter is not really possible due to backwards compatibility concerns, especially for a pluggable function like get_avatar(), which could entirely be replaced with a custom implementation, but needs to maintain a compatible function signature.

[dikiy_forester]

Changing the type of an existing parameter is not really possible due to backwards compatibility concerns

This makes sense. Thank you.

So, the only one sensible way is to extract the processing identifier code from get_avatar_data() to a separate function.

Thanks.

[birgire]

I support this. I remember I stumbled up on this problem here:

​https://wordpress.stackexchange.com/questions/228870/custom-user-avatar-in-the-wordpress-users-listing/228913#228913

with similar thoughts for an attribute or a function.

I'm planning to look into this.

Here's a suggestion:

Take the similar approach as with the processed found_avatar argument and introduce found_user_id.

When user is found then found_user_is is the user id, else it's 0.

We note that processed arguments are not available in the pre_get_avatar_data filter.

We could additionally add a new function wp_parse_id_or_email( $id_or_email ) that would return an array like:

Array(
    [user_id] => 1
    [email] => user@example.org
    [email_hash] => 572c3489ea700045927076136a969e27
    [context] => user_id
)

Array
(
    [user_id] => 0
    [email] => user@example.org
    [email_hash] => 572c3489ea700045927076136a969e27
    [context] => email
)

Array
(

    [user_id] => 0
    [email] =>
    [email_hash] => 572c3489ea700045927076136a969e27
    [context] => email-md5-gravatar
)

Array
(
    [user_id] => 1
    [email] => user@example.org
    [email_hash] => 572c3489ea700045927076136a969e27
    [context] => post
)

Array
(
    [user_id] => 1
    [email] => user@example.org
    [email_hash] => c52e94b32934ec08c573b1c850a7a8a3
    [context] =>  comment-allowed-type
)
Array
(
    [user_id] => 0
    [email] =>
    [email_hash] =>
    [context] => comment-disallowed-type
)

where the possible input context values would be

'user_id', 'email-md5-gravatar', 'email', 'user', 'post', 'comment-allowed-type', 'comment-disallowed-type'

The context could also be added as a processed argument aka found_avatar.

What do you think of this plan?

[birgire]

In 41544.diff​ is a first attempt to introduce a new wp_parse_id_or_email() function.

Included are tests.

If we want patch the new processed arguments, found_user_id and context, then we could use:

$parsed = wp_parse_id_or_email( $id_or_email );

$args['found_user_id'] =  $parsed['user_id'];
$args['context'] =  $parsed['context'];

within get_avatar_data(), with some minor adjustments.

[birgire]

In 41544.2.diff​ adjusted the post object input case for $id_or_email in the test.

[birgire]

In 41544.3.diff​ simplified the test comment creation and adjusted formatting according to the phpcs.