-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Backport: Allow template duplication + concept of active templates #8063
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
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
e7fa52b to
3f3f40f
Compare
| * @since 4.7.0 | ||
| */ | ||
| function create_initial_rest_routes() { | ||
| global $wp_post_types; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add global documentation for global $wp_post_types ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would that documentation be? Looking through other instances of global $wp_post_types, I'm not seeing anything for those. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this @global array $wp_post_types List of post types. ?
Reference PR: #72020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's ok I will leave this for a follow-up because we're very close to code freeze.
e3d29b3 to
7b12619
Compare
7b12619 to
0f85749
Compare
ntsekouras
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
| // For wp_template, slugs no longer have to be unique within the same theme. | ||
| if ( 'wp_template' !== $post_type ) { | ||
| return $override_slug; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition overrides the one above it. Should the one above be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was too conservative in touching existing code
| $template_files = _get_block_templates_files( 'wp_template', $query ); | ||
| $query_result = array(); | ||
|
|
||
| // _get_block_templates_files seems broken, it does not obey the query. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we mark this somehow to see later if that function needs fixing?
* Adds the `active_templates` setting, which is an object holding the template slug as a key and template post ID as the value. * To maintain backwards compatibility, any `wp_template` (post type) not created through the new API will be activated. * `get_block_template` and `get_block_templates` have been adjusted to check `active_templates`. These functions should never return inactive templates, just like before, to maintain backwards compatibility. * The pre-existing `/templates` endpoint and sub-endpoints remain and work exactly as before. * A new endpoint `/wp_template` has been added, but this is just a regular posts controller (`WP_REST_Posts_Controller`). We do register an additional `theme` field and expose the `is_wp_suggestion` meta. * Another new endpoint `/wp_registered_template` has been added, which is read-only and lists the registered templates from themes and plugin (un-edited, without activations applied). These changes are to be iterated on. See #8063. Props ellatrix, shailu25, ntsekouras. Fixes #62755. git-svn-id: https://develop.svn.wordpress.org/trunk@61029 602fd350-edb4-49c9-b593-d223f7449a82
* Adds the `active_templates` setting, which is an object holding the template slug as a key and template post ID as the value. * To maintain backwards compatibility, any `wp_template` (post type) not created through the new API will be activated. * `get_block_template` and `get_block_templates` have been adjusted to check `active_templates`. These functions should never return inactive templates, just like before, to maintain backwards compatibility. * The pre-existing `/templates` endpoint and sub-endpoints remain and work exactly as before. * A new endpoint `/wp_template` has been added, but this is just a regular posts controller (`WP_REST_Posts_Controller`). We do register an additional `theme` field and expose the `is_wp_suggestion` meta. * Another new endpoint `/wp_registered_template` has been added, which is read-only and lists the registered templates from themes and plugin (un-edited, without activations applied). These changes are to be iterated on. See WordPress/wordpress-develop#8063. Props ellatrix, shailu25, ntsekouras. Fixes #62755. Built from https://develop.svn.wordpress.org/trunk@61029 git-svn-id: http://core.svn.wordpress.org/trunk@60365 1a063a9b-81f0-0310-95a4-ce76da25c4cd
| return $changes; | ||
| } | ||
|
|
||
| function wp_maybe_activate_template( $post_id ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from documenting, we should audit these new functions to determine which ones to make private.
* Adds the `active_templates` setting, which is an object holding the template slug as a key and template post ID as the value. * To maintain backwards compatibility, any `wp_template` (post type) not created through the new API will be activated. * `get_block_template` and `get_block_templates` have been adjusted to check `active_templates`. These functions should never return inactive templates, just like before, to maintain backwards compatibility. * The pre-existing `/templates` endpoint and sub-endpoints remain and work exactly as before. * A new endpoint `/wp_template` has been added, but this is just a regular posts controller (`WP_REST_Posts_Controller`). We do register an additional `theme` field and expose the `is_wp_suggestion` meta. * Another new endpoint `/wp_registered_template` has been added, which is read-only and lists the registered templates from themes and plugin (un-edited, without activations applied). These changes are to be iterated on. See WordPress/wordpress-develop#8063. Props ellatrix, shailu25, ntsekouras. Fixes #62755. Built from https://develop.svn.wordpress.org/trunk@61029 git-svn-id: https://core.svn.wordpress.org/trunk@60365 1a063a9b-81f0-0310-95a4-ce76da25c4cd
| } elseif ( false === $active_templates[ $slug ] ) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this nested correctly? We can only get here via the ! empty( $active_templates[ #slug ] ) on L1351 (maybe we want an isset there? )
| // Register the registered templates endpoint. For that we need to copy the | ||
| // wp_template post type so that it's available as an entity in core-data. | ||
| $wp_post_types['wp_registered_template'] = clone $wp_post_types['wp_template']; | ||
| $wp_post_types['wp_registered_template']->name = 'wp_registered_template'; | ||
| $wp_post_types['wp_registered_template']->rest_base = 'wp_registered_template'; | ||
| $wp_post_types['wp_registered_template']->rest_controller_class = 'WP_REST_Registered_Templates_Controller'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ellatrix We need to pick a shorter post type slug which is 20 characters or less, for example wp_registered_tmpl.
I discovered that this causes a very esoteric error in unit tests. It was difficult to debug, but it comes down to this: wp_registered_template is too long as a post type slug. The maximum length of posts.post_type in the database schema is 20 characters. When attempting to insert a post with this wp_registered_template post type, a database error ensues:
WordPress database error: Processing the value for the following field failed: post_type. The supplied value may be too long or contains invalid data.
This causes the unit tests in the WordPress/performance repo to start to fail. For example:
1) Test_OD_Storage_Post_Type::test_delete_all_posts
Failed asserting that false is identical to 'bar'.
/var/www/html/wp-content/plugins/performance/plugins/optimization-detective/tests/storage/test-class-od-url-metrics-post-type.php:464
The unit test doesn't fail when run in isolation. But it runs when running with all the other unit tests. This is because the Test_OD_REST_URL_Metrics_Store_Endpoint tests call rest_get_server() which now has the effect of running this code here to add wp_registered_template to $wp_post_types. The Test_OD_REST_URL_Metrics_Store_Endpoint tests aren't resetting the global $wp_post_types back to their original value (as this wasn't clear it was needed), so later when Test_OD_Storage_Post_Type runs it is looping over all get_post_types() to create some test posts of each post type to ensure their data remains intact when OD_URL_Metrics_Post_Type::delete_all_posts() is called. However, this ends up failing in the unit test when $post_type is wp_registered_template with the above database error:
$other_post_ids = array_merge(
$other_post_ids,
self::factory()->post->create_many( 10, compact( 'post_type' ) )
);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it seems this is being removed in #10425
Note: about 80% of the line changes are REST API fixture updates.
This backports the PHP changes from:
Summary:
active_templatessetting, which is an object holding the template slug as a key and template post ID as the value.get_block_templateandget_block_templateshave been adjusted to checkactive_templates. These functions should never return inactive templates, just like before, to maintain backwards compatibility./templatesendpoint and sub-endpoints remain and work exactly as before./wp_templatehas been added, but this is just a regular posts controller (WP_REST_Posts_Controller). We do register an additionalthemefield and expose theis_wp_suggestionmeta./wp_registered_templatehas been added, which is read-only and lists the registered templates from themes and plugin (un-edited, without activations applied).Note: one thing to fix is creating a "fake" post type for
wp_registered_templateto create the entity client-side. We can address this after Beta 1. We should manually create the entity and manually register the endpoint.Trac ticket: https://core.trac.wordpress.org/ticket/62755
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.