WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 months ago

#40030 closed defect (bug) (fixed)

Pass User and Comment Objects to rest_get_avatar_urls

Reported by: dshanske Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: good-first-bug has-patch
Focuses: Cc:
PR Number:

Description

This function retrieves avatars based on email address. It is used by the comments and user controllers.

However, this means that we are locking ourselves into Gravatar, which is a whole other conversation.

The dependent Comment and User 'prepare_item_for_response' functions pass the email address from a User Object or Comment Object to the rest_get_avatar_urls function.

Instead, they should pass the entire Comment or User object. Since get_avatar_url supports this, it will result in no backward compatibility issues, and this will allow the objects to be filtered using existing filters, assuming that you wanted to return alternative avatar data, such as a local avatar, alternative remote service, what have you.

Since this is internal to Core, it won't result in any data leakage to the API.

Attachments (1)

pass_object_to_rest_get_avatar_urls.40030.diff (2.4 KB) - added by donmhico 3 months ago.

Download all attachments as: .zip

Change History (14)

#1 @joehoyle
3 years ago

  • Milestone changed from Awaiting Review to Future Release

This makes sense to me, we'll need to change the signature of rest_get_avatar_urls to accept an object too - Another idea would be to define a new function to be used instead; if we don't want to make more magical functions!

#2 @dshanske
3 years ago

@joehoyle All of the avatar functions accept multiple inputs. Is there an issue with changing the passed variables and the signature? I wish avatars had a different design, but we're stuck with it.

Or, if I prepare the patch, for milestoning this for a point release?

#3 follow-up: @joehoyle
3 years ago

I wish avatars had a different design, but we're stuck with it.

That's kinda my point, we are _not_ stuck with it in this case, we have the oppurtinty to make something like rest_get_avatar_urls_for_user if we wanted to; I think.

#4 @dshanske
3 years ago

Well, that does add a level of complexity to it. I am perfectly happy to write functions like that, but is that the best approach?

#5 @pputzer
15 months ago

Can we do something about this? I just stumbled upon the issue in one of my plugins and there don't even seem to be enough hooks in the controllers to implement a workaround :(

#6 in reply to: ↑ 3 @pputzer
14 months ago

Replying to joehoyle:

I wish avatars had a different design, but we're stuck with it.

That's kinda my point, we are _not_ stuck with it in this case, we have the oppurtinty to make something like rest_get_avatar_urls_for_user if we wanted to; I think.

Having implemented a workaround in Avatar Privacy, you will then also need a rest_get_avatar_urls_for_comment to handle a similar issue in WP_REST_Comments_Controller::prepare_item_for_response. As is, these functions would each have to reimplement the looping logic. I'm not sure this is worth the duplication.

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


3 months ago

#8 @TimothyBlynJacobs
3 months ago

  • Keywords needs-patch good-first-bug added
  • Version changed from 4.4 to 4.7

I think passing the full objects to rest_get_avatar_urls() is workable. Considering there aren’t any filters here and the function is really only a wrapper around get_avatar_url, I don’t think a separate function is necessary.

@dshanske, @pputzer do either of you want to work on a patch for this?

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


3 months ago

#10 follow-up: @pputzer
3 months ago

  • Keywords has-patch added; needs-patch removed

@donmhico's patch looks good to me, except that I think the parameter object should not be referred to as "the Gravatar". I know that's the wording from get_avatar_url(), but I think "The object to retrieve an avatar URL for." would be better to make it clear what the parameter actually is. (The suggested wording would also not reinforce the erroneous conception that "avatar" and "Gravatar" are interchangeable.)

#11 @SergeyBiryukov
3 months ago

  • Milestone changed from Future Release to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#12 in reply to: ↑ 10 @SergeyBiryukov
3 months ago

Replying to pputzer:

@donmhico's patch looks good to me, except that I think the parameter object should not be referred to as "the Gravatar". I know that's the wording from get_avatar_url(), but I think "The object to retrieve an avatar URL for." would be better to make it clear what the parameter actually is.

Given that the $id_or_email parameter is described as "The Gravatar to retrieve a URL for" in
get_avatar_url(), get_avatar_data(), get_avatar(), and their respective filters, I'd rather do the same for consistency here.

We could open a new ticket to change that description globally if necessary.

#13 @SergeyBiryukov
3 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 45632:

REST API: Allow rest_get_avatar_urls() to accept full user, post, or comment objects, rather than just an email address, to provide better flexibility for alternative avatar data.

Since the function uses get_avatar_url() internally, which already supports it, this should not have any backward compatibility concerns.

Props donmhico, dshanske, pputzer, joehoyle, TimothyBlynJacobs.
Fixes #40030.

Note: See TracTickets for help on using tickets.