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

Rename WP_Theme::is_block_based to WP_Theme::is_block_theme and wp_is_block_template_theme to wp_is_block_theme #2014

Conversation

anton-vlasenko
Copy link

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


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.

anton-vlasenko added a commit to anton-vlasenko/wordpress-develop that referenced this pull request Dec 6, 2021
Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

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

Renaming looks good 👍 Just one note that the test can be simplified.

tests/phpunit/tests/theme/wpTheme.php Show resolved Hide resolved
function wp_is_block_template_theme() {
return is_readable( get_theme_file_path( '/block-templates/index.html' ) ) ||
is_readable( get_theme_file_path( '/templates/index.html' ) );
function wp_is_block_theme() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@anton-vlasenko As Gutenberg has to support WP 5.7 and 5.8, how will renaming this function impact the plugin? Are you planning to rename it in Gutenberg too and deprecate the original name? Or does renaming not affect Gutenberg?

Copy link
Contributor

Choose a reason for hiding this comment

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

@hellofromtonya Companion issue is here and the PR for that issue seems to have been updated in line with this page.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @costdev for pointing that out.

Copy link
Author

@anton-vlasenko anton-vlasenko Dec 6, 2021

Choose a reason for hiding this comment

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

@hellofromtonya
Yes, I'm planning to rename it in Gutenberg. I've created a PR that renames it.
IMO it doesn't make sense to keep both wp_is_block_theme and wp_is_block_template_theme functions in WordPress Core.
I think we still have time to rename it in Gutenberg before the Gutenberg 12.1.0 release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @anton-vlasenko. Part of the consolidation discuss is to help with the backports from Gutenberg to Core. So wanted to make sure whatever happens here also happens in the Gutenberg repo too, i.e. code matches.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for making sure, @hellofromtonya.
I 100% agree.
We need to be careful to make sure we don't break Gutenberg.

@hellofromtonya
Copy link
Contributor

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

Successfully merging this pull request may close these issues.

3 participants