Skip to content
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.

Sign up for GitHub

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

Edit Site: Fetch template parts in Template Switcher from REST API #21878

Merged
merged 1 commit into from
Jun 9, 2020
Merged

Edit Site: Fetch template parts in Template Switcher from REST API #21878

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/template-loader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,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' ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅 )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think it's just something to be looked at some point.

foreach ( parse_blocks( $current_template_post->post_content ) as $block ) {
$template_part_ids = array_merge( $template_part_ids, create_auto_draft_for_template_part_block( $block ) );
}
Expand Down
48 changes: 45 additions & 3 deletions lib/template-parts.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,14 +141,22 @@ function gutenberg_render_template_part_list_table_column( $column_name, $post_i


/**
* Filter for adding a `theme` parameter to `wp_template_part` queries.
* Filter for adding a `resolved`, a `template`, and a `theme` parameter to `wp_template_part` queries.
*
* @param array $query_params The query parameters.
* @return array Filtered $query_params.
*/
function filter_rest_wp_template_part_collection_params( $query_params ) {
$query_params += array(
'theme' => array(
'resolved' => array(
'description' => __( 'Whether to filter for resolved template parts.', 'gutenberg' ),
'type' => 'boolean',
),
'template' => array(
'description' => __( 'The template slug for the template that the template part is used by.', 'gutenberg' ),
'type' => 'string',
),
'theme' => array(
'description' => __( 'The theme slug for the theme that created the template part.', 'gutenberg' ),
'type' => 'string',
),
Expand All @@ -158,13 +166,46 @@ function filter_rest_wp_template_part_collection_params( $query_params ) {
apply_filters( 'rest_wp_template_part_collection_params', 'filter_rest_wp_template_part_collection_params', 99, 1 );

/**
* Filter for supporting the `theme` parameter in `wp_template_part` queries.
* Filter for supporting the `resolved`, `template`, and `theme` parameters in `wp_template_part` queries.
*
* @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 ) {
/**
* Unlike `filter_rest_wp_template_query`, we resolve queries also if there's only a `template` argument set.
* The difference is that in the case of templates, we can use the `slug` field that already exists (as part
* of the entities endpoint, wheras for template parts, we have to register the extra `template` argument),
* 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)).
*
* A template parts query with a `template` arg but not a `resolved` one is conceivable, but probably wouldn't be
* very useful: It'd be all template parts for all templates matching that `template` slug (including auto-drafts etc).
*
* @see filter_rest_wp_template_query
* @see filter_rest_wp_template_part_collection_params
* @see https://github.com/WordPress/gutenberg/pull/21878#discussion_r436961706
*/
if ( $request['resolved'] || $request['template'] ) {
$template_part_ids = array( 0 ); // Return nothing by default (the 0 is needed for `post__in`).
$template_types = $request['template'] ? array( $request['template'] ) : get_template_types();

foreach ( $template_types as $template_type ) {
// Skip 'embed' for now because it is not a regular template type.
if ( in_array( $template_type, array( 'embed' ), true ) ) {
continue;
}

$current_template = gutenberg_find_template_post_and_parts( $template_type );
if ( isset( $current_template ) ) {
$template_part_ids = $template_part_ids + $current_template['template_part_ids'];
}
}
$args['post__in'] = $template_part_ids;
$args['post_status'] = array( 'publish', 'auto-draft' );
}

if ( $request['theme'] ) {
$meta_query = isset( $args['meta_query'] ) ? $args['meta_query'] : array();
$meta_query[] = array(
Expand All @@ -174,6 +215,7 @@ function filter_rest_wp_template_part_query( $args, $request ) {

$args['meta_query'] = $meta_query;
}

return $args;
}
add_filter( 'rest_wp_template_part_query', 'filter_rest_wp_template_part_query', 99, 2 );
8 changes: 0 additions & 8 deletions packages/edit-site/src/components/header/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,33 +36,27 @@ export default function Header( {
const {
deviceType,
hasFixedToolbar,
homeTemplateId,
templateId,
templatePartId,
templateType,
templatePartIds,
page,
showOnFront,
} = useSelect( ( select ) => {
const {
__experimentalGetPreviewDeviceType,
isFeatureActive,
getHomeTemplateId,
getTemplateId,
getTemplatePartId,
getTemplateType,
getTemplatePartIds,
getPage,
getShowOnFront,
} = select( 'core/edit-site' );
return {
deviceType: __experimentalGetPreviewDeviceType(),
hasFixedToolbar: isFeatureActive( 'fixedToolbar' ),
homeTemplateId: getHomeTemplateId(),
templateId: getTemplateId(),
templatePartId: getTemplatePartId(),
templateType: getTemplateType(),
templatePartIds: getTemplatePartIds(),
page: getPage(),
showOnFront: getShowOnFront(),
};
Expand Down Expand Up @@ -116,11 +110,9 @@ export default function Header( {
/
</div>
<TemplateSwitcher
templatePartIds={ templatePartIds }
page={ page }
activeId={ templateId }
activeTemplatePartId={ templatePartId }
homeId={ homeTemplateId }
isTemplatePart={ templateType === 'wp_template_part' }
onActiveIdChange={ setTemplate }
onActiveTemplatePartIdChange={ setTemplatePart }
Expand Down
101 changes: 56 additions & 45 deletions packages/edit-site/src/components/template-switcher/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { useSelect } from '@wordpress/data';
import { useState } from '@wordpress/element';
import { useRegistry, useSelect } from '@wordpress/data';
import { useEffect, useState } from '@wordpress/element';
import {
Tooltip,
DropdownMenu,
Expand All @@ -16,6 +16,7 @@ import { Icon, home, plus, undo } from '@wordpress/icons';
/**
* Internal dependencies
*/
import { findTemplate } from '../../utils';
import TemplatePreview from './template-preview';
import ThemePreview from './theme-preview';

Expand Down Expand Up @@ -46,11 +47,9 @@ function TemplateLabel( { template, homeId } ) {
}

export default function TemplateSwitcher( {
templatePartIds,
page,
activeId,
activeTemplatePartId,
homeId,
isTemplatePart,
onActiveIdChange,
onActiveTemplatePartIdChange,
Expand All @@ -70,52 +69,64 @@ export default function TemplateSwitcher( {
setThemePreviewVisible( () => false );
};

const registry = useRegistry();
const [ homeId, setHomeId ] = useState();

useEffect( () => {
findTemplate(
'/',
registry.__experimentalResolveSelect( 'core' ).getEntityRecords
).then(
( newHomeId ) => setHomeId( newHomeId ),
() => setHomeId( null )
);
Comment on lines +76 to +82
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could wrap it in a custom hook too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolvers need to be tied to the store state, and it would be awkward to have these temporary values there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather avoid adding templateId as a parameter only to remove it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙁

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave it for later then.

}, [ registry ] );

const { currentTheme, template, templateParts } = useSelect(
( select ) => {
const { getCurrentTheme, getEntityRecord } = select( 'core' );
const {
getCurrentTheme,
getEntityRecord,
getEntityRecords,
} = select( 'core' );

const _template = getEntityRecord(
'postType',
'wp_template',
activeId
);

return {
currentTheme: getCurrentTheme(),
template: {
label: _template ? (
<TemplateLabel
template={ _template }
homeId={ homeId }
/>
) : (
__( 'Loading…' )
),
value: activeId,
slug: _template ? _template.slug : __( 'Loading…' ),
content: _template?.content,
},
templateParts: templatePartIds.map( ( id ) => {
const templatePart = getEntityRecord(
'postType',
'wp_template_part',
id
);
return {
label: templatePart ? (
<TemplateLabel template={ templatePart } />
) : (
__( 'Loading…' )
),
value: id,
slug: templatePart
? templatePart.slug
: __( 'Loading…' ),
};
} ),
template: _template,
templateParts: _template
? getEntityRecords( 'postType', 'wp_template_part', {
resolved: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
resolved: true,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #21878 (comment)

template: _template.slug,
} )
: null,
};
},
[ activeId, templatePartIds, homeId ]
[ activeId ]
);

const templateItem = {
label: template ? (
<TemplateLabel template={ template } homeId={ homeId } />
) : (
__( 'Loading…' )
),
value: activeId,
slug: template ? template.slug : __( 'Loading…' ),
content: template?.content,
};

const templatePartItems = templateParts?.map( ( templatePart ) => ( {
label: <TemplateLabel template={ templatePart } />,
value: templatePart.id,
slug: templatePart.slug,
} ) );

const overwriteSlug =
TEMPLATE_OVERRIDES[ page.type ] &&
page.slug &&
Expand All @@ -125,10 +136,10 @@ export default function TemplateSwitcher( {
slug: overwriteSlug,
title: overwriteSlug,
status: 'publish',
content: template.content.raw,
content: templateItem.content.raw,
} );
const revertToParent = async () => {
onRemoveTemplate( template.value );
onRemoveTemplate( activeId );
};
return (
<>
Expand All @@ -141,8 +152,8 @@ export default function TemplateSwitcher( {
label={ __( 'Switch Template' ) }
toggleProps={ {
children: ( isTemplatePart
? templateParts
: [ template ]
? templatePartItems
: [ templateItem ]
).find(
( choice ) =>
choice.value ===
Expand All @@ -156,18 +167,18 @@ export default function TemplateSwitcher( {
<MenuItem
onClick={ () => onActiveIdChange( activeId ) }
>
{ template.label }
{ templateItem.label }
</MenuItem>
{ overwriteSlug &&
overwriteSlug !== template.slug && (
overwriteSlug !== templateItem.slug && (
<MenuItem
icon={ plus }
onClick={ overwriteTemplate }
>
{ __( 'Overwrite Template' ) }
</MenuItem>
) }
{ overwriteSlug === template.slug && (
{ overwriteSlug === templateItem.slug && (
<MenuItem
icon={ undo }
onClick={ revertToParent }
Expand All @@ -178,7 +189,7 @@ export default function TemplateSwitcher( {
</MenuGroup>
<MenuGroup label={ __( 'Template Parts' ) }>
<MenuItemsChoice
choices={ templateParts }
choices={ templatePartItems }
value={
isTemplatePart
? activeTemplatePartId
Expand Down