50152 - Add all themes to API

[spacedmonkey]

Add all themes to API

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

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[TimothyBJacobs]

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

[spacedmonkey]

#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.

[TimothyBJacobs]

👍 Test failure looks legit.

[spacedmonkey]

@TimothyBJacobs Can you review again.

Small note, seems like lints failing for unrelated reason

[TimothyBJacobs]

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?

[spacedmonkey]

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 @TimothyBJacobs . I didn't see that you could pass an array there. I have made the logic much more simple.

[TimothyBJacobs]

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.

[spacedmonkey]

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.

[TimothyBJacobs]

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.

[spacedmonkey]

@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?

[TimothyBJacobs]

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?

[spacedmonkey]

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

[TimothyBJacobs]

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.

[lukaspawlik]

@TimothyBJacobs Your feedback has been addressed in c2855ea

[TimothyBJacobs]

@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

[lukaspawlik]

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

[lukaspawlik]

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

[TimothyBJacobs]

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

[TimothyBJacobs]

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

[TimothyBJacobs]

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