WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 4 weeks ago

Last modified 4 weeks ago

#52321 closed enhancement (fixed)

Add site icon to rest index

Reported by: spacedmonkey Owned by: spacedmonkey
Milestone: 5.9 Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: good-first-bug has-patch has-unit-tests commit
Focuses: rest-api Cc:

Description

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

Attachments (2)

52321.diff (668 bytes) - added by palmiak 9 months ago.
52321-1.diff (2.7 KB) - added by palmiak 5 weeks ago.

Download all attachments as: .zip

Change History (31)

#1 @TimothyBlynJacobs
11 months ago

  • Keywords good-first-bug added
  • Milestone changed from Awaiting Review to Future Release
  • Version set to 4.4

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?

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


11 months ago

#3 @palmiak
9 months ago

@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.

@palmiak
9 months ago

#4 @spacedmonkey
9 months ago

  • Keywords has-patch needs-unit-tests added; needs-patch removed

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.

#5 @lephleg
9 months ago

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

This ticket was mentioned in PR #1111 on WordPress/wordpress-develop by palmiak.


9 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#7 @palmiak
9 months ago

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

#8 @spacedmonkey
6 months ago

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

#9 @TimothyBlynJacobs
6 months ago

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

#10 @spacedmonkey
6 months ago

  • Milestone changed from Future Release to 5.9

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


8 weeks ago

#12 @spacedmonkey
8 weeks ago

  • Keywords needs-refresh added

#13 @spacedmonkey
8 weeks ago

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.

#14 @palmiak
8 weeks ago

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

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


5 weeks ago

#16 @spacedmonkey
5 weeks ago

@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.

#17 @spacedmonkey
5 weeks ago

  • Owner set to spacedmonkey
  • Status changed from new to assigned

#18 @palmiak
5 weeks ago

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).

@palmiak
5 weeks ago

#19 @spacedmonkey
5 weeks ago

@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.

#20 @spacedmonkey
5 weeks ago

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.

#21 @spacedmonkey
4 weeks ago

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

This ticket was mentioned in PR #1844 on WordPress/wordpress-develop by spacedmonkey.


4 weeks ago

  • Keywords needs-refresh removed

#23 @spacedmonkey
4 weeks ago

  • Focuses rest-api added
  • Keywords commit added

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

#24 @TimothyBlynJacobs
4 weeks ago

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?

#25 @spacedmonkey
4 weeks ago

@TimothyBlynJacobs

Like this?

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

And added to both logo and icon?

#26 @TimothyBlynJacobs
4 weeks ago

Yeah I think so.

#27 @TimothyBlynJacobs
4 weeks ago

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

In 52080:

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

Props spacedmonkey, palmiak.
Fixes #52321.

Note: See TracTickets for help on using tickets.