WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 4 weeks ago

#50152 new enhancement

Return all themes into the themes REST API

Reported by: spacedmonkey Owned by:
Milestone: 5.6 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests
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 (23)

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


3 months ago

Add all themes to API

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

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


2 months ago

#5 @prbot
2 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
2 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
2 months ago

TimothyBJacobs commented on PR #264:

👍 Test failure looks legit.

#8 @prbot
7 weeks ago

spacedmonkey commented on PR #264:

@TimothyBJacobs Can you review again.

Small note, seems like lints failing for unrelated reason

#9 @prbot
7 weeks 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
7 weeks 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
7 weeks 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
7 weeks ago

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

CC @ockham

#13 @prbot
7 weeks 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
6 weeks 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
6 weeks 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
6 weeks 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
6 weeks 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
5 weeks 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
5 weeks 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
5 weeks 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
5 weeks 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 weeks 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.

Note: See TracTickets for help on using tickets.