Edit Site: Fetch template parts in Template Switcher from REST API #21878
Conversation
|
Size Change: +24 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
ockham
force-pushed
the
update/edit-site-fetch-templates-client-side
branch
3 times, most recently
from
f6123d7 to
cfd7708
Compare
ockham
force-pushed
the
update/edit-site-fetch-templates-client-side
branch
2 times, most recently
from
79d43c2 to
72d292a
Compare
| @@ -321,7 +321,7 @@ function gutenberg_find_template_post_and_parts( $template_type, $template_hiera | |||
|
|
|||
| if ( $current_template_post ) { | |||
| $template_part_ids = array(); | |||
| if ( is_admin() ) { | |||
| if ( is_admin() || defined( 'REST_REQUEST' ) ) { | |||
There was a problem hiding this comment.
REST_REQUEST isn't defined for internal requests done with rest_do_request and generally it isn't encouraged. I see that it is being used elsewhere in this function, but I think it'd be better to change this conditional to use a parameter.
There was a problem hiding this comment.
Right, I was indeed copy-pasting here. If permissible, I'd like to address this in a later iteration/separate PR, since it's pre-existing, and I'd have to read up a bit on this (and would rather not block this PR by that issue 😅 )
There was a problem hiding this comment.
Yeah I think it's just something to be looked at some point.
|
|
||
| const homeId = getEntityRecords( 'postType', 'wp_template', { | ||
| resolved: true, | ||
| slug: 'front-page', |
There was a problem hiding this comment.
front-page is not necessarily the home template. You need to make the same API call navigate-to-link makes.
There was a problem hiding this comment.
Done in ff313c4. Tho in the long run, I think we should modify the wp_template endpoint, or altenatively write a new endpoint that returns the template for a given entity or query.
ockham
force-pushed
the
update/edit-site-fetch-templates-client-side
branch
from
583b19a to
d8749dc
Compare
|
I'll pause work on this until next week, when I'm back from my maintenance rotation. Switching in an out of context for this PR clearly doesn't give me enough headspace to tackle the issues related to asynchronicity. |
ockham
force-pushed
the
update/edit-site-fetch-templates-client-side
branch
from
6e26ef7 to
f51da73
Compare
|
Going to rebase. |
ockham
force-pushed
the
update/edit-site-fetch-templates-client-side
branch
2 times, most recently
from
73101af to
fd2c805
Compare
ockham
changed the title
|
Well, that was a big rebase 😅 Should be ready for another look 🤞 |
|
Thanks! As discussed per DM, I think that the natural progression for this PR would now be to move the template part fetching logic into a Edit: As @epiqueras clarified via DM, we'll stick with this PR as-is (i.e. we won't move logic to resolvers.) |
ockham
force-pushed
the
update/edit-site-fetch-templates-client-side
branch
2 times, most recently
from
6c0fc8c to
91b0423
Compare
|
Rebased. I ended up reverting the changes to Anyway, this PR should be ready for another look. |
ockham
force-pushed
the
update/edit-site-fetch-templates-client-side
branch
from
ba283da to
59d0688
Compare
| findTemplate( | ||
| '/', | ||
| registry.__experimentalResolveSelect( 'core' ).getEntityRecords | ||
| ).then( | ||
| ( newHomeId ) => setHomeId( newHomeId ), | ||
| () => setHomeId( null ) | ||
| ); |
There was a problem hiding this comment.
It's great that we now have the findTemplate util, but TBH, its signature (specifically, the need to pass in a selector as an argument) and async nature (requiring local component state) are among the reasons why I was opting for moving this kind of stuff into resolvers.
Not relevant for this PR, but I wanted to point it out in case we'd be willing to reconsider later.
There was a problem hiding this comment.
We could wrap it in a custom hook too.
There was a problem hiding this comment.
Resolvers need to be tied to the store state, and it would be awkward to have these temporary values there.
There was a problem hiding this comment.
I wouldn't introduce a dedicated resolver for findTemplate; I was specifically thinking of one for getTemplateParts( path ) that would call const templateId = yield findTemplate( path ) internally, and use that to fetch the template parts for that template.
Or we could even modify the entity endpoints for templates and template parts some more to accept a path arg, and do the template lookup on the server side.
There was a problem hiding this comment.
Or we could even modify the entity endpoints for templates and template parts some more to accept a path arg, and do the template lookup on the server side.
That makes more sense don't you think?
There was a problem hiding this comment.
Possibly, yeah. I'm not sure if it'd maybe bend the entities abstraction a bit much (if we keep adding rather specific custom args to it that change the query significantly), but maybe that's alright.
Think it still makes sense to merge this PR as a first step towards fetching data from the REST API (rather than from a server-passed JS variable)? 🤔
There was a problem hiding this comment.
I'd rather avoid adding templateId as a parameter only to remove it later.
There was a problem hiding this comment.
Makes sense.
Unfortunately, doing the path-to-template resolution on the server side will be non-trivial AFAICS -- the get_{$type}_template() functions don't accept any args to pass entity specifiers (e.g. post/page IDs, category slugs, etc) but infer those from context (e.g. via get_queried_id()). Maybe it'll be possible to hook a filter into those, but there's a chance that we'll end up replicating a lot of that logic 🙁
There was a problem hiding this comment.
Let's leave it for later then.
|
Getting some weird (ES)Lint errors. I'll try rebasing. |
ockham
force-pushed
the
update/edit-site-fetch-templates-client-side
branch
from
6623b9f to
c4d0ca3
Compare
lib/template-parts.php
Outdated
| * | ||
| * @param array $args The query arguments. | ||
| * @param WP_REST_Request $request The request object. | ||
| * @return array Filtered $args. | ||
| */ | ||
| function filter_rest_wp_template_part_query( $args, $request ) { | ||
| if ( $request['resolved'] ) { |
There was a problem hiding this comment.
| if ( $request['resolved'] ) { | |
| if ( $request['resolved'] || $request['template'] ) { |
There was a problem hiding this comment.
FTR, I was trying to retain consistency with lib/templates.php here, where we only check for resolved:
Line 184 in 206c5f1
...but also take the template slug into account:
Line 186 in 206c5f1
The difference is that in the case of templates, we can use the slug field that already exists (as part of the entities endpoint), so we need the resolved flag to convey the different semantics (only return 'resolved' templates that match the slug vs return all templates that match it (e.g. including all auto-drafts)).
We could theoretically assign a different meaning to a template parts query that has a template arg but not a resolved one -- it'd be all template parts for all templates matching that template slug (including auto-drafts etc) -- but I doubt that'd be very useful in practice.
There was a problem hiding this comment.
I'll apply this change and add a comment with the gist of the above.
There was a problem hiding this comment.
For template parts, template implies resolved. I can't think of a case where the opposite would be useful.
| template: _template, | ||
| templateParts: _template | ||
| ? getEntityRecords( 'postType', 'wp_template_part', { | ||
| resolved: true, |
There was a problem hiding this comment.
| resolved: true, |
ockham
force-pushed
the
update/edit-site-fetch-templates-client-side
branch
from
df5546a to
e3e9763
Compare
ockham
force-pushed
the
update/edit-site-fetch-templates-client-side
branch
from
00800f0 to
3419ed0
Compare
Description
Instead of iterating over
templateIdsand fetching each corresponding template from the REST API, use one network fetch.This is to decouple
edit-siteeven further from thesettingsvariable that is at the moment directly passed from PHP, and will use the REST API instead. #21877 is complementary to this PR.How has this been tested?
Verify that the templates and template parts listed in the template switcher are the same as before.
Checklist: