WordPress.org

Make WordPress Core

Opened 17 months ago

Closed 10 months ago

Last modified 8 months ago

#50152 closed enhancement (fixed)

Return all themes into the themes REST API

Reported by: spacedmonkey Owned by: TimothyBlynJacobs
Milestone: 5.7 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests early commit has-dev-note
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 (46)

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


17 months ago

Add all themes to API

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

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


17 months ago

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

TimothyBJacobs commented on PR #264:

👍 Test failure looks legit.

#8 @prbot
16 months ago

spacedmonkey commented on PR #264:

@TimothyBJacobs Can you review again.

Small note, seems like lints failing for unrelated reason

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

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

CC @ockham

#13 @prbot
16 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
16 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
16 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
16 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
16 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
16 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
16 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
16 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
16 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
16 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.


13 months ago

#25 @helen
13 months 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
13 months ago

Is this a Multisite-specific change?

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


13 months ago

#28 @hellofromTonya
13 months ago

  • Keywords needs-refresh added

Adding the refresh keyword for the PR merge conflict.

#29 @TimothyBlynJacobs
12 months 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
12 months ago

  • Milestone changed from 5.6 to Future Release

#31 @prbot
12 months ago

spacedmonkey commented on PR #264:

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

#32 @prbot
12 months 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
12 months 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
12 months ago

lukaspawlik commented on PR #264:

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

#36 @prbot
12 months ago

lukaspawlik commented on PR #264:

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

#37 @prbot
12 months 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
12 months ago

  • Keywords early added
  • Milestone changed from Future Release to 5.7

#39 @prbot
10 months ago

TimothyBJacobs commented on PR #264:

Hey @lukaspawlik! Thanks again for working on this. I'm going to plan on merging this next week.

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


10 months ago

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


10 months ago

#42 @metalandcoffee
10 months ago

  • Keywords commit added; needs-refresh removed

#43 @TimothyBlynJacobs
10 months ago

  • Owner set to TimothyBlynJacobs
  • Resolution set to fixed
  • Status changed from new to closed

In 49925:

REST API: Expose all themes in the themes controller.

Previously, only the active theme was made available. This commit allows for all themes to be queried if the user has the switch_themes or manage_network_themes capabilities.

This commit also no longer exposes the page, per_page, search and context query parameters since they are not supported by this controller.

Props spacedmonkey, lpawlik, TimothyBlynJacobs.
Fixes #50152.

#44 @prbot
10 months ago

TimothyBJacobs commented on PR #264:

Committed in https://core.trac.wordpress.org/changeset/49925, thanks again for your work on this @lukaspawlik!

#45 @audrasjb
9 months ago

  • Keywords needs-dev-note added

This enhancement needs a dev note :)

#46 @audrasjb
8 months ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.