Fix: Adapt to More Current Avatar Filters

[dinhtungdu]

Description of the Change

• Switch to pre_get_avatar_data filter.
• Remove get_avatar function that overrides the core one.
• Add $args parameter to get_simple_local_avatar function.
• Remove Simple_Local_Avatars::get_avatar method. ~> added back for backward compat.
• Add Simple_Local_Avatars::get_avatar_data() method.
• Add Simple_Local_Avatars::get_simple_local_avatar_url() method.
• Add Simple_Local_Avatars::get_default_avatar_url() method.
• Fix coding standard issues.

Benefits

In fact, we only need to filter the url of the avatar, not the avatar
img element. So instead of hooking to get_avatar filter and overriding
the get_avatar function, we hook to pre_get_avatar_data filter to
manipulate the avatar url and let the core handle the rest. This reduces
duplication code and minimizes effort to follow core changes.

Possible Drawbacks

We need to care about/check for backward compatibility issues.

Verification Process

Check if avatars display correctly when Local Avatars Only is enabled/disabled.

Checklist:

• I have read the CONTRIBUTING document.
• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my change.
• All new and existing tests passed.

Applicable Issues

Closes #39
Relates #19

[jeffpaul]

Pinging @dkotter and @ryanwelcher on this to see if they can review this PR.

[jeffpaul]

@heyjones - I wanted to check and see if you'd mind giving this PR a review in relation to your PR to see if this approach might also work for your use?

[heyjones]

Sorry for the delay... it maintains the class args I'm passing through the theme!

@dinhtungdu I'd recommend possibly running $this->options through wp_parse_args() as I had a PHP notice locally since I didn't have the only option set:

Notice: Undefined index: only ... on line 70

Aside from that, @jeffpaul as long as you don't see this causing any issues with backwards capability I think it's a really clever solution!

[dinhtungdu]

@heyjones, thanks for the suggestion, I fixed it in 00a8362

[MikeNGarrett]

I really like this method better than continuing support via a pluggable function.

[lincolnlemos]

This is what I was looking for! Thanks 👍

[MikeNGarrett]

Is there an expected timeline for when this PR will be released?

[jeffpaul]

@MikeNGarrett one item I'd like to see is some test coverage, be that unit tests and/or WP Acceptance tests, before pushing forward with another release. If someone is able to add a PR to increase (heck, add) test coverage then that'd be wonderful.

[dkotter]

I know this was just copied from the current codebase but we should make sure any if statements here have brackets { } at the end of the lines.

[dkotter]

Same as above

[dinhtungdu]

@dkotter Currently we have a lot of coding style issues, I think it's better to address them in a separated PR after this is merged, does it work for you?

[Log1x]

any update on this?

[jeffpaul]

@Log1x we have a couple other plugins scheduled for releases ahead of Simple Local Avatars. Getting this PR through review and into a release is on our todo list, just not in the immediate future. If you have an urgent or important need for this specific change, then please reach out to me directly and we discuss... thanks!

[joshuaabenazer]

So while using Simple Local Avatars on a project recently, I just needed the image url rather than the whole image tag. I assumed that since get_avatar() works out of the box, the other functions would also be intercepted similarly. The case in question here is the get_avatar_url() function.
We can use the pre_get_avatar_data filter in get_avatar_data() to short-circuit the function and updated the URL or maybe just let it go through the whole function and update the args accordingly in the get_avatar_data filter or maybe just get_avatar_url filter

I feel that if we update the URL at that stage, we may not be required to override the pluggable get_avatar() function. I haven't spent much time in this and I could be wrong, but I feel this is the right ticket to look into this and see if we can take over all the avatar related functions in core, just so that we are able to use them as is while the plugin does the rest behind the scenes.

[ammon-lockwood]

@dinhtungdu Thank you, I'm seeing the right URL as well if I use the get_avatar_url and the get_avatar functions. It appears the issue is between this plugin and the wpgraphql plugin which expects to have the found_avatar set. In the code in #40 when we find the local avatar and set it in the $args on line 81 we should also set the found_avatar to true right after on line 82 so that anything looking for that argument can key off it. In our case wpgraphql doesn't return anything from the avatar data unless found_avatar is true. WordPress sets it to true when the default Gravatar is set and so we should set this argument as well when we find the local avatar. I propose the get_avatar_data function look like this:

        /**
	 * Filter avatar data early to add avatar url if needed. This filter hooks
	 * before Gravatar setup to prevent wasted requests.
	 *
	 * @since 2.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.
	 */
	public function get_avatar_data( $args, $id_or_email ) {
		$simple_local_avatar_url = $this->get_simple_local_avatar_url( $id_or_email, $args['size'] );
		if( $simple_local_avatar_url ) {
			$args['url'] = $simple_local_avatar_url;
			$args['found_avatar'] = true;
		}

		// Local only mode
		if( ! $simple_local_avatar_url && ! empty( $this->options['only'] ) ) {
			$args['url'] = $this->get_default_avatar_url( $args['size'] );
		}

		return $args;
	}

[dinhtungdu]

@sumnercreations Can you please test the latest commit and see if your issue is resolved? https://github.com/10up/simple-local-avatars/tree/fix/39

[jeffpaul]

After internal discussion and review, it was agreed this was ready for merge so I'm going to get this in and start looking towards what else remains in a 2.2.0 release.