Add site icon to rest index

[spacedmonkey]

Add site icon to list of site metadata return in the index.

[TimothyBlynJacobs]

Should we expose the id of the attachment and then link to it? Or output get_site_icon_url. If the latter, what sizes should we expose?

[palmiak]

@spacedmonkey you mean the /wp-json/ url right? Together with name, description etc.

Personally I would use the default size provided by get_site_icon_url - it's filtrable so it wouldn't be a problem if someone would like to change it.

[spacedmonkey]

Thanks for the patch @palmiak.

Looking at the patch, it seems like we should add some basic unit tests. Maybe a case with and without a site icon set to see what happens. The tests should be added ​here.

[lephleg]

Hello @palmiak, is there a PR for this in order to add the unit tests?

https://core.trac.wordpress.org/ticket/52321

[palmiak]

I added a test to check if key exists and one more, after setting the image, is the image name correct.

[spacedmonkey]

@TimothyBlynJacobs I feel like we can get this into WP 5.8. It is a simple change.

[TimothyBlynJacobs]

Feature freeze for 5.8 has already ​passed. Feel free to milestone for 5.9 though.

[spacedmonkey]

This ticket was discussed in this week's REST API core meeting.

It was decided that the patch needs to be refreshed to be inline with changes in REST API.

There is current a ​add_site_logo_to_index method, that add site logo to the rest api. This ticket should follow this pattern and add a data for a link.

I know there is a patch @palmiak, I wonder if he update be willing to update his ticket with this feedback.

If not, willing to take a look at this myself.

[palmiak]

I'll do it this weekend. Shouldn't be a problem

[spacedmonkey]

@palmiak Any chance of getting that patch before the Beta 1 cut off on Tuesday the 9th?

If not, I will create a refreshed patch myself.

[palmiak]

Hi @spacedmonkey

I'm attaching the fixed diff. I hope this is alright.

I also fixed the tests in PR, but I'm getting a merge conflict (and it's blocking tests from running).

[spacedmonkey]

@palmiak the GitHub pr seems to have lots of unrelated changes it is. The last patch doesn’t seem to contain tests.

Let me know if you need help, I can message you slack to talk you thought it.

[spacedmonkey]

Let's update the patch to change this line

 rest_url( 'wp/v2/media/' . $site_icon_id )

to

  rest_url( rest_get_route_for_post( $site_icon_id ) )

See this commit for ​details.

[spacedmonkey]

Updated patch looks good. We need to update wp-api-generated.js but it I think we are good to commit.

Trac ticket: https://core.trac.wordpress.org/ticket/52321

[spacedmonkey]

I have an updated version of the ticket at ​#1844. This ticket is ready to commit.

[TimothyBlynJacobs]

Thanks @spacedmonkey. Let's add a key to the link definition to disambiguate between the logo and icon. Perhaps kind and set it to logo or icon?

[spacedmonkey]

@TimothyBlynJacobs

Like this?

array(
   'embeddable' => true,
   'kind' => 'logo',
)

And added to both logo and icon?

[TimothyBlynJacobs]

Yeah I think so.

[TimothyBlynJacobs]

In 52080:

REST API: Expose the site icon in the REST API index.

Props spacedmonkey, palmiak.
Fixes #52321.

[prbot]

​TimothyBJacobs commented on ​PR #1844:

Merged in https://core.trac.wordpress.org/changeset/52080.

[prbot]

​TimothyBJacobs commented on ​PR #1111:

Merged in https://core.trac.wordpress.org/changeset/52080.