Check block templates and parts directories to the Gutenberg 12.1.0 convention #5455
Conversation
rubikuserbot
requested review from
a team and
tjcafferkey
and removed request for
a team
sunyatasattva
added
focus: FSE
|
Size Change: 0 B Total Size: 819 kB ℹ️ View Unchanged
|
There was a problem hiding this comment.
I have a few thoughts on this:
- We shouldn't be importing the
BlockTemplatesControllerinto theBlockTemplatesUtils. - The consts defined at the top of the
BlockTemplatesControllerare for the Woo Blocks templates/parts, not the themes. If we are going to update the Woo Block template locations (Update the path of WooCommerce templates to match the new Gutenberg folders #5343) we should do this in a separate PR - All we need to do is to add
templatesandpartsto the checks we do when looking for themes template files whilst leaving theblock-templatesandblock-template-partsdirectories already in place
I feel like we are doing too much in this PR. Let's limit the scope to just consider the new theme directories. We can move our templates in a separate PR. Let me know what you think.
Edit: The reason for me suggesting we should be limiting scope in this PR is because this is quite an important issue we need to solve. Limiting scope will make it easier to test it works, avoid any negative side effects and easily trace back to this PR if there are. Given that we don't have much test coverage for this feature I am just trying to be a bit more cautious.
|
Understood, I'll separate the two things! |
Will open a different PR to address #5343
Removing as it's part of the Woo Blocks-specific migration
|
Alright, as requested, I have split the original changes from this PR within here and #5464. I've also updated the OP to reflect just these changes in the changelog, references and testing notes. @Aljullu I took the liberty to tag you as a reviewer as @tjcafferkey is AFK for the next week and you have context on what's this about since you had originally opened #5343. |
src/Utils/BlockTemplateUtils.php
Outdated
| * | ||
| * @var string | ||
| */ | ||
| const TEMPLATE_PARTS_DIR_NAME = 'parts'; |
There was a problem hiding this comment.
Can we not put these values in an associative array?
There was a problem hiding this comment.
We 100% can. I was just following the pre-existing convention. Also, in the spirit of trying to avoid as many side-effects as possible, not changing that convention makes the PR slightly safer.
But I can convert those values in an associative array, if you so prefer.
There was a problem hiding this comment.
I think it might make sense to put these in an associative array, I don't think it'll be too risky given the convention hasn't been set in this file.
|
If I put |
Oh! Thanks for spotting it! I see why: I moved everything I had changed in the Controller to the other PR, to separate the changes as requested. But they are somewhat tightly coupled so, this change was moved as well, going to revert that at once. |
|
@tjcafferkey I've addressed the issues:
Did not change the |
Thanks!
The themes |
It took me a long time, but I found the culprit. Turns out that this is a WooCommerce problem, as they don't support this convention yet. I have opened a bug on their repo here: woocommerce/woocommerce#31518 (also a Slack thread) I could also open a PR to fix that. In the meantime, I added a fix on our side by hooking into the |
|
Nice catch @sunyatasattva!
I think we should revert the temporary fix on our side which you just added, and open a PR to amend the following code directly in the WC Core codebase. Then in the testing instructions for this PR just detail that the tester should have the other WC PR checked out locally too, to test this. Feel free to add me as a reviewer too. As a side note we should also add to the testing instructions anyway for the reviewer to check the frontend templating is working as expected. Let me know what you think. |
…filter" This reverts commit 855ae2e.
|
Hey @tjcafferkey ! First of all, thanks a lot for keeping an eye on this despite being AFK. To the subject matter: I did submit a PR to Woo Core with the permission of @vedanshujain on Slack. I also did remove the temporary patch and also added the caveat in the testing note on the OP. However, I have a few remarks I'd like to bounce to you:
I'm mostly worried about (1.), I don't think it's much harm to leave the patch in as long as we remember to remove it when everything is aligned and stable. As for now, I have reverted as requested. Let me know your thoughts. |
No problem, thank you for doing such a thorough job on this issue I really appreciate it!
It depends on the issue I think. In this case we don't need the temp fix for themes using If this was a time sensitive issue that was needed for an imminent release I would totally be onboard with the fix, however I don't think that it's the case here (unless I am wrong?). Also by putting in a temp fix this means we need to review and test:
This I think would be unnecessary unless it was absolutely needed.
Could you please elaborate on this I am not sure what you mean? Sorry. Let me know your thoughts. |
There was a problem hiding this comment.
Great job @sunyatasattva thanks for addressing all the feedback. I've tested this and it seems to be working as expected. Thank you for fixing.
|
@tjcafferkey Alright, everything you said sounds good to me. I thought it was a bit more time sensitive because there already are themes (like Tove) which use the new convention. Just that.
I see that in that piece of code I linked, we are basically forcing Woo to know that, in specific circumstances, there are indeed block templates. So I thought that perhaps that logic should be made available to Woo Core itself, if possible. Not sure if that made any sense. In any case, nothing that should take more of your AFK attention :) Thanks again for sticking with it and approving! |
| $carry[] = get_template_directory() . $filepath; | ||
| $carry[] = get_stylesheet_directory() . $filepath; |
There was a problem hiding this comment.
Sorry for being late on this one, but if I'm not wrong this is searching templates in this order:
- Parent theme
templatesfolder. - Child theme
templatesfolder. - Parent theme
block-templatesfolder. - Child theme
block-templatesfolder.
But I think the child theme should always have priority. Something like this:
- Child theme
templatesfolder. - Child theme
block-templatesfolder. - Parent theme
templatesfolder. - Parent theme
block-templatesfolder.
What do you think @sunyatasattva?
There was a problem hiding this comment.
@Aljullu Indeed this is correct! I'll open a PR to amend this! Thanks for spotting that.
Yeah you are correct that is how that code works, it gives us the ability to override Core and load templates from the plugin. I'm not so sure how this would fit in Core currently? Core checks the active theme for block templates, this is our way of loading them from our plugin Woo Blocks (which Core would have no concept of, although we do include the Woo Blocks package in Core every month). Maybe I'm missing something or misunderstanding but I'm open to discussing/seeing if what you mean could work 🙂 |
|
Ok I see now! I think you got my meaning, it was I who did not fully understand the relationship between Core and Blocks and themes and plugins. Thanks for taking the time to clarify. |
tjcafferkey
changed the title
ralucaStan
added
skip-changelog
…5464) * Align Woo Block template locations with the newest convention While we now support both the old and new conventions for the templates paths, our own repo should be aligned with the latest convention. See: #5455 Fixes: #5343 * Simplify `generate_template_slug_from_path` function * Change `BlockTemplatesController` constructor to get correct dir names * Update Mini Cart template path
With WordPress/gutenberg#36647, Gutenberg 12.1.0 has changed the convention for the directory paths from
block-templatesandblock-template-partstotemplatesandpartsrespectively.We are also moving to that convention but making sure that we remain backwards-compatible.
Fixes #5450
Testing
Manual Testing
How to test the changes in this Pull Request:
block-templates) still shows the templates correctly.templatesandpartsrespectively, or download a theme which uses this new convention (e.g. Tove), and make sure the templates still show correctly.User Facing Testing
These are steps for user testing (where "user" is someone interacting with this change that is not editing any code).
Changelog