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

Introduce wp_template post type and use it for full-site templates #17513

Merged
merged 7 commits into from
Sep 27, 2019

Conversation

felixarntz
Copy link
Member

@felixarntz felixarntz commented Sep 22, 2019

This PR is the initial piece for addressing #17512. It registers a wp_template post type, as a foundation for storing template-wide block data.

Types of changes

  • Register wp_template post type.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@felixarntz felixarntz added Framework Issues related to broader framework topics, especially as it relates to javascript Customization Issues related to Phase 2: Customization efforts labels Sep 22, 2019
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

😍Thanks for your work on this PR Felix. I tried running some quick tests on it, it's a great start.

To ease the reviews and to keep the discussions focused, I'd like to propose that we split this PR into three separate PRs.

  • One for the CPT addition (it would be a blocker for the two others)
  • One for the template hierarchy matching and rendering behavior
  • One for the temporary UI to edit templates. I think this UI might not be the end UI we end up with as we might prefer something more tied to the post/page editing instead but I think it's a great way to start building these templates as we move forward with the parallel work in terms of editor refactoring, FSE blocks...

What do you think?

if ( ! is_array( $_wp_current_template_hierarchy ) ) {
$_wp_current_template_hierarchy = $templates;
} else {
$_wp_current_template_hierarchy = array_merge( $_wp_current_template_hierarchy, $templates );
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we merge here? Merging two ordered lists like this doesn't seem like it would result in the right ordering.

Copy link
Member Author

Choose a reason for hiding this comment

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

How so? As these are regular indexed arrays, the $templates one would always be appended to the $_wp_current_template_hierarchy one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly, I'm not even sure if this runs more than once, but the code looks like it expects it too and if on every run $templates contains a priority ordered list, what guarantees that the head of the next list is the tail of the previous list?

* @param string $template_file Original template file. Will be overridden.
* @return string Path to the canvas file to include.
*/
function gutenberg_find_template( $template_file ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we still load a template file when all available wp_templates have less preference in the hierarchy?

That was one of the goals of #4659.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm certainly open to it, but let's discuss first. It leads to the question whether we would want to enable the usage of wp_template blocks in an "all-or-nothing" mode (e.g. only if something like add_theme_support( 'block-templates' ) is given) or whether we want to go for a hybrid. The latter at first glance certainly sounds nicer, but it might open a can of worms, as I think supporting both "old" PHP file templates and block templates would make theme development overly complex. Thoughts on this @youknowriad?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the latter approach as a way for theme authors to opt-in iteratively into block-templates. For example a first step could be to adopt it for "static page templates" only...

Even in terms of shipping strategy, I feel like we can decide to only allow block templates in static ones first and then open to the whole hierarchy as a second step. It's good to start thinking about these discussions but I'm not too concerned at the moment, this will come in time as we move forward, I feel we have room to adapt our approach as we move forward with the implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the hybrid approach is important, because it allows users to still leverage wp_templates in old themes that will never be updated.

@felixarntz
Copy link
Member Author

@youknowriad

To ease the reviews and to keep the discussions focused, I'd like to propose that we split this PR into three separate PRs.

  • One for the CPT addition (it would be a blocker for the two others)
  • One for the template hierarchy matching and rendering behavior
  • One for the temporary UI to edit templates. I think this UI might not be the end UI we end up with as we might prefer something more tied to the post/page editing instead but I think it's a great way to start building these templates as we move forward with the parallel work in terms of editor refactoring, FSE blocks...

Makes sense, I'll make these updates tomorrow. I suggest the following "PR hierarchy":

  • The CPT registration PR (add/wp-template-post-type) against master
  • The UI PR (add/wp-template-post-type-ui) against add/wp-template-post-type
  • The hierarchy matching (add/wp-template-hierarchy-matching) against add/wp-template-post-type

That would be because the last two each depend on the first.

@felixarntz
Copy link
Member Author

@youknowriad I've simplified this PR to only register the post type itself (without any UI), and opened #17625 and #17626 against this branch with the UI and template loader changes respectively.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

This is looking good to me 👍

@felixarntz felixarntz merged commit 13e1519 into master Sep 27, 2019
@@ -124,6 +135,7 @@ function gutenberg_experiments_editor_settings( $settings ) {
'__experimentalEnableLegacyWidgetBlock' => $experiments_exist ? array_key_exists( 'gutenberg-widget-experiments', get_option( 'gutenberg-experiments' ) ) : false,
'__experimentalEnableMenuBlock' => $experiments_exist ? array_key_exists( 'gutenberg-menu-block', get_option( 'gutenberg-experiments' ) ) : false,
'__experimentalBlockDirectory' => $experiments_exist ? array_key_exists( 'gutenberg-block-directory', get_option( 'gutenberg-experiments' ) ) : false,
'__experimentalEnableFullSiteEditing' => $experiments_exist ? array_key_exists( 'gutenberg-full-site-editing', get_option( 'gutenberg-experiments' ) ) : false,
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a few more places where we have to update things because of this change.

See: 2aa1d04

$args = array(
'labels' => $labels,
'description' => __( 'Templates to include in your theme.', 'gutenberg' ),
'public' => false,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is already the default.

'labels' => $labels,
'description' => __( 'Templates to include in your theme.', 'gutenberg' ),
'public' => false,
'has_archive' => false,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is already the default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Customization Issues related to Phase 2: Customization efforts Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants