-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Themes REST API endpoint: Add stylesheet_uri and template_uri fields to the response #6399
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
I've updated and added tests. A few questions for anyone reviewing:
|
It seems unexpected that this is not applying the |
Thanks for the review @TimothyBJacobs!
Good catch! I've updated the logic in b1dd7ff so that it calls the global functions for both the template directory uri and the stylesheet directory uri if the current theme matches the requested theme, so I believe this should hook into those filters now. I'm not very familiar with this part of the code base, though, so do let me know if there's anything off with this approach. Thanks! |
Gave this for a spin locally by checking the branch out and creating a child theme of TT4 called
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM to me too. Nice work 👍
* @ticket 61021 | ||
*/ | ||
public function test_theme_template_uri() { | ||
$response = self::perform_active_theme_request(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it worth testing inactive themes as well after b1dd7ff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, thanks for spotting that! I've updated the tests in b444489 to include both active and inactive themes in the response, and loop over the results to check against the global function or the method on the theme object, depending on the status.
Let me know if you think it's better to split into separate tests, though.
foreach ( $result as $theme_result ) { | ||
$this->assertArrayHasKey( 'template_uri', $theme_result ); | ||
if ( 'active' === $theme_result['status'] ) { | ||
$this->assertSame( get_template_directory_uri(), $theme_result['template_uri'] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating! LGTM
The only thing I'd do is add a message as the third argument to the assertSames so that failing tests return something meaningful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point 👍
Added in: 23cae05. Happy to tweak the wording if need be.
Thanks for working on this 🙇🏻 LGTM! |
Thanks for all the reviews, everyone! I'll be AFK over the next week, so in case there's any further feedback, I won't be able to get to it until the week after. In the meantime, feel free to commit to this branch if anyone would like to make changes, (or commit the changeset if this feels like it's in a good enough place). Cheers! |
Merged in r58282. |
Should we also sync these changes to Gutenberg? There's currently no feature that relies on these properties, but maybe it should be synced to It's likely these properties will be needed to resolve relative asset paths in template/pattern files. I'd already synced the REST change for a previous experiment so I threw up a PR just in case: |
Good idea @ramonjd — yes, even though there are no features depending on it, I think it'd be good to have parity for the feature in Gutenberg to make it easier to use if need be. |
Based on an idea from @ramonjd and @noisysocks in WordPress/gutenberg#60578, this PR explores adding
stylesheet_uri
andtemplate_uri
fields to the API response for thethemes
endpoint. The goal being to expose the URI of the directory for a theme (or its parent theme when dealing with child themes) so that theme relative URIs can be constructed by consumers of this endpoint. An example use case is in WordPress/gutenberg#60578A quick way to test this is to apply the PR and then open up the post editor and run the following from the console:
In the API response, you should see stylesheet and template uris in the response:
Trac ticket: https://core.trac.wordpress.org/ticket/61021
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.