Blockified Templates: Improve migration logic #10415
Conversation
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
|
Size Change: 0 B Total Size: 1.33 MB ℹ️ View Unchanged
|
2192086 to
397344a
Compare
|
|
||
| // This check is necessary because the version was not being set in the database until 10.3.0. | ||
| // Checking wc_blocks_db_schema_version determines if it's a fresh install (value will be empty) |
There was a problem hiding this comment.
I would suggest exploring an alternative route here, as the assumption that wc_blocks_db_schema_version is empty on fresh installs is a bit fragile. This option is updated whenever maybe_create_tables is triggered and this method is hooked to 'admin_init', which is fired whenever an admin screen or script is being initialized, so fresh installs will have the default 0 value updated to 260 whenever the table creation is complete.
There was a problem hiding this comment.
I agree that checking wc_blocks_db_schema_version isn't the best approach, but unfortunately, it seems that it is the only way to check if it is a new installation.
This code will be executed before that admin_init is triggered. Empty returns true if you pass 0 or "".
There was a problem hiding this comment.
This code will be executed before that admin_init is triggered. Empty returns true if you pass 0 or "".
This option is not deleted from the DB when the plugin is removed, so if someone at some point in the past had Woo and/or Woo blocks up and running, removed them, and now enabled them again, $this->wc_blocks_update_1030_blockified_product_grid_block(); will be triggered and cause the same problem.
I agree that checking wc_blocks_db_schema_version isn't the best approach, but unfortunately, it seems that it is the only way to check if it is a new installation.
Just to clarify, is the goal here to run the migration to all users that already had Woo installed (keeping the classic template) while at the same time displaying the blockified template for brand-new users?
There was a problem hiding this comment.
This code will be executed before that admin_init is triggered. Empty returns true if you pass 0 or "".
This option is not deleted from the DB when the plugin is removed, so if someone at some point in the past had Woo and/or Woo blocks up and running, removed them, and now enabled them again,
$this->wc_blocks_update_1030_blockified_product_grid_block();will be triggered and cause the same problem.
I think that it is fine since it is an uncommon use case. The user has an old version of WC Core. Remove it. Install the new one. The user will have the classic templates.
I agree that checking wc_blocks_db_schema_version isn't the best approach, but unfortunately, it seems that it is the only way to check if it is a new installation.
Just to clarify, is the goal here to run the migration to all users that already had Woo installed while at the same time displaying the blockified template for brand-new users?
Yes.
There was a problem hiding this comment.
Yes.
Thanks for confirming. I think that one way to reliably ensure a store is new when WooCommerce Blocks is enabled, is by looking for the existence of products or orders: a store that is brand new won't have at least one of those. Here's a loose example:
$show_default_blockified_template = get_option( 'example_should_display_blockified_template', false );
if ( $show_default_blockified_template ) {
return;
} else {
$store_has_products = wc_get_products( array( 'limit' => 1 ) );
$store_has_orders = wc_get_orders( array( 'limit' => 1 ) );
if ( ! $store_has_products || ! $store_has_orders ) {
update_option( 'example_should_display_blockified_template', true );
} else {
return;
}
}This way we have something more robust to rely on instead of the value of a DB migration to determine if the blockified template should be displayed or not. Also, notice that there's no condition to change the option back to false at any point in execution: this ensures that if someone enabled the plugin on a brand new store and after that created products/received orders, they won't have the legacy template accidentally displayed again.
There was a problem hiding this comment.
If you upgrade the version even if you don't have products, it means that the store isn't new:
This graphic is just a representation of the current logic & unfortunately, doesn't answer my question: nothing in it explains why the plugin version is being used to determine if a store is new or not. And to reinforce my comment over here: #10415 (comment) there's no proposal to change what is represented in this image.
There was a problem hiding this comment.
We started saving the version of WC Blocks in the database only with WooCommerce Blocks 10.3.0 release.
So, if this condition below is true, it means that it isn't a new installation because:
- $schema_version is already set
- $current_db_version is empty, so it means that the user is upgrading from a version of WC Blocks < 10.3.0
! empty( $schema_version ) && ( empty( $current_db_version )
There was a problem hiding this comment.
nothing in it explains why the plugin version is being used to determine if a store is new or not.
I think "fresh WooCommerce installation" tries to answer it. It may be the insufficient choice of words, but this diagram was supposed to represent this table from this PR:
which clearly differentiates "New installation" from "Upgrade" and that's what the first condition in the diagram was supposed to represent ("Fresh installation" meaning "New installation" and not "Upgrade"). And we'd like to preserve that logic. Does that help answer the concern?
There was a problem hiding this comment.
And we'd like to preserve that logic. Does that help answer the concern?
Thanks @kmanijak , it does represent what the current implementation is, but unfortunately, I couldn't find any explanation why this decision was made on that PR =/.
Either way, we still have all the aforementioned problems to clear out, as the solution provided here does not apply to all use-case scenarios, with very important limitations as flagged on #10415 (comment) , #10415 (comment) and #10415 (comment) .
Since the decision was to proceed with this work for the patch release, I'll propose circling back to solve these problems in our next cycle.
There was a problem hiding this comment.
but unfortunately, I couldn't find any explanation why this decision was made on that PR =/.
Unfortunately, I cannot provide more context, as this PR was made way before I joined the project. It's possible the PR only represents the outcome of other discussions but I can't justify that as I wasn't there 😅. What I was trying to say is that the table represents the logic present in the project for months now and this PR aims to fix a specific scenario preserving those rules.
Since the decision was to proceed with this work for the patch release, I'll propose circling back to solve these problems in our next cycle.
Agree! 👍
There was a problem hiding this comment.
I don't feel comfortable with approving this PR in its current shape and form, given all the technical limitations this solution has. See more details about the problems and the proposed alternative solution at #10415 (comment).
I'm glad to circle back for another pass if changes are made. Otherwise, I withdraw my review.
Thanks for the review! I'm ready to update the logic, but before proceeding, it would be great to have feedback from @tjcafferkey, who checked the migration strategy code for another issue (#10196). I don't agree that we should change the migration strategy logic for several reasons:
|
Just to make it super clear, what is being questioned on #10415 (comment) is the changes made on this PR, not the current implementation.
The proposal is not to change the pre-existing migration logic: the proposal is to preserve it exactly the same, but with a more robust implementation that isn't sensible to plugin activation/deactivation and/or removal.
This part is still very obscure to me as the version of the plugin shouldn't be part of the equation to determine if a store is new or not. |
|
There is a misunderstanding about the definition of "new store". For the current logic, "new store" means a "fresh installation of WooCommerce", so for this reason, the version of the plugin is essential. For your logic, "new stores" means a "store without any products/orders".
Not really. With the current logic, if a user upgrades a store (without any products/orders) from WC 7.8 to WC 8.0 will have the classic templates. Instead, with your logic, the templates will be blockified. It is something that I would not do since the migration strategy has already been communicated. cc @kmanijak Please, correct me if I'm wrong! |
@gigitux can you clear out the following, please? First, you mentioned there was an exception for updating from version 7.8 to 7.9, but this PR doesn't touch any of those versions (this is planned for WC 8.0); after that you affirmed that this limitation also applies to upgrading from 7.8 to 8.0. This question I did earlier is still pending:
I would appreciate a concrete and technical explanation here. Where are these decisions being made? Is there any convo I missed where it was clearly discussed that we should port over this decision to WooCommerce version 8 that I'm unaware of? I would also appreciate any link of discussion when this decision of preserving the classic template for new stores when upgrading from version 7.8 to version 7.9 was made. |
This PR keeps the logic as it is, but if it fixes a bug when the user updates from 7.9 -> 8.0 or the WC Blocks is enabled, the user disables it and enables it again.
As I said above, I don't see any real problem issue. Also, I agreed with your idea, but we didn't think about it when we worked on this migration strategy. You are proposing the update the logic. The downsides that I see are explained in this comment.
This is not true, it doesn't preserve the same behavior as I wrote here.
I don't see a huge benefit of changing the current logic. It could create confusion, especially for 3rd party developers.
Sure: #6538 |
There was a problem hiding this comment.
I appreciate the passion around this proposed change and its impact, and I also acknowledge that there are some circumstances that may require further thought and consideration. There appears to be a misalignment between how some of us distinguish between a fresh install vs upgrade vs unused (no orders, no products) store.
Given that this migration has run for all stores upgrading from 7.8 > 7.9 already and that this check will be removed in the near future, furthermore the time sensitive nature of this since its currently blocking two releases I am going to approve the PR and we can discuss this further in a more appropriate setting away from the pressures of a release.
Again, I appreciate and value of all the differing opinions and I’m glad we work in a team where we can express them between each other.
Thanks: I already shared my response to these concerns over here #10415 (comment) . Also, since the decision was to merge this PR as-is for the patch release, we now have a few more limitations to circumvent. As flagged over here #10415 (comment) , I'll propose working on solving these problems in our next cycle.
It does for the vast majority of cases, but this PR is not solving the problem for all users either.
I see many limitations and flaws with the current implementation on this PR, not the logic for the migrations. IMO we can maintain the current logic unchanged, the critical part is getting rid of all problems introduced here, as flagged earlier on #10415 (comment) .
Thanks! |
There was a problem hiding this comment.
Re-tested scenarios:
(When mentioning WC Core 8.0 that means build including changes from this PR).
| Scenario | Outcome |
|---|---|
| Updating WC Core 7.8 -> 7.9 -> 8.0 | Classic templates ✅ |
| Updating WC Core 7.8 -> 8.0 | Classic templates ✅ |
| Updating WC Core 7.9 -> 8.0 | Blockified templates ✅ |
| Updating WC Core 7.8 -> 7.9 (blockifying template manually) -> 8.0 | Blockified templates ✅ |
| WC Core 7.9 and activate WC Blocks including changes from this PR | Blockified templates ✅ |
| Deactivate WC Blocks from the previous scenario | Revert to Classic templates. We understand why that happens, the explanation below. |
In the last scenario when we deactivate the newer version of WC Blocks, this condition is true - versions of WC Blocks are different:
if ( $this->package->get_version() !== $this->package->get_version_stored_on_db() ) {
and the migration is run with THE OLD logic (the one in trunk) which causes the classic templates to be loaded.
Along with WC Core 8.0, we may consider a patch update to WC Core 7.9 as well to fix that problem.
|
I re-tested the PR and I can confirm the @kmanijak results. Also, I tested another two scenarios:
|
|
Since that @tjcafferkey and @kmanijak tested and approved the PR, I will merge this PR. Thanks for the discussion and feedback! |
|
Issue created to address the aforementioned technical limitations: woocommerce/woocommerce#42268 |
* improve migration to blockified templates (#10415) * Add to Cart Form: Fix broken styles for the block when using inside the Single Product Block (#10282) * Fix broken styles for Add to Cart Form block * Fix PHP CS error * add testing instructions * Empty commit for release pull request * update changelog * update version * add zip link * improve testing instructions --------- Co-authored-by: Luigi Teschio <gigitux@gmail.com> Co-authored-by: Alexandre Lara <allexandrelara@gmail.com> Co-authored-by: github-actions <github-actions@github.com>
* Add block foundation * Add block styles * Add Dots Pager * Move icons to the block folder * Add block settings * Add Pager to Product Gallery template * Add setting to change Pager display mode * Change the block description * Fix the block icon color when selected * Fix php cs errors * Fix php cs errors * Fix css lint errors * Fix eslint error * Move enum to its own file * Remove unnused call to request context * Add block template * Fix php cs errors * fix php cs errors * improve docs * Remove duplicate HTML element and added classnames for single product block (#10374) * Show only products with rating (#10434) * Add Product Gallery Thumbnails block (#10442) * WIP Product Gallery: Add the Thumbnails block * Product Gallery Thumbnails: Add block settings * Add template for the Product Gallery block * Add template for the Product Gallery block. Add the rest of the files. * Product Gallery Thumbnails: Add context settings sharing between the Product Gallery and Thumbnails block. * Product Gallery Thumbnails: Add UI functionality and frontend functionality. Add settings for the Thumbnails in both places - Product Gallery and the Thumbnails block. * Product Gallery Thumbnails: Move the static template ouside of the component * Make sure the context is set before accesing the array values * Product Gallery Thumbnails: Move the setGroupAttributes() function outside of the component * Product Gallery Thumbnails: Fix TS errors * Product Gallery Thumbnails: Update the Features Flags and Experimental Interfaces doc * Product Gallery Thumbnails: Fix TS error * Product Gallery Thumbnails: Remove unused stylesheet * Product Gallery Thumbnails: Fix TS errors * Product Gallery Thumbnails: Remove unused context and fix the thumbnails bottom position styling on the frontend. * Product Gallery Thumbnails: Allow the user to move the horizontal thumbnails above the large image and don't overwrite that automatically * Product Gallery Thumbnails: Add code comments and remove the incorrect conditional check when moving thumbnails up and down * Product Gallery Thumbnails: Fix the eslint dependency error * Product Gallery Thumbnails: Refactor Product Gallery edit code and move the logic to a utils file * Product Gallery Thumbnails: Update the utils file * Product Gallery Thumbnails: Update the utils file. Fix comment indentation * Product Gallery Thumbnails: Fix undefined variable html when only 1 product image is set * Product Gallery: Rename clientId to productGalleryClientId * Product Gallery Thumbnails: Combine the useEffect code having the same dependencies * Product Gallery Thumbnails: Combine all useEffect code together * Product Gallery Thumbnails: Add a ThumbnailsPosition enum * Product Gallery Thumbnails: Update the thumbnailsPosition to an enum * Product Gallery Thumbnails: Fix TS errors * Product Gallery Thumbnails: Fix TS errors * Product Gallery Thumbnails: Add missing dependency * Product Gallery Thumbnails: Uppercase the enum and fix the thumbnails position bug when initially adding the Product Gallery block * Product Gallery: Add crop, zoom and full-screen settings * Product Gallery Thumbnails: Replace ts-ignore with ts-expect-error * Product Gallery Thumbnails: Replace ts-ignore with ts-expect-error * Product Gallery Thumbnails: Revert back to ts-ignore * Revert "Product Gallery: Add crop, zoom and full-screen settings" This reverts commit 7de1f0f. * Remove propTypes (#10432) * Fix badge wrong spacing on the newest arrivals pattern (#10446) * Product Gallery: Add Crop, Zoom and Full-screen settings (#10445) * WIP Product Gallery: Add the Thumbnails block * Product Gallery Thumbnails: Add block settings * Add template for the Product Gallery block * Add template for the Product Gallery block. Add the rest of the files. * Product Gallery Thumbnails: Add context settings sharing between the Product Gallery and Thumbnails block. * Product Gallery Thumbnails: Add UI functionality and frontend functionality. Add settings for the Thumbnails in both places - Product Gallery and the Thumbnails block. * Product Gallery Thumbnails: Move the static template ouside of the component * Make sure the context is set before accesing the array values * Product Gallery Thumbnails: Move the setGroupAttributes() function outside of the component * Product Gallery Thumbnails: Fix TS errors * Product Gallery Thumbnails: Update the Features Flags and Experimental Interfaces doc * Product Gallery Thumbnails: Fix TS error * Product Gallery Thumbnails: Remove unused stylesheet * Product Gallery Thumbnails: Fix TS errors * Product Gallery Thumbnails: Remove unused context and fix the thumbnails bottom position styling on the frontend. * Product Gallery Thumbnails: Allow the user to move the horizontal thumbnails above the large image and don't overwrite that automatically * Product Gallery Thumbnails: Add code comments and remove the incorrect conditional check when moving thumbnails up and down * Product Gallery Thumbnails: Fix the eslint dependency error * Product Gallery Thumbnails: Refactor Product Gallery edit code and move the logic to a utils file * Product Gallery Thumbnails: Update the utils file * Product Gallery Thumbnails: Update the utils file. Fix comment indentation * Product Gallery Thumbnails: Fix undefined variable html when only 1 product image is set * Product Gallery: Rename clientId to productGalleryClientId * Product Gallery Thumbnails: Combine the useEffect code having the same dependencies * Product Gallery Thumbnails: Combine all useEffect code together * Product Gallery Thumbnails: Add a ThumbnailsPosition enum * Product Gallery Thumbnails: Update the thumbnailsPosition to an enum * Product Gallery Thumbnails: Fix TS errors * Product Gallery Thumbnails: Fix TS errors * Product Gallery Thumbnails: Add missing dependency * Product Gallery Thumbnails: Uppercase the enum and fix the thumbnails position bug when initially adding the Product Gallery block * Product Gallery: Add crop, zoom and full-screen settings * Product Gallery Thumbnails: Replace ts-ignore with ts-expect-error * Product Gallery Thumbnails: Replace ts-ignore with ts-expect-error * Product Gallery Thumbnails: Revert back to ts-ignore * Revert "Product Gallery: Add crop, zoom and full-screen settings" This reverts commit 7de1f0f. * Product Gallery: Add crop, zoom and full-screen settings * Product Gallery: Remove the redundant React Fragment * Remove nested filled and empty cart blocks in cart template (#10447) * improve migration to blockified templates (#10415) * fix compatibility with WP 6.2 (#10449) * Add Product Gallery Pager to template * Add Pager settings to Product Gallery block * Remove save function and rename icon * Use nullish coalescing operator for the block context --------- Co-authored-by: Roy Ho <roykho77@gmail.com> Co-authored-by: Alba Rincón <albarin@users.noreply.github.com> Co-authored-by: Daniel Dudzic <daniel.dudzic@automattic.com> Co-authored-by: Thomas Roberts <5656702+opr@users.noreply.github.com> Co-authored-by: Luigi Teschio <gigitux@gmail.com>
@kmanijak and I worked on this PR.
This PR refactors the migration logic to fix #10413. We improved to check to ensure that the migration is executed ONLY during the update from a WooCommerce version < 7.9.
Fixes #10413
Testing
Automated Tests
Zip files:
User Facing Testing
Upgrade from WooCommerce Core 7.8 to WooCommerce 7.9 to WooCommerce 8.0
Upgrade from WooCommerce Core 7.8 to WooCommerce 8.0
Upgrade from WooCommerce Core 7.9 to WooCommerce 8.0
WooCommerce Visibility
Changelog