Edit Site: Remove templateIds prop from NavigateToLink #21877
Conversation
|
Size Change: +3.28 kB (0%) Total Size: 831 kB
ℹ️ View Unchanged
|
| const { getEntityRecord } = select( 'core' ); | ||
| newTemplateId = templateIds | ||
| .map( ( id ) => | ||
| getEntityRecord( 'postType', 'wp_template', id ) | ||
| ) | ||
| .find( | ||
| ( template ) => template.slug === data.post_name | ||
| ).id; | ||
| const { getEntityRecords } = select( 'core' ); | ||
| newTemplateId = getEntityRecords( | ||
| 'postType', | ||
| 'wp_template', | ||
| { | ||
| slug: data.post_name, | ||
| } | ||
| )[ 0 ].id; |
There was a problem hiding this comment.
The slug approach does not guarantee you'll get the same post ID that is loaded in the editor. It doesn't account for multiple auto-drafts, a published template, etc.
Was the previous approach making REST requests? All of those template IDs should be preloaded.
There was a problem hiding this comment.
The slug approach does not guarantee you'll get the same post ID that is loaded in the editor. It doesn't account for multiple auto-drafts, a published template, etc.
Oh, okay. I think that's something we should seek to resolve though -- possibly through some modifications to the endpoint, in order to reflect the template loading mechanism.
Aside, is the 'proper' resolution documented anywhere? I.e. when to use the published template, when to fall back to an auto-draft, when to generate one (?) etc? If it isn't, it'd be good to have it on file somewhere so it's easier to reason about.
It'd be good to have a client-side mechanism to request an up-to-date list of templates, so we can use the familiar useSelect/useDispatch pattern to update those. This will help solving glitches like this, and will overall help decouple the client from the PHP-provided variables (which in turn will eventually allow us to directly integrate edit-site with e.g. Calypso).
There was a problem hiding this comment.
Oh, okay. I think that's something we should seek to resolve though -- possibly through some modifications to the endpoint, in order to reflect the template loading mechanism.
We shouldn't modify the standard REST API collection behavior. We can introduce a separate endpoint for finding the currently published template for a given slug.
Aside, is the 'proper' resolution documented anywhere? I.e. when to use the published template, when to fall back to an auto-draft, when to generate one (?) etc? If it isn't, it'd be good to have it on file somewhere so it's easier to reason about.
Published templates are always used first and mean the template was customized. Auto drafts are only used when the templates haven't been customized. We should preferably use the latest ones, but because certain preloading flows can run template resolution more than once, you can end up in a situation where the site editor loaded the N-1 auto drafts and then when you do this fetch by slug, you end up with an N auto draft.
This isn't documented because it's a regression I recently found when catching up with the preloading work.
It'd be good to have a client-side mechanism to request an up-to-date list of templates, so we can use the familiar useSelect/useDispatch pattern to update those. This will help solving glitches like this, and will overall help decouple the client from the PHP-provided variables (which in turn will eventually allow us to directly integrate edit-site with e.g. Calypso).
Agreed.
There was a problem hiding this comment.
Oh, okay. I think that's something we should seek to resolve though -- possibly through some modifications to the endpoint, in order to reflect the template loading mechanism.
We shouldn't modify the standard REST API collection behavior. We can introduce a separate endpoint for finding the currently published template for a given slug.
Agreed. If the required query can still be done on the client side (and doesn't impact perf too negatively), we can also resort to that. (I didn't mean to mess with the collection endpoint format, I was rather considering adding some standard modifications akin to e.g. your #21851.)
Aside, is the 'proper' resolution documented anywhere? I.e. when to use the published template, when to fall back to an auto-draft, when to generate one (?) etc? If it isn't, it'd be good to have it on file somewhere so it's easier to reason about.
Published templates are always used first and mean the template was customized. Auto drafts are only used when the templates haven't been customized. We should preferably use the latest ones, but because certain preloading flows can run template resolution more than once, you can end up in a situation where the site editor loaded the
N-1auto drafts and then when you do this fetch by slug, you end up with anNauto draft.
Thanks for explaining, that's very helpful!
This isn't documented because it's a regression I recently found when catching up with the preloading work.
Also good to know, thanks 👍
There was a problem hiding this comment.
Perhaps, a new top-level query param would be nice. That way, we can keep using core/data semantically in the client, but leave the awkward publish/auto-draft logic in the server. Something like ?loaded or ?resolved.
There was a problem hiding this comment.
BTW, I just realized that the logic you described seems to be implemented in useTemplatePartPost, right? So it might make sense to use this in NavigateToLink in order to centralize the implementation of this behavior, and allow us to change it if we end up making changes to the endpoint.
Perhaps, a new top-level query param would be nice. That way, we can keep using
core/datasemantically in the client, but leave the awkward publish/auto-draft logic in the server. Something like?loadedor?resolved.
Yeah, keeping that logic on the server side sounds like a good idea. Would you add those query args to the existing endpoint (using rest_wp_template_part_collection_params and rest_wp_template_part_query)?
There was a problem hiding this comment.
BTW, I just realized that the logic you described seems to be implemented in useTemplatePartPost, right? So it might make sense to use this in NavigateToLink in order to centralize the implementation of this behavior, and allow us to change it if we end up making changes to the endpoint.
Yes
Yeah, keeping that logic on the server side sounds like a good idea. Would you add those query args to the existing endpoint (using rest_wp_template_part_collection_params and rest_wp_template_part_query)?
Yes
Would you like to do those things in this PR instead?
There was a problem hiding this comment.
Yeah, I'll give that a shot tomorrow!
There was a problem hiding this comment.
Prep PRs took a while 😅 WIP PR for this feature: #21981
ockham
force-pushed
the
remove/navigate-to-link-template-ids-prop
branch
2 times, most recently
from
4e46c18
to
3a657a9
Compare
ockham
force-pushed
the
remove/navigate-to-link-template-ids-prop
branch
from
3a657a9
to
961d690
Compare
|
Is this ready for another look? |
It is now -- there was one annoying bug that I missed in #21981 and that I've finally fixed. I've added an (unfortunately lengthy 😅 ) note to the PR desc (including instructions how to test the fix). |
| @@ -183,7 +183,7 @@ function filter_rest_wp_template_collection_params( $query_params ) { | |||
| function filter_rest_wp_template_query( $args, $request ) { | |||
| if ( $request['resolved'] ) { | |||
| $template_ids = array( 0 ); // Return nothing by default (the 0 is needed for `post__in`). | |||
| $template_types = $request['slug'] ? array( $request['slug'] ) : get_template_types(); | |||
| $template_types = $request['slug'] ? $request['slug'] : get_template_types(); | |||
There was a problem hiding this comment.
This is the bug I missed in #21981. Turns out that $request['some-param'] is an array 😬
| @@ -194,10 +194,10 @@ function filter_rest_wp_template_query( $args, $request ) { | |||
|
|
|||
| $current_template = gutenberg_find_template_post_and_parts( $template_type ); | |||
| if ( isset( $current_template ) ) { | |||
| $template_ids[ $current_template['template_post']->post_name ] = $current_template['template_post']->ID; | |||
| $template_ids[] = $current_template['template_post']->ID; | |||
There was a problem hiding this comment.
This is kinda cosmetic, we just don't need array keys.
There was a problem hiding this comment.
It's to avoid duplication, but I guess post__in doesn't care.
|
Apologies, I found another issue. Putting on hold. |
ockham
added
the
[Status] Blocked
Fixed in 2528cfa. Looks like I'd broken template rendering 😅 Should be ready for review now. |
ockham
removed
the
[Status] Blocked
Description
Instead of iterating over
templateIds, fetching each corresponding template from the REST API, and then locally filtering for the matching template slug, use one network fetch with the slug passed as a query param (You can verify that that works by runningwp.data.select('core').getEntityRecords('postType', 'wp_template', { slug: 'front-page' } )in the browser console -- twice, to make it resolve properly.)This is preparatory work for another PR that will decouple
edit-siteeven further from thesettingsvariable that is at the moment directly passed from PHP, and will use the REST API instead.Update 2020-05-13: This also fixes a bug in #21981, where previously, a matching template (specified via
{ slug: 'front-page'; resolved: true }) would return an empty array. To verify, try the following (similar to #21981, but including the cases I failed to test for):We'll compare the results the
wp_templatecollections REST API endpoint gives to$settings['templateIds']. To that end, apply the following patch and re-build the app (npm run build):Next, in wp-admin, navigate to the Appearance > Templates section, and create a few dummy templates whose names are not actually part of the template hierarchy (e.g.
some-templateetc).Now go to Full-Site Editing and open your browser console. You should see the output of the
settingsvariable there. Compare thetemplateIdsproperty to the output of the following commands you run in the console (twice each!). Icons ✔️ and ❌ indicate whether the output is expected to matchsettings.templateIds, or not.wp.data.select('core').getEntityRecords('postType', 'wp_template', { slug: 'front-page' } )should return thewp_templateCPT whose slug isfront-page.wp.data.select('core').getEntityRecords('postType', 'wp_template', { slug: 'front-page', resolved: true } )should also return thewp_templateCPT whose slug isfront-page, as it is part of the template hierarchy.wp.data.select('core').getEntityRecords('postType', 'wp_template' )should return the list of allwp_templateCPTs, including those that are not part of the template hierarchy.wp.data.select('core').getEntityRecords('postType', 'wp_template', { resolved: true } )should return the list of templates that match the template hierarchy.wp.data.select('core').getEntityRecords('postType', 'wp_template', { slug: 'some-template' } )should return thewp_templateCPT whose slug issome-template.wp.data.select('core').getEntityRecords('postType', 'wp_template', { slug: 'some-template', resolved: true } )should return an empty array, assome-templateis not part of the template hierarchy.This highlights that we need unit tests for these functions, which we'll hopefully be able to run more easily once #20090 is in 🙂
How has this been tested?
In case you're not familiar with this feature: This is present when editing a template part that contains a link to a URL that's part of the current website. Specifically, it's a button in the link editing popover that takes you to the template that corresponds to that URL. (If that's still confusiong, the instructions and screenshots below should make this clearer.)
singletemplate for the current theme. You can add a bit of content there, or just leave it empty.front-pagetheme./, and one to/?p=1./link, you're taken back to thefront-pagelink, whereas for the/?p=1link, you're taken to thesingletemplate.Screenshots
Checklist: