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

Honor 'template_hierarchy' filters when creating a template in the Site Editor #6322

Conversation

Aljullu
Copy link

@Aljullu Aljullu commented Mar 26, 2024

Testing steps

  1. Add this code snippet to your site. You can use the Code Snippets plugin:
add_filter( 'page_template_hierarchy', function ( $templates ) {
        return ['single'];
} );
  1. Visit any page in the frontend. You will notice the Single template is used, instead of the Page template.
  2. Now, go to Appearance > Editor > Templates > Add New Template > Pages and select any page.
  3. Verify the suggested content to start with in the Choose a pattern screen is from the Single template, instead of the Page template.
Before After

Trac ticket: https://core.trac.wordpress.org/ticket/60846


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.

Copy link

github-actions bot commented Mar 26, 2024

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props aljullu, mukesh27, ntsekouras, jorgefilipecosta, gziolo.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@Aljullu Aljullu force-pushed the 60846-template_hierarchy-filter-choose-a-pattern branch 2 times, most recently from 379772f to 82149d6 Compare March 26, 2024 13:09
Copy link

Test using WordPress Playground

The 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

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@Aljullu
Copy link
Author

Aljullu commented Mar 26, 2024

cc @gziolo in case you have some thoughts on the issue and the approach taken in this PR.

Also cc @ntsekouras as I think you wrote most of the code this PR is based on here: WordPress/gutenberg#42520.

Thanks in advance, folks!

}
$valid_template_types = array( '404', 'archive', 'attachment', 'author', 'category', 'date', 'embed', 'frontpage', 'home', 'index', 'page', 'paged', 'privacypolicy', 'search', 'single', 'singular', 'tag', 'taxonomy' );
if ( in_array( $template_type, $valid_template_types ) ) {
return apply_filters( "{$template_type}_template_hierarchy", $template_hierarchy );
Copy link
Member

Choose a reason for hiding this comment

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

The document is missing for filter.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for letting me know, I just added it in e742973. Given that this filter is documented in wp-includes/template.php, my understanding is that adding a comment like This filter is documented in wp-includes/template.php is the correct approach? I took these two examples (1, 2) as a reference.

@gziolo
Copy link
Member

gziolo commented Mar 29, 2024

Also cc @ntsekouras as I think you wrote most of the code this PR is based on here: WordPress/gutenberg#42520.

Nik might have better insights as he worked on the code.

Copy link

@ntsekouras ntsekouras 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 the PR @Aljullu! In general I think it's a fine addition to apply the filter. I'm wondering though why not start with a PR in Gutenberg repo. This issue it seems will be included in 6.6 and is needed in GB too.

} else {
list( $template_type ) = explode( '-', $slug );
}
$valid_template_types = array( '404', 'archive', 'attachment', 'author', 'category', 'date', 'embed', 'frontpage', 'home', 'index', 'page', 'paged', 'privacypolicy', 'search', 'single', 'singular', 'tag', 'taxonomy' );

Choose a reason for hiding this comment

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

I guess you got these from: https://developer.wordpress.org/reference/hooks/type_template_hierarchy/, right?

The logic for checking whether to apply the filter or not, I guess could go on top of the function, since we already have some early returns.

Regarding the actual logic for template_type, I'd love some eyes from @jorgefilipecosta, since while I introduced this function, he has updated it afterwards.

Copy link
Author

Choose a reason for hiding this comment

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

I guess you got these from: https://developer.wordpress.org/reference/hooks/type_template_hierarchy/, right?

Yup, directly from the documented code, in fact:

https://github.com/WordPress/wordpress-develop/blob/6.4/src/wp-includes/template.php#L39-L56

The logic for checking whether to apply the filter or not, I guess could go on top of the function, since we already have some early returns.

Good catch, I missed those early returns. In c83b3ac I updated the code so we call apply_filter() on those early returns as well. It's not 100% clear to me if that's what you were proposing or you have a better idea?

Choose a reason for hiding this comment

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

That was it, thank you! We just need a quick look from @jorgefilipecosta now :)

@Aljullu Aljullu force-pushed the 60846-template_hierarchy-filter-choose-a-pattern branch from 74251ec to a9065d5 Compare April 1, 2024 12:07
@Aljullu Aljullu force-pushed the 60846-template_hierarchy-filter-choose-a-pattern branch from a9065d5 to c83b3ac Compare April 1, 2024 12:19
@Aljullu
Copy link
Author

Aljullu commented Apr 1, 2024

Thanks for taking a look, folks! I updated the PR based on the provided feedback.

I'm wondering though why not start with a PR in Gutenberg repo. This issue it seems will be included in 6.6 and is needed in GB too.

I'm probably missing something from how the Gutenberg → WordPress code transfer works. While I know these functions were first introduced in Gutenberg, I thought they were moved to WordPress core and new contributions/updates should be done here? Or is there another way of approaching this?


Note: some PHP tests are failing, but it doesn't seem related to this PR and they are failing in other PRs too.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

The changes look good 👍 Thank you @Aljullu for working on this. I think this PR can be added to core. It deserves some testing on the Gutenberg plugin, but testing the function on Gutenberg plugin is not trivial as it is not there anymore. I will look into hot to reverse port this PR to Gutenberg and propose a solution today.

@ntsekouras
Copy link

but testing the function on Gutenberg plugin is not trivial as it is not there anymore. I will look into hot to reverse port this PR to Gutenberg and propose a solution today.

Thanks for looking here @jorgefilipecosta and for looking for the GB PR. I think we need to override just one class function that uses this one.

@Aljullu
Copy link
Author

Aljullu commented Apr 8, 2024

@jorgefilipecosta thanks for bringing this change to Gutenberg! Now that this has been merged there, should I close this PR and the associated trac ticket?

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Apr 8, 2024

@jorgefilipecosta thanks for bringing this change to Gutenberg! Now that this has been merged there, should I close this PR and the associated trac ticket?

Yes, This PR can be closed as it was committed to both Gutenberg and core trunk at f18c917.

@Aljullu Aljullu closed this Apr 9, 2024
@Aljullu Aljullu deleted the 60846-template_hierarchy-filter-choose-a-pattern branch April 9, 2024 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants