This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Allow block templates to be customised and saved #5062
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
opr
added
focus: FSE
opr
requested review from
a team and
tjcafferkey
and removed request for
a team
This is necessary because the gutenberg helper functions sometimes turn it into a WP_Block_Template object, and other times it's an array. Because of this it's safer to normalise them both as objects.
When a template is saved it gets saved to the database, we need to handle processing these WooCommerce templates that have been saved in the db and we need to use the gutenberg utils that are private, this is why they've been copied over.
This is because the templates we're dealing with here should always belong to WooCommerce, not the currently selected theme
These are needed to get the template from either the DB or the filesystem when saving/retrieving the template.
This will ensure the correct slug is used in the gutenberg editor.
opr
force-pushed
the
fix/saving-modified-template
branch
from
58f2dab to
3cc5e67
Compare
I have just a bit of feedback that you may have already considered/researched. It might be good to check with the GB team regarding the private functions you referenced (and are duplicating logic for) to see:
The above doesn't necessarily need to block the work here given the tight timelines we have, but it would be good to followup with so we don't end up diverging too much from what may be a more ideal solution in utilizing an API built into WP. |
|
Size Change: -4 kB (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
|
Hey @nerrad this is something I checked with GB team as we already considered this. They informed us that these were not intended to used publicly outside of Gutenberg for the time being as the APIs for these functions may change at any time unannounced. In the interest of time we have chosen to duplicate these utility functions instead as you've pointed out. That being said, I plan on writing a P2 post in the aftermath of this project to demonstrate how we have used these duplicate functions and how/why they were useful. Hopefully from this conversation we'll be able to understand what needs to happen to make this available for public usage, if at all possible. |
|
The below error happened due to the following: We need to change references such as Seemed to be going OK (refreshing the Site Editor persisted changes) and then I got to the following step.
The template was present, but when I clicked "Edit" on this template I got the following fatal error:
After this error, when I tried to navigate back to the Site Editor via the menu item, I got stuck on the following loading state:
|
tjcafferkey
reviewed
Nov 4, 2021
tjcafferkey
reviewed
Nov 4, 2021
tjcafferkey
reviewed
Nov 4, 2021
tjcafferkey
reviewed
Nov 4, 2021
tjcafferkey
reviewed
Nov 4, 2021
tjcafferkey
reviewed
Nov 4, 2021
tjcafferkey
approved these changes
Nov 4, 2021
frontdevde
suggested changes
Nov 5, 2021
There was a problem hiding this comment.
While testing as per testing instructions works well and as expected, I'm seeing issues when testing with additional modified templates.
Could you please test the following:
- Add an
archive-product.html
<!-- wp:template-part {"slug":"header"} /-->
<!-- wp:paragraph --> <p>archive-product</p> <!-- /wp:paragraph -->
<!-- wp:woocommerce/legacy-template {"template":"archive-product"} /-->
<!-- wp:template-part {"slug":"footer"} /-->
- Modify the archive product template in the editor & save
- Load a single product page on the Frontend
The expected behavior would be that the single product page still loads the single product template. What I'm seeing in my testing though is that it starts loading the archive product template on the single product page.
Can you confirm this behavior?
This does not happen for me, it works as expected here. |
frontdevde
previously approved these changes
Nov 5, 2021
frontdevde
dismissed
their stale review
Still testing, just wanted to dismiss the previous blocking review as the issue reported has been addressed.
Aljullu
reviewed
Nov 5, 2021
There was a problem hiding this comment.
Good job with this PR @opr! I really like how you solved several of the challenges from this PR.
I left some comments below, most of them are not blocking, but if you feel they make sense, we should either fix them or create an issue to track them.
src/BlockTemplatesController.php
Outdated
Comment on lines
294
to
299
| ) || | ||
| array_filter( | ||
| $this->get_block_templates(), | ||
| function ( $template ) use ( $template_name ) { | ||
| return $template->slug === $template_name; | ||
| } |
There was a problem hiding this comment.
If we only care about templates saved in the database, would this be equivalent?
Suggested change
| ) || | |
| array_filter( | |
| $this->get_block_templates(), | |
| function ( $template ) use ( $template_name ) { | |
| return $template->slug === $template_name; | |
| } | |
| ) || gutenberg_get_block_template( 'woocommerce//' . $template_name ); |
Not blocking, but I also wonder if this fix should be done directly in WC core. I think that was an overlook from my part. But the WC core function shouldn't ignore templates saved in the database.
There was a problem hiding this comment.
@Aljullu you are right, WC Core seems to only look at templates on the filesystem. I can address this later. This has changed during the refactor, do you think we should go straight to the Gutenberg function, or do you think it's OK to use our get_block_templates with the $slugs arg?
Aljullu
approved these changes
Nov 5, 2021
src/BlockTemplatesController.php
Outdated
| * in the theme directory. | ||
| * | ||
| * @param string[] $slugs An array of slugs to filter templates by. Templates whose slug does not match will not be returned. | ||
| * @param array $already_found_templates Templates that have already been found, these will customised templates that are loaded from the database. |
There was a problem hiding this comment.
Nit: There seems to be a typo in the comment these will customised.
src/BlockTemplatesController.php
Outdated
| return $query_result; | ||
| } | ||
|
|
||
| /** | ||
| * Get and build the block template objects from the block template files. | ||
| * Removes templates that were added to a theme's block-templates director, but already had a customised version saved in the database. |
There was a problem hiding this comment.
Suggested change
| * Removes templates that were added to a theme's block-templates director, but already had a customised version saved in the database. | |
| * Removes templates that were added to a theme's block-templates directory, but already had a customised version saved in the database. |
src/BlockTemplatesController.php
Outdated
|
|
||
| /** | ||
| * This function checks if there's a blocks template (ultimately it resolves either a saved blocks template from the | ||
| * database or a template file in `woo-gutenberg-products/block/templates/block-templates/`) |
There was a problem hiding this comment.
Suggested change
| * database or a template file in `woo-gutenberg-products/block/templates/block-templates/`) | |
| * database or a template file in `woo-gutenberg-products-block/templates/block-templates/`) |
This was referenced Nov 8, 2021
jonny-bull
pushed a commit
to jonny-bull/woocommerce-gutenberg-products-block
that referenced
this pull request
Dec 14, 2021
* Ensure $template is object before accessing properties This is necessary because the gutenberg helper functions sometimes turn it into a WP_Block_Template object, and other times it's an array. Because of this it's safer to normalise them both as objects. * Add Gutenberg utils for processing templates based on a post from the db When a template is saved it gets saved to the database, we need to handle processing these WooCommerce templates that have been saved in the db and we need to use the gutenberg utils that are private, this is why they've been copied over. * Force theme to always be WooCommerce This is because the templates we're dealing with here should always belong to WooCommerce, not the currently selected theme * Add maybe_return_blocks_template and get_single_block_template funcs These are needed to get the template from either the DB or the filesystem when saving/retrieving the template. * Set theme to always be woocommerce when making templates from files This will ensure the correct slug is used in the gutenberg editor. * Check if template has been customised and saved in the database first * Prevent filesystem templates being used if a custom database one exists * Fix syntax error from rebase * Remove unnecessary code from BlockTemplateUtils * Ensure template item is an object containing correct properties * Prevent warnings from appearing * Ensure title is added to the template when saving * Filter templates that don't match the queried slug. * Remove unused code * Check if a saved version of the template exists when trying to render * Rename default_block_template_is_available to block_template_is_available * Re-hook pre_get_block_template before returning from maybe_return_blocks_template * Make comment easier to read * Look for template in woocommerce theme or real theme taxonomy * Remove duplicated title assignment * Prevent template being added twice when loading from the db * Filter templates before returning if slugs are supplied * Simplify `get_block_templates` function into two functions * Add function to stop theme templates that are added after db ones showing * Fix typographical errors
jonny-bull
pushed a commit
to jonny-bull/woocommerce-gutenberg-products-block
that referenced
this pull request
Dec 16, 2021
* Ensure $template is object before accessing properties This is necessary because the gutenberg helper functions sometimes turn it into a WP_Block_Template object, and other times it's an array. Because of this it's safer to normalise them both as objects. * Add Gutenberg utils for processing templates based on a post from the db When a template is saved it gets saved to the database, we need to handle processing these WooCommerce templates that have been saved in the db and we need to use the gutenberg utils that are private, this is why they've been copied over. * Force theme to always be WooCommerce This is because the templates we're dealing with here should always belong to WooCommerce, not the currently selected theme * Add maybe_return_blocks_template and get_single_block_template funcs These are needed to get the template from either the DB or the filesystem when saving/retrieving the template. * Set theme to always be woocommerce when making templates from files This will ensure the correct slug is used in the gutenberg editor. * Check if template has been customised and saved in the database first * Prevent filesystem templates being used if a custom database one exists * Fix syntax error from rebase * Remove unnecessary code from BlockTemplateUtils * Ensure template item is an object containing correct properties * Prevent warnings from appearing * Ensure title is added to the template when saving * Filter templates that don't match the queried slug. * Remove unused code * Check if a saved version of the template exists when trying to render * Rename default_block_template_is_available to block_template_is_available * Re-hook pre_get_block_template before returning from maybe_return_blocks_template * Make comment easier to read * Look for template in woocommerce theme or real theme taxonomy * Remove duplicated title assignment * Prevent template being added twice when loading from the db * Filter templates before returning if slugs are supplied * Simplify `get_block_templates` function into two functions * Add function to stop theme templates that are added after db ones showing * Fix typographical errors
1 task
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR focuses on allowing blocks templates to be customised, saved, and retrieved without errors.
The changes made are:
BlockTemplateUtilsclass.gutenberg_add_template_infogutenberg_build_template_result_from_post$template's properties are accessed, normalise$templateto be an object. This is necessary because this is sometimes an array or sometimes aWP_Block_Template.woocommerce. This is necessary as the current behaviour is to save the template under the current theme's slug, which seems incorrect.maybe_return_blocks_templatefunction - this is used to short-circuit thegutenberg_get_block_templatesmethod from Gutenberg. It checks the db to see if there's a saved template already. If there is then we can return that.get_single_block_templatefunction. Until now there was no way to get a single woo-blocks block template in our codebase. This function is used withingutenberg_get_block_templatesto override/augment the data returned by thegutenberg_get_block_templatesfunction. This is how we inject our templates into the list of templates gutenberg finds.get_block_templatesinBlockTemplatesController.phpto check the database first to see if any saved templates from thewoocommercetheme exist. If they do, build a template from the saved posts.add_block_templatesinBlockTemplatesController.phpto only add the templates from files that don't have a customised version in the database.get_block_templatesfilter containsslug__inthen filter the results ofBlockTemplatesController'sget_block_templatesmethod to only included those that match the queried slugs.remove_theme_templates_with_custom_alternative- this will cater for the situation described in Second testing steps for alternative scenario (Theme adds a template for an already customised template)Fixes #5024
Other Checks
Screenshots
Testing
Automated Tests
Manual Testing
How to test the changes in this Pull Request:
single-product.htmltowoocommerce-gutenberg-products-block/templates/block-templates/- insert the following content:single-product.htmlfileAppearance > Templatesand verify the template is saved with a theme ofwoocommerce.single-product.htmlwith the above contents towp-content/themes/<your-theme>/block-templates/and repeat steps 3-6.Second testing steps for alternative scenario (Theme adds a template for an already customised template)
single-product.htmlfile above towoo-blocks/templates/block-templates. Ensure there is not asingle-product.htmlfile in your theme's block-templates directory!single-product.htmlfile to your theme's block-templates directory.single-productis loaded.General templateslist in the editor.User Facing Testing
These are steps for user testing (where "user" is someone interacting with this change that is not editing any code).
The block theme needs to declare
add_theme_support( 'woocommerce' );in thefunctions.phpfile.Appearance > Templates. Verify there's a template calledSingle productwith the themewoocommerce