-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
Initialize BlockTemplatesController for block themes only #48382
Conversation
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. Test this pull request with WordPress Playground. Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit. |
04fccfd
to
5fe9247
Compare
Hi @Aljullu, @tjcafferkey, @woocommerce/woo-fse Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
Asked about the failing blocks E2Es here: #48224 (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.
It's working great and code changes look good, thanks @WunderBart! Just a few questions:
- Did you evaluate placing the
wc_check_should_use_blockified_product_grid_templates
function somewhere else? I don't have a better suggestion, but thinking that it would be really nice if we could keep it inside/src/Blocks/
, given that it's related to block themes. Not sure how, thought, maybe in a new file that is only initialized in the editor? 🤔 (This is not a hard opinion, just thinking out loud to know your thoughts) - I think the testing steps should be separated on what should be tested by GlobalStep and what shouldn't (probably the database part)? Btw, I think the option name is
wc_blocks_use_blockified_product_grid_block_as_template
$this->container->get( ClassicTemplatesCompatibility::class ); | ||
$this->container->get( ArchiveProductTemplatesCompatibility::class )->init(); | ||
$this->container->get( SingleProductTemplateCompatibility::class )->init(); | ||
$this->container->get( Notices::class )->init(); | ||
$this->container->get( PTKPatternsStore::class ); | ||
|
||
if ( wc_current_theme_is_fse_theme() ) { |
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.
A classic theme might support block template parts, if I'm not wrong. So we should account for that:
if ( wc_current_theme_is_fse_theme() ) { | |
if ( wc_current_theme_is_fse_theme() || current_theme_supports( 'block-template-parts' ) ) { |
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.
Great catch, and I'm kinda surprised that all the tests have passed without it. Should we add a basic test to cover this part?
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.
Yup, good point! We definitely should. Do you want to open an issue and add it to the Project board?
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.
Sounds good. OK if we add those tests along with this PR?
The issue is here: #48468
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.
Sure thing!
Good question, @Aljullu. I figured it should be outside of the
What if we moved that check to the Options class itself? Depending on the perspective, this could either support or contradict the separation of concerns. If we'd like to separate it, maybe some sort of |
That sounds like a good plan. It's not a big deal, but what I would really like is to:
So creating an |
…late-functions.php`
This reverts commit 54a965d.
Co-authored-by: Albert Juhé Lluveras <contact@albertjuhe.com>
Co-authored-by: Albert Juhé Lluveras <contact@albertjuhe.com>
d9536dc
to
6431a9c
Compare
This PR unveiled an issue where the For testing purposes, I created a classic theme with the block template parts support and uploaded it to this PR. I've also pushed some debugging logs for easier inspection. To verify the issue:
Note If the npx wp-env run tests-cli wp config set WP_DEBUG 1
npx wp-env run tests-cli wp config set WP_DEBUG_LOG 1
npx wp-env run tests-wordpress -- touch wp-content/debug.log
npx wp-env run tests-wordpress -- chmod 666 wp-content/debug.log
npx wp-env run tests-wordpress -- tail -f wp-content/debug.log @Aljullu @gigitux @tjcafferkey, how do you suggest approaching this issue? Should we move the BTC initialization somewhere else? Or maybe the entire |
Hmm, I am trying to think if there is another method in which we can reliably determine if we should initialize the BTC.
I think if we can't find another way then it would make more sense/be safer to move the BTC initialization rather than the Bootstrap one. But only if we can't find another way around this limitation. |
Would there be any danger to doing something similar to this and initializing the BTC when the |
Good idea! I'm trying this approach in #48905 and it seems to work fine, I did even test it with the modified Storefront version @WunderBart created and I couldn't spot anything off. We can either hook into I will continue working on this tomorrow, as there are some tests failing, but hopefully I'll be able to open the PR for review before EOW. 🤞 |
Well, not really. We need to use In any case, I opened the PR for review in #48905. 🙂 |
Closing in favor of #48905. Thanks @Aljullu and @tjcafferkey! 🙇 |
What?
Closes #46463
Supersedes #48385
wc_blocks_use_blockified_product_grid_block_as_template
option check from BTC towc-template-functions.php
as it's shared between classic and block themes.How to test
For the
wc_blocks_use_blockified_product_grid_block_as_template
option part, ensure:For the BTC initialization switch part: