#52321 closed enhancement (fixed)
Add site icon to rest index
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (31)
#1
@
2 years ago
- Keywords good-first-bug added
- Milestone changed from Awaiting Review to Future Release
- Version set to 4.4
This ticket was mentioned in Slack in #core-restapi by spacedmonkey. View the logs.
2 years ago
#3
@
2 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.
#4
@
2 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.
This ticket was mentioned in PR #1111 on WordPress/wordpress-develop by palmiak.
2 years ago
#6
- Keywords has-unit-tests added; needs-unit-tests removed
#7
@
2 years ago
I added a test to check if key exists and one more, after setting the image, is the image name correct.
This ticket was mentioned in Slack in #core-restapi by spacedmonkey. View the logs.
20 months ago
#13
@
20 months 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.
This ticket was mentioned in Slack in #core-restapi by spacedmonkey. View the logs.
19 months ago
#16
@
19 months 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.
#18
@
19 months 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).
#19
@
19 months 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
@
19 months 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
@
19 months 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.
19 months ago
#22
- Keywords needs-refresh removed
Trac ticket: https://core.trac.wordpress.org/ticket/52321
#23
@
19 months 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
@
19 months 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
@
19 months ago
@TimothyBlynJacobs
Like this?
array(
'embeddable' => true,
'kind' => 'logo',
)
And added to both logo and icon?
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?