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

Introduce a filter that short-circuits the avatar URL generation #237

Merged
merged 8 commits into from
Nov 23, 2023
Merged

Introduce a filter that short-circuits the avatar URL generation #237

merged 8 commits into from
Nov 23, 2023

Conversation

johnbillion
Copy link
Contributor

@johnbillion johnbillion commented Sep 21, 2023

Description of the Change

When a dynamic image resizing service (such as Tachyon or Photon) is in use, Simple Local Avatars provides no way for the URL of an avatar of a given size to be short-circuited so as to avoid its own dynamic resizing that happens via a WP_Image_Editor instance.

This change introduces a filter to short-circuit the URL for an avatar (once it's been determined that a "full" size exists), allowing a dynamic image resizing service to return its own scaled version of the user's image.

How to test the Change

Here's a simplified example of how this filter could be used on a site that uses Tachyon:

add_filter(
	'pre_simple_local_avatar_url',
	function( $url, $user_id, $size, $local_avatars ) {
		return tachyon_url( $local_avatars['full'], [ 'resize' => "{$size},{$size}" ] );
	},
	10,
	4
);

Changelog Entry

Added - pre_simple_local_avatar_url filter to allow an avatar image to be short-circuited before Simple Local Avatars processes it.

Credits

@johnbillion

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@jeffpaul
Copy link
Member

Thanks @johnbillion, that usage makes sense to me. Tagging @faisal-alvi for code review/testing to see if there are any concerns here before moving towards merge/release.

@peterwilsoncc
Copy link
Contributor

@faisal-alvi I have a copy of Tachyon set up locally if you need guidance on getting it going/want me to test.

@faisal-alvi
Copy link
Member

@peterwilsoncc it would be great if you test + code review this as I do not have Tachyon set up locally. Thanks! ❤️

Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests well and works as described.

I've just raised a discussion internally as to whether the plugin should switch to using wp_get_attachment_image_src()[0] and storing dynamically resized images in the image post meta. That will allow the plugin to just work with Tachyon, Photon and other services using the image_downsize filter to dynamically resize images.

includes/class-simple-local-avatars.php Outdated Show resolved Hide resolved
@faisal-alvi faisal-alvi removed their request for review October 2, 2023 12:01
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnbillion We've considered the options and decided to put this in for now rather than switch to the proper media functions. The team needs to do a little more investigating of the implications for multisite installations using shared avatars.

I think that would be to be esc_url_raw anyway

Yeah, it probably should be but we use esc_url() elsewhere in the function so it's intended for printing rather than getting.. It's probably too late to make the change now.

@peterwilsoncc peterwilsoncc merged commit e09ab84 into 10up:develop Nov 23, 2023
@johnbillion johnbillion deleted the get-url-filter branch November 23, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants