Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#32572 closed enhancement (wontfix)

Remove extra gravatar api call

Reported by: ravinderk's profile ravinderk Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Priority: normal
Severity: normal Version: 4.2
Component: Users Keywords: needs-testing has-patch
Focuses: performance Cc:

Description

get_avatar function is making two api call to fetch simple avatar and 2x avatar, we can save one extra api call by replacing s url query parameter by 2x size of current avatar size.

Attachments (5)

32572v1.patch (583 bytes) - added by ravinderk 9 years ago.
change under get_avatar function to remove extra api call
miqro-32572.patch (1.7 KB) - added by miqrogroove 9 years ago.
Restore avatar filters broken by previous patch.
32572v2.patch (1.7 KB) - added by ravinderk 9 years ago.
Fix for breaking gravatar functionality on pre_get_avatar_data hook
32572v3.patch (1.7 KB) - added by ravinderk 9 years ago.
Removing extra $url2x variable from patch
32572v4.patch (2.1 KB) - added by ravinderk 9 years ago.
set $argsurl2x? to false if current comment type is not in list of allowed comment types

Download all attachments as: .zip

Change History (29)

@ravinderk
9 years ago

change under get_avatar function to remove extra api call

#1 @johnbillion
9 years ago

  • Component changed from General to Users
  • Keywords has-patch needs-testing added
  • Milestone changed from Awaiting Review to 4.3
  • Version changed from trunk to 4.2

Thanks for the patch, ravinderk!

#2 @SergeyBiryukov
9 years ago

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

In 32702:

In get_avatar(), avoid a second get_avatar_data() call to get the 2x URL.

props ravinderk.
fixes #32572.

#3 @iseulde
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This could affect plugins that rely on the size parameter. The link could have a very different structure.

#4 @miqrogroove
9 years ago

  • Keywords has-patch removed

Agreeing with @iseulde here. The only way to avoid the second call is to restructure get_avatar_data() to return two or more URLs.

@miqrogroove
9 years ago

Restore avatar filters broken by previous patch.

#5 @miqrogroove
9 years ago

  • Keywords has-patch added

New patch restores filtering of the 2x url and allows plugins to supply their own args, though slightly different from how it works in 4.2.

#6 @ravinderk
9 years ago

@johnbillion @SergeyBiryukov
Thank for appreciation :)

@iseulde
Can you elaborate more, how can my patch affect plugin?
$url2x used to get image for double size of current avatar size, it will be always 2x of current size. in my patch i calculated $url2x just after $url that means i always have latest $args.

#7 @iseulde
9 years ago

The URL may not have an s parameter as there are filters in place so that a plugin can return a URL with a completely different structure.

#8 follow-up: @ravinderk
9 years ago

@iseulde
I did not understand, if url may not have s parameter then add_query_arg function will add 2x image size value to url and if it already exist then it will replace it with new 2x image size value.

Yes url can be different but it will be a gravatar api url in which s parameter is valid.

#9 in reply to: ↑ 8 ; follow-up: @miqrogroove
9 years ago

Replying to ravinderk:

Yes url can be different but it will be a gravatar api url in which s parameter is valid.

That is an invalid assumption.

#10 in reply to: ↑ 9 @ravinderk
9 years ago

Replying to miqrogroove:

That is an invalid assumption.

Let take a case where gravatar api changes and s parameter is not valid, in that case we have to rewrite my patch and get_avatar_data() function because they are both using s parameter in there url params.

$url_args = array(
		's' => $args['size'],
		'd' => $args['default'],
		'f' => $args['force_default'] ? 'y' : false,
		'r' => $args['rating'],
	);

	$url = sprintf( 'http://%d.gravatar.com/avatar/%s', $gravatar_server, $email_hash );

	$url = add_query_arg(
		rawurlencode_deep( array_filter( $url_args ) ),
		set_url_scheme( $url, $args['scheme'] )
	);


#12 @ravinderk
9 years ago

@iseulde
Thank for link. I am creating a new patch maybe it will not break WordPress core.

@ravinderk
9 years ago

Fix for breaking gravatar functionality on pre_get_avatar_data hook

@ravinderk
9 years ago

Removing extra $url2x variable from patch

@ravinderk
9 years ago

set $argsurl2x? to false if current comment type is not in list of allowed comment types

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


9 years ago

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


9 years ago

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


9 years ago

#16 @ravinderk
9 years ago

@johnbillion @iseulde @SergeyBiryukov
what do you think about my latest patch?

#17 @nacin
9 years ago

Is get_avatar_data() that slow?

#18 @obenland
9 years ago

@SergeyBiryukov, beta1 is one week away, can you get this resolved until then?

#19 @nacin
9 years ago

I don't see compelling data to do this. What's the performance difference? I'd suggest, pending any feedback, a revert of [32702] and a wontfix.

#20 @miqrogroove
9 years ago

I am also in favor of revert. Adding "2x" args filters may be beneficial some day, but the premise of this ticket is misguided.

#21 @ravinderk
9 years ago

@nacin

what happen if i want 2x image form my custom avatar.

  1. add filter at pre_get_avatar and write some more custom functionality on this hook or,
  2. add a filter at pre_get_avatar_data but i get only simple 1x images from it, because get_avatar_url return simple 1x url

Question

  1. Is it good to call same function two time while we can get same thing in one call.
  2. What is a best way to hanble 2x image for custom avatar?
Last edited 9 years ago by ravinderk (previous) (diff)

#22 @SergeyBiryukov
9 years ago

In 32969:

Revert [32702]. The URL may not have an s parameter as there are filters in place so that a plugin can return a URL with a completely different structure.

see #32572.

#23 @SergeyBiryukov
9 years ago

  • Milestone 4.3 deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

get_avatar_data() does not make any external requests or complex calculations, so the performance difference should be negligible.

#24 @rachelbaker
8 years ago

#36315 was marked as a duplicate.

Note: See TracTickets for help on using tickets.