Refactor BlockTemplatesController #44537
Conversation
Aljullu
added
focus: template
Aljullu
changed the title
github-actions
bot
added
the
plugin: woocommerce
Test Results SummaryCommit SHA: 1326883
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
Aljullu
force-pushed
the
update/BlockTemplatesController-refactor
branch
from
b55c752
to
d8261bf
Compare
woocommercebot
requested review from
a team and
dinhtungdu
and removed request for
a team
|
Hi @gigitux, @dinhtungdu, @tjcafferkey, 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: |
Aljullu
force-pushed
the
update/BlockTemplatesController-refactor
branch
2 times, most recently
from
028714b
to
fdf0310
Compare
|
Thanks @Aljullu, great work on this. I will make sure I get around to reviewing this today/tomorrow! |
|
@Aljullu It looks like with this PR we have just moved the individual template rendering logic into its own controller. Can you confirm that's right? Whereas a bunch of the logic still remains in the main I also notice that we are still creating fallbacks for alternative product archive templates. For example if the Product Catalog has been customised it will be used for the other archive templates such as Products by Category etc. I think it was agreed that we could do the following during this refactor:
I think if we achieved these two things as part of this PR it would do two things:
My only concern is with this PR at the moment is that we have created a bunch of new files and code and spread the complexity rather than reduce it. |
plugins/woocommerce/src/Blocks/Templates/AbstractTemplatePart.php
Outdated
Show resolved
Hide resolved
| /** | ||
| * Renders the default block template from Woo Blocks if no theme templates exist. | ||
| */ | ||
| public function render_block_template() { |
There was a problem hiding this comment.
Can we not use this class more generically to render the other product archive templates and remove the additional classes?
There was a problem hiding this comment.
I guess it could be done, but what would be the benefit of doing so?
One of the benefits I see from having one file per template is that there is a clear relation between:
- The template PHP file where we define title, description, specific hooks...
- Templates stored in the database.
- Template files from themes.
All of them sharing the same slug. While we could move the logic from other archive templates into this file, that would break the 1:1 relation between the above, which IMO would make things more confusing. 🤔
There was a problem hiding this comment.
I guess the change only makes sense if/once we remove the other product archive templates altogether and render the Product Catalog template in every case unless the user has added a specific template via the Add New UI in the Site Editor.
There was a problem hiding this comment.
Thanks for your work!
As @tjcafferkey noticed, in the Site Editor, we decided to list only the Product Catalog in the Site Editor. As Tom sustains (and I agree), this would allow us to reduce some complexity.
I added a few comments on the BlockTemplatesController containing specific functions for the Product Catalog template and the fallback system. I don't recall the logic (maybe we can remove them), but we should keep the Controller with general-purpose logic. Any dedicated logic for a specific template should be in the class-specific of that template. I'm afraid if we keep some dedicated logic in this class, there is the risk that, in the future, we start to add dedicated logic for a specific template, and we will be back to where we started.
Also, I noticed that the priority of the templates during the theme switch differs from WordPress's behavior.
In this video, using TT4, I update the Index template and the Single Product Template. After the switch to Tsubaki:
- The Index template uses the Tsubaki template.
- The Single Product template uses the previously saved version. Instead, it seems that the correct behavior is that it should use the Tsubaki one.
Screen.Capture.Feb.15.at.10-47-21.mp4
|
Thanks for taking a look and all the suggestions, @tjcafferkey!
Correct, the idea is that all the logic specific to a single template is in the template file, while the logic which is template-agnostic is kept in
Edit: ok, I see @gigitux also suggested moving the "fall back" logic out of
My understanding is that you are referring to what's described in these two issues: #42531 and #42532, right? What I'm not sure to understand is how that would simplify the code. My understanding is that it's mostly a UI change (how we present the templates in the Site Editor UI), but the "fallback logic" will still need to exist, no? There will still be the case that the user or the theme have customized a specific template (ie: Products by Category) and we will need some logic to decide which template is loaded both in the frontend and in the Site Editor. I might be missing some previous conversations on this, though. Don't get me wrong, I'm happy to include those changes in this PR, but my understanding is that they would only increase the lines of code modified and would increase the testing surface area. As mentioned, the PR is already 1000 lines of code and I didn't want it to grow bigger to make it easier to review. 😟
It's true this PR creates a lot more files, but all of them are quite small and there is not much complexity in them: each template file defines the slug, title, description and any hooks required by that specific template. Are there any specific areas where you think we could reduce the complexity? I guess the fall backing system 😄 , but as mentioned above I'm not sure if it could be simplified much. |
|
Thanks for your review as well, @gigitux! (Sorry I answered to @tjcafferkey before seeing your reply)
As mentioned in Tom's reply. Could you clarify how that would simply the codebase? What I understand from the issues is that it's mostly a UI change (and that's why I left it for a task after the refactor), but I might be missing something.
Makes sense. My initial thought was to create an agnostic
Did you investigate if this is a regression? I could reproduce the same in WooCommerce 8.5 and seems to be an instance of what's described in #42181. (Customized WooCommerce templates persist after a theme switch) |
Mainly, I was referring to the BlockTemplatesController. Moving the code to the specific template makes the class easier to read and understand.
Thanks!
Yeah, it is a regression. I was convicted that the refactor included this kind of bug fixing. It is fine for me if we decide to work on this fix on a dedicated PR 👍 |
|
@gigitux @Aljullu can you remind me what logic like this is for again? What would make a template eligible or ineligible for a fallback? My thinking was (and maybe I'm over simplifying it):
I am just trying to identify if we can remove things like this. |
I started working on #42531 and #42532 in parallel. Hopefully, by the end of tomorrow I will publish another PR (or update this one) with these changes.
Sorry for not being clear with that. My goal with the refactor was not to fix all existing bugs, but just to improve the code and leave things working as they did, that means not intentionally fixing bugs. #44264 is fixed in this PR, but it was mostly a fix that we got "for free".
I would need to check, but AFAIK it doesn't. Gutenberg supports specific taxonomy templates (ie: a template for the Clothing category). But it's not possible to create a generic "Product Category" template. |
I don't recall tbh. From a quick look, it seems that we checked the slug of the template: https://github.com/woocommerce/woocommerce/blob/e49543b03cfbfddfad96643acc7080b6bcd82f86/plugins/woocommerce/src/Blocks/Utils/BlockTemplateUtils.php/#L495-L506
No worries! Feel free to create a dedicated PR 👍 ! I think that we should aim to make the Controller generic-purpose; the rest of the feedback/suggestions could be implemented with dedicated PRs! |
Aljullu
force-pushed
the
update/BlockTemplatesController-refactor
branch
from
b53dc77
to
d8b07a5
Compare
|
I'm just doing a smoke test of the PR as I don't have enough context to evaluate the implementation. Things look good so far. Just 1 thing I noticed when using Tsubaki, the checkout templates still default to WooCommerce. Is this expected? Also, in this image, I notice that Tsubaki is written in 2 ways: |
Completely agree! Thanks for your effort! |
This reverts commit 866c9b9.
There was a problem hiding this comment.
@Aljullu Thanks so much for working on this PR. The code change (especially class structure and organization) looks great to me.
Testing the PR again with the latest commits, I can see the PR passes the testing instruction except a potential issue, please refer to my testing log below to grab the full context. I'm happy to approve this PR once we're clear about that.
With a classic theme (ie: Storefront).
I tested the following templates and they all loaded as normal.
- Shop page: ✅
- Single product page: ✅
- Cart page: ✅
- Checkout page: ✅
- Category: ✅
- Thank you page: ✅
With a block theme without custom WooCommerce templates (ie: Twenty Twenty-Four)
- I can visit shop, single product, cart, and checkout pages.
- I can edit templates in the editor and see the change applied on the front end.
- I can revert the templates and both reverted or customized work as expected. I have a small issue though, but I don't think it belongs to the scope of this PR:
- When I edit the product catalog template, all templates inheriting it are marked as customized, like Category or Tag. If I revert one of those templates, all templates and the Product Catalog get reverted as well.
With a block theme with custom WooCommerce templates (ie: Tsubaki)
I first tested with Twenty Twenty-Four and had some customized templates. After I installed and activated Tsubaki, Woo still used the customized templates from Twenty Twenty-Four. Is that expected behavior? If not, does it happen on trunk?
Apart from the above issue, I can edit the templates and see the change applied on the frontend. I can also revert the changes without error.
Make sure Mini-Cart template part from the theme is correctly assigned to the Mini-Cart template part area (#44264)
I can see the Mini Cart provide by Tsubaki is assigned to the correct area.
|
Thanks for the in-depth review, @dinhtungdu!
Good catch! I tested and it's the same in
If I'm not wrong, this is what @gigitux highlighted earlier and what the issue #42181 is about. Customized WooCommerce templates persist after a theme switch, while any non-WC customized template is "attached" to the theme, so they don't persist after a theme switch. That causes some weird issues with customized WooCommerce templates after a theme switch, for example, that they are pointing to the wrong header template part (that's what's described in #42181). But given that this issue is also present in |
There was a problem hiding this comment.
I've followed the testing instructions, and everything seems to work as expected! 🎉
I did find two issues, though, but they're likely unrelated to / out of context of this PR:
-
When opening the Product Catalog template with Tsubaki for the first time, I was presented with the non-blockified template. It transformed seamlessly, though, so no real issue here.
-
Each time I open the All Templates view, the list seems to be in a different order. For example, when I edited the Product Catalog (Tsubaki) and went back to All Templates, it got pushed to the very top, while the rest of the templates don't seem to be in any specific order:
As agreed with @Aljullu, this one will be reported as a separate issue.
gigitux
dismissed
their stale review
I removed myself as a reviewer given that I didn't follow the progress of this PR given my AFK
|
Thanks for the reviews, folks! I will go ahead and merge this PR. |
github-actions
bot
added
the
needs: analysis
Reported in #45180 |
alvarothomas
added
needs: external testing
* Cleanup BlockTemplatesController constructor * Remove unnecessary elseif * Remove unnecessary param comment * Move Single Product Template responsibilities to the SingleProductTemplate.php file * Move Mini-Cart Template reposnibility to the MiniCartTemplate.php file * Create the other template files * Code cleanup * Move template redirect into template files * PHP cleanup * Add changelog entry * Fix PHP tests * Replace hardcoded 'archive-product' slug with ProductCatalogTemplate::SLUG * Make it so AbstractTemplatePart extends AbstractTemplate * Register templates in BlockTemplatesRegistry * Fix slug usage in AbstractPageTemplate.php * Add get_template_title and get_template_description methods to AbstractTemplate * Cleanup * Make init functions protected in template classes * Avoid using static constants and methods in BlockTemplatesRegistry * Avoid using static constants and methods in block template classes * Fix lint errors * Fix Single Product classes not being applied * Fix tests * Get BlockTemplatesRegistry directly from BlockTemplateUtils to simplify code * Init BlockTemplatesRegistry and BlockTemplatesController from Bootstrap.php * Fix wrong static::SLUG call * Init template classes from BlockTemplatesRegistry * Revert "Fix wrong static::SLUG call" This reverts commit 866c9b9.
Changes proposed in this Pull Request:
Part of #42251.
Closes #44264.
Closes #42191.
Closes #44453. (Update: the testing steps for this issue have been added to the PR description after merge, that's because we didn't notice this PR was fixing #44453 until after merge)
This PR refactors
BlockTemplatesController.phpwith the goal to extract all the logic which is specific to certain templates to their own files.Based on that, every template and template part now has its own class, where the slug, title, description and fallback template are define, as well as any custom logic needed for the template to work. Templates inherit from the
AbstractTemplateclass, which takes care of registering the template into theBlockTemplatesRegistry, that's a class that keeps an up-to-date list of registered templates, to make it easy to retrieve data from them (ie: titles, descriptions, etc.).Next steps:
template_redirectaction. I hope this can be simplified in the future, but given that it involves code from WooCommerce core, it probably makes sense to handle it in Investigate moving code related to block templates in class-wc-template-loader.php to BockTemplatesController.php #44540.template_is_eligible_for_product_archive_fallback(), it might make sense to make it more generic so it allows any template to fall back to any other one. For now, I left it as-is to avoid increasing the complexity of this PR.How to test the changes in this Pull Request:
Make sure there are no regressions related to block templates
Test it as you wish and feel free to go outside of what's described below. In any case, some ideas of things to test:
With a classic theme (ie: Storefront).
With a block theme without custom WooCommerce templates (ie: Twenty Twenty-Four)
With a block theme with custom WooCommerce templates (ie: Tsubaki)
Make sure Mini-Cart template part from the theme is correctly assigned to the Mini-Cart template part area (#44264)
/wp-admin/site-editor.php?path=%2Fpatterns&categoryType=wp_template_part)Verify correct template is rendered with certain settings (#44453)
/shop/?product_cat=Clothing&filter_size=large. Note: the values ofproduct_catandfilter_xyzmight vary depending on the categories and attributes of your store. In case of doubt, you can import the products fromsample-data.Changelog entry
Significance
Type
Message
Updated the template logic used by block themes, to make it more performant and resilient
Comment