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.

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

Backport: Block template utils and rest templates controller #3221

Conversation

ntsekouras
Copy link

@ntsekouras ntsekouras commented Sep 9, 2022

@ntsekouras
Copy link
Author

ntsekouras commented Sep 12, 2022

Do not merge before this GB PR: WordPress/gutenberg#44085


edit:

The GB PR has merged.

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Thank you, @ntsekouras! This LGTM; however, we need to wait for Gutenberg 14.1 (which is the first release to contain WordPress/gutenberg#44085) to be released before merging this PR. Otherwise, a WP install running Core trunk and Gutenberg stable will fatal because of the duplicate get_template_hierarchy definition.

Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

First pass through the code. Most changes requested are for compliance to Core's coding / testing standards and/or for consistency.

@hellofromtonya
Copy link
Contributor

@spacedmonkey Are you happy with the REST code in this PR?

'icon' => array(
'description' => __( 'The icon for the post type.' ),
'type' => 'string',
'context' => array( 'view', 'edit', 'embed' ),
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be in the view context? The icon is not public right now?

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean public? I think it would be good to have in view as it can be useful in some cases. Do you see any drawback being in view context?

Copy link
Member

Choose a reason for hiding this comment

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

If you make a property as view it means it can't be viewed pubilically. Do we want to expose this data publically?

Copy link
Author

Choose a reason for hiding this comment

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

I see no harm in it, but if you have a strong opinion on this, I can remove it.

}
$hierarchy = get_template_hierarchy( $request['slug'], $request['is_custom'], $request['template_prefix'] );
$fallback_template = resolve_block_template( $request['slug'], $hierarchy, '' );
return rest_ensure_response( $fallback_template );
Copy link
Member

@spacedmonkey spacedmonkey Sep 20, 2022

Choose a reason for hiding this comment

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

Suggested change
return rest_ensure_response( $fallback_template );
$response = $this->prepare_item_for_response( $fallback_template, $request );
return rest_ensure_response( $response );

Please ensure that response is run through prepare_item_for_response, so the response is formatted correctly.

Copy link
Author

Choose a reason for hiding this comment

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

I'll update but this change will also need a GB PR(therefore a packages update) to retrieve the content from content.raw.

Copy link
Member

Choose a reason for hiding this comment

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

Make the change and backport it. I can approve the change there as well. Change the properties / shape of response can / will cause problems down the road.

Copy link
Author

Choose a reason for hiding this comment

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

GB PR is here: WordPress/gutenberg#44299 --cc @ockham

@ockham
Copy link
Contributor

ockham commented Sep 20, 2022

Thank you @spacedmonkey and @ntsekouras!

Unfortunately, it seems like unit tests are failing:

1) Tests_REST_WpRestTemplatesController::test_get_template_fallback
Trying to get property 'slug' of non-object

/var/www/tests/phpunit/tests/rest-api/wpRestTemplatesController.php:766
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97
/var/www/vendor/bin/phpunit:118

@@ -221,6 +221,10 @@ public function prepare_item_for_response( $item, $request ) {
$data['slug'] = $post_type->name;
}

if ( in_array( 'icon', $fields, true ) ) {
$data['icon'] = $post_type->menu_icon;
Copy link
Member

Choose a reason for hiding this comment

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

If this value is null, should it maybe default to something?

Copy link
Author

Choose a reason for hiding this comment

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

I think it's okay to be null since it's not required and the consumers can decide what to do with it. For example in php core for post types we assign some default icon if it's in the admin menu, but in the site editor we assign different default icon..

@hellofromtonya
Copy link
Contributor

Currently reviewing. When ready, will prep the commit.

@hellofromtonya hellofromtonya force-pushed the backport/expand-created-templates-site-editor branch from 0bda281 to 5ba2daf Compare September 20, 2022 19:12
@hellofromtonya
Copy link
Contributor

Rebased the branch on top of the latest trunk.

After rebasing on top of trunk, the PR reverted some
of the changes in wp-includes/block-template-utils.php
that were added in changest 54184.

This commit reapplies the code from that changeset.

It also cleans up in one test.
Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

LTGM 👍 Ready for commit, which I'm prepping next.

@hellofromtonya
Copy link
Contributor

Note: The npm CI job failure is unrelated to this PR.

@hellofromtonya
Copy link
Contributor

Committed via changeset https://core.trac.wordpress.org/changeset/54269. Thank you everyone for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants