WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 7 days ago

#50152 new enhancement

Return all themes into the themes REST API

Reported by: spacedmonkey Owned by:
Milestone: 5.7 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests needs-refresh early
Focuses: rest-api Cc:

Description

Currently only the current theme is returned in the themes REST API api. The theme api should be about to list all active themes installed.

Change History (38)

#1 @spacedmonkey
6 months ago

  • Summary changed from Return all themes into the themes API to Return all themes into the themes REST API

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


6 months ago

Add all themes to API

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

#3 @spacedmonkey
6 months ago

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

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


5 months ago

#5 @prbot
5 months ago

TimothyBJacobs commented on PR #264:

#222 has landed so it should be possible to add tests now. We should also add a status field to the prepared item.

#6 @prbot
5 months ago

spacedmonkey commented on PR #264:

#222 has landed so it should be possible to add tests now. We should also add a status field to the prepared item.

Status was added. I added the ability to filter by inactive themes.

#7 @prbot
5 months ago

TimothyBJacobs commented on PR #264:

👍 Test failure looks legit.

#8 @prbot
5 months ago

spacedmonkey commented on PR #264:

@TimothyBJacobs Can you review again.

Small note, seems like lints failing for unrelated reason

#9 @prbot
5 months ago

TimothyBJacobs commented on PR #264:

that's a very odd lint issue.

What happens if I do ?status=active,inactive? It seems like it'll only return the active theme?

#10 @prbot
5 months ago

spacedmonkey commented on PR #264:

that's a very odd lint issue.

What happens if I do ?status=active,inactive? It seems like it'll only return the active theme?

Good catch. I didn't see that you could pass an array there. I have made the logic much more simple.

#11 @prbot
5 months ago

TimothyBJacobs commented on PR #264:

The new logic looks great. I think we also need to adjust the permissions check so that you can't bypass the fetching inactive caps check by doing a active,inactive. Let's also get unit tests for permissions with those different scenarios.

#12 @spacedmonkey
4 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Milestone changed from Future Release to 5.5

CC @ockham

#13 @prbot
4 months ago

spacedmonkey commented on PR #264:

The new logic looks great. I think we also need to adjust the permissions check so that you can't bypass the fetching inactive caps check by doing a active,inactive. Let's also get unit tests for permissions with those different scenarios.

I have added some tests, using a data_provider, to pass different status to check permissions. This should be enough.

#14 @prbot
4 months ago

TimothyBJacobs commented on PR #264:

I think we should introduce a get_item route at the same time here.

Do you have any idea for what it would look like to switch themes? Even if we don't implement it now, I think we should consider what that API call would look like.

#15 @prbot
4 months ago

spacedmonkey commented on PR #264:

@TimothyBJacobs I have added the get_item method with tests.

I am not sure what switch_theme might look like right now. Do this PR have to tackle this?

#16 @prbot
4 months ago

TimothyBJacobs commented on PR #264:

I don't think this PR needs to implement it, but I want to make sure we could with this format. Would a PUT status=active work here?

#17 @prbot
4 months ago

spacedmonkey commented on PR #264:

I don't think this PR needs to implement it, but I want to make sure we could with this format. Would a PUT status=active work here?

I would like that format / design of the API the same as https://github.com/WordPress/wordpress-develop/pull/359 .

  • For WP_REST_Server::EDITABLE, you can to pass status of active and inactive. Active, will switch theme to this theme. Inactive, will switch the theme to the default theme.
  • For WP_REST_Server::DELETABLE this would delete theme.
  • Any allow for installing themes from the WP.org repo.

We could even create a abstration for dependencies like themes and plugins, something like WP_REST_Dependiens_Controller or similar. Just an idea.

#18 @prbot
4 months ago

TimothyBJacobs commented on PR #264:

As discussed in #core-restapi, let's switch out the hardcoded twentytwenty for the more future proof WP_DEFAULT_THEME constant.

Additionally, we should definitely make sure that if a user has permission to access the current theme in the list view, that they can access the theme directly as wp/v2/themes/<name>.

We could then explore adding a link to the index response in WP_REST_Server::get_index that points to the currently active theme for the site. We should probably only expose that link if the current user has permission to view the resource.

Let's also make sure that a user can't discover what themes a user has installed by pinging the single theme route when they don't have permission. So it should do a general caps check first, before returning a 404.

#19 @prbot
4 months ago

spacedmonkey commented on PR #264:

We could then explore adding a link to the index response in WP_REST_Server::get_index that points to the currently active theme for the site. We should probably only expose that link if the current user has permission to view the resource.

I am not sure what this means or how to do it?

#21 @prbot
4 months ago

spacedmonkey commented on PR #264:

Adding a link here,

https://github.com/WordPress/wordpress-develop/blob/3785439c8bf01ad18727433f3586dc2cbf641e2a/src/wp-includes/rest-api/class-wp-rest-server.php#L1125

I don't know what this does and why we would need to do this? Never touched added one these link. Can you explain the change a little more.

#22 @prbot
4 months ago

TimothyBJacobs commented on PR #264:

It is the same as any other link in the REST API and is served at the index of wp-json. The index is meant to describe the site, so we would link to the active theme so users can programmatically access that instead of fetching the collection.

#23 @davidbaumwald
4 months ago

  • Milestone changed from 5.5 to 5.6

With Beta 1 for 5.5 landing in just a few hours, this is being moved to 5.6. If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

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


4 weeks ago

#25 @helen
4 weeks ago

We're well into 5.6 now, so somebody needs to pick this up and get the PR refreshed with the review suggestions in the next week or so if we still want to keep this in consideration for the release.

#26 @johnbillion
4 weeks ago

Is this a Multisite-specific change?

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


4 weeks ago

#28 @hellofromTonya
4 weeks ago

  • Keywords needs-refresh added

Adding the refresh keyword for the PR merge conflict.

#29 @TimothyBlynJacobs
3 weeks ago

This hasn't seen activity since last week's checkin. Going to go ahead and bump to future release. Feel free to remilestone if someone wants to own this in the next few days.

#30 @TimothyBlynJacobs
3 weeks ago

  • Milestone changed from 5.6 to Future Release

#31 @prbot
10 days ago

spacedmonkey commented on PR #264:

@TimothyBJacobs Can you re-review? I think this can land early 5.7 thanks @lukaspawlik !

#32 @prbot
10 days ago

TimothyBJacobs commented on PR #264:

Thanks for the refresh @lukaspawlik!

We need to update the @since versions to 5.7.0.

This still needs to be addressed:

We could then explore adding a link to the index response in WP_REST_Server::get_index that points to the currently active theme for the site. We should probably only expose that link if the current user has permission to view the resource.

#34 @prbot
9 days ago

TimothyBJacobs commented on PR #264:

@lukaspawlik Thanks! To clarify, only the new functions should have an @since of 5.7.0, the existing function should stay the same.

https://developer.wordpress.org/coding-standards/inline-documentation-standards/php/#since-section-changelogs

#35 @prbot
9 days ago

lukaspawlik commented on PR #264:

Thanks @TimothyBJacobs for the feedback. My mistake with versioning - updating it as per recommendation.

#36 @prbot
9 days ago

lukaspawlik commented on PR #264:

@TimothyBJacobs thanks for overall feedback. It's been addressed and awaiting your review.

#37 @prbot
8 days ago

TimothyBJacobs commented on PR #264:

This looks good, thanks for your contribution! We're still in 5.6, so this won't be merged for a few weeks.

#38 @TimothyBlynJacobs
7 days ago

  • Keywords early added
  • Milestone changed from Future Release to 5.7
Note: See TracTickets for help on using tickets.