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

Jump to bottom

FSE: Fall back to next best template in hierarchy when querying through REST API #37258

Closed
wants to merge 19 commits into from
Closed

FSE: Fall back to next best template in hierarchy when querying through REST API #37258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
7d9f20e
Don't create template upon clicking Add New
ockham Dec 9, 2021
175e327
Have controller's get_item() fall back to next best template in hiera…
ockham Dec 9, 2021
1d447f8
Revert "Have controller's get_item() fall back to next best template …
ockham Dec 9, 2021
c59e70b
Make gutenberg_get_block_template fall back to use the template hiera…
ockham Dec 9, 2021
5458b94
Compute theme name
ockham Dec 9, 2021
94b23b3
Correctly set title and description
ockham Dec 9, 2021
645616d
Try simplifying Add New button
ockham Dec 9, 2021
5ed9ed7
Revert "Try simplifying Add New button"
ockham Dec 9, 2021
232f3ba
phpcbf
ockham Dec 9, 2021
275cc3c
When deleting, make sure we're not deleting a fallback template
ockham Dec 15, 2021
aaabafc
Add back original gutenberg_get_block_template and rename ours to gut…
ockham Dec 21, 2021
190b1ed
Introduce resolve flag, use in site editor
ockham Dec 21, 2021
21da936
Make fallback behavior the default
ockham Dec 21, 2021
65b6e6b
phpcbf
ockham Dec 21, 2021
5a1ef2b
Simplify new-template.js
ockham Dec 21, 2021
32403a8
Revert "phpcbf"
ockham Jun 21, 2022
ec8804e
Revert "Make fallback behavior the default"
ockham Jun 21, 2022
661fd91
Revert "Introduce resolve flag, use in site editor"
ockham Jun 21, 2022
e53448c
Revert "Add back original gutenberg_get_block_template and rename our…
ockham Jun 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 13 additions & 26 deletions lib/compat/wordpress-5.9/block-template-utils.php
View file
Open in desktop
Expand Up @@ -755,33 +755,20 @@ function gutenberg_get_block_template( $id, $template_type = 'wp_template' ) {
if ( count( $parts ) < 2 ) {
return null;
}
list( $theme, $slug ) = $parts;
$wp_query_args = array(
'post_name__in' => array( $slug ),
'post_type' => $template_type,
'post_status' => array( 'auto-draft', 'draft', 'publish', 'trash' ),
'posts_per_page' => 1,
'no_found_rows' => true,
'tax_query' => array(
array(
'taxonomy' => 'wp_theme',
'field' => 'name',
'terms' => $theme,
),
),
);
$template_query = new WP_Query( $wp_query_args );
$posts = $template_query->posts;

if ( count( $posts ) > 0 ) {
$template = _build_block_template_result_from_post( $posts[0] );

if ( ! is_wp_error( $template ) ) {
return $template;
}
list( , $slug ) = $parts;
$block_template = resolve_block_template( $slug, array(), '' );
if ( ! $block_template ) {
$block_template = resolve_block_template( 'index', array(), '' );
}
// This might give us a fallback template with a different ID,
// so we have to override it to make sure it's correct.
$block_template->id = $id;
$block_template->slug = $slug;
$default_template_types = get_default_block_template_types();
if ( array_key_exists( $slug, $default_template_types ) ) {
$block_template->title = $default_template_types[ $slug ]['title'];
$block_template->description = $default_template_types[ $slug ]['description'];
}

$block_template = get_block_file_template( $id, $template_type );
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I feel the logic change here doesn't make sense, at least not from the gutenberg_get_block_template perspective, this is a low level API to retrieve a template given its ID. Potentially it can be moved to the endpoint to create a template but not while quering. WDYT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's the big question here. I think that there's good arguments for each side:

  • It is indeed a fairly low-level API, and it seems like when given a template ID, it should just return that template (or nothing, if it can't find it).
  • OTOH, I think a point can be made that we've now identified one significant use case where that doesn't cut it, and we need the fallback. My hypothesis was that this could be the case pretty much anywhere where we'd call gutenberg_get_block_template (or "require a template given its ID") -- not just for the endpoint -- since it replicates what happens on the frontend, and it's likely that we always want to replicate that.

I'll do a quick audit of all the other occurrences of gutenberg_get_block_template. If indeed I find that the fallback makes sense in all current cases, I'll probably opt to change it to that effect. (The argument then being YAGNI: If we currently only need it with the fallback, we should only provide it that way.)

Another option would be to add a bool arg to the function signature to indicate whether or not to look for fallback templates or not, but I'd only do that if we find that we really need both versions of the function already.

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'll do a quick audit of all the other occurrences of gutenberg_get_block_template.

#37258 (comment)


/**
* Filters the queried block template object after it's been fetched.
Expand Down
2 changes: 1 addition & 1 deletion lib/compat/wordpress-5.9/class-gutenberg-rest-templates-controller.php
View file
Open in desktop
Expand Up @@ -336,7 +336,7 @@ public function delete_item_permissions_check( $request ) {
*/
public function delete_item( $request ) {
$template = gutenberg_get_block_template( $request['id'], $this->post_type );
if ( ! $template ) {
if ( ! $template || $template->id !== $request['id'] ) { // Make sure there is a template for this ID (and not just a fallback one).
return new WP_Error( 'rest_template_not_found', __( 'No templates exist with that id.', 'gutenberg' ), array( 'status' => 404 ) );
}
if ( 'custom' !== $template->source ) {
Expand Down
61 changes: 9 additions & 52 deletions packages/edit-site/src/components/add-new-template/new-template.js
View file
Open in desktop
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { filter, find, includes, map } from 'lodash';
import { filter, includes, map } from 'lodash';

/**
* WordPress dependencies
Expand All @@ -12,11 +12,9 @@ import {
MenuItem,
NavigableMenu,
} from '@wordpress/components';
import { useSelect, useDispatch } from '@wordpress/data';
import { useSelect } from '@wordpress/data';
import { store as coreStore } from '@wordpress/core-data';
import { store as editorStore } from '@wordpress/editor';
import { __ } from '@wordpress/i18n';
import { store as noticesStore } from '@wordpress/notices';

/**
* Internal dependencies
Expand All @@ -35,7 +33,7 @@ const DEFAULT_TEMPLATE_SLUGS = [

export default function NewTemplate( { postType } ) {
const history = useHistory();
const { templates, defaultTemplateTypes } = useSelect(
const { templates, defaultTemplateTypes, theme } = useSelect(
( select ) => ( {
templates: select( coreStore ).getEntityRecords(
'postType',
Expand All @@ -45,57 +43,16 @@ export default function NewTemplate( { postType } ) {
defaultTemplateTypes: select(
editorStore
).__experimentalGetDefaultTemplateTypes(),
theme: select( coreStore ).getCurrentTheme(),
} ),
[]
);
const { saveEntityRecord } = useDispatch( coreStore );
const { createErrorNotice } = useDispatch( noticesStore );
const { getLastEntitySaveError } = useSelect( coreStore );

async function createTemplate( { slug } ) {
try {
const { title, description } = find( defaultTemplateTypes, {
slug,
} );

const template = await saveEntityRecord(
'postType',
'wp_template',
{
excerpt: description,
// Slugs need to be strings, so this is for template `404`
slug: slug.toString(),
status: 'publish',
title,
}
);

const lastEntitySaveError = getLastEntitySaveError(
'postType',
'wp_template',
template.id
);
if ( lastEntitySaveError ) {
throw lastEntitySaveError;
}

// Navigate to the created template editor.
history.push( {
postId: template.id,
postType: template.type,
} );

// TODO: Add a success notice?
} catch ( error ) {
const errorMessage =
error.message && error.code !== 'unknown_error'
? error.message
: __( 'An error occurred while creating the template.' );

createErrorNotice( errorMessage, {
type: 'snackbar',
} );
}
function createTemplate( { slug } ) {
history.push( {
postId: theme.stylesheet + '//' + slug.toString(),
postType: 'wp_template',
} );
}

const existingTemplateSlugs = map( templates, 'slug' );
Expand Down