Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#52321 closed enhancement (fixed)

Add site icon to rest index

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile 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 3 years ago.
52321-1.diff (2.7 KB) - added by palmiak 3 years ago.

Download all attachments as: .zip

Change History (31)

#1 @TimothyBlynJacobs
3 years 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.


3 years ago

#3 @palmiak
3 years 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
3 years ago

#4 @spacedmonkey
3 years 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
3 years 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.


3 years ago
#6

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

#7 @palmiak
3 years ago

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

#8 @spacedmonkey
3 years ago

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

#9 @TimothyBlynJacobs
3 years ago

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

#10 @spacedmonkey
3 years ago

  • Milestone changed from Future Release to 5.9

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


3 years ago

#12 @spacedmonkey
3 years ago

  • Keywords needs-refresh added

#13 @spacedmonkey
3 years 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
3 years 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.


3 years ago

#16 @spacedmonkey
3 years 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
3 years ago

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

#18 @palmiak
3 years 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
3 years ago

#19 @spacedmonkey
3 years 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
3 years 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
3 years 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.


3 years ago
#22

  • Keywords needs-refresh removed

#23 @spacedmonkey
3 years 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
3 years 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
3 years ago

@TimothyBlynJacobs

Like this?

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

And added to both logo and icon?

#26 @TimothyBlynJacobs
3 years ago

Yeah I think so.

#27 @TimothyBlynJacobs
3 years 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.