Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Blockified Templates: Improve migration logic #10415

Merged
merged 1 commit into from
Aug 2, 2023
Merged

Blockified Templates: Improve migration logic #10415

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/Migration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,14 @@ class Migration {
*/
public function run_migrations() {
$current_db_version = get_option( Options::WC_BLOCK_VERSION, '' );
$schema_version = get_option( 'wc_blocks_db_schema_version', '' );

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@gigitux gigitux Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 "".

Copy link
Contributor

@nefeline nefeline Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@gigitux gigitux Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 👍

// or an update from WC Blocks older than 10.3.0 (it will have some value). In the latter scenario
// we should run the migration.
// We can remove this check in the next months.
if ( ! empty( get_option( 'wc_blocks_db_schema_version', '' ) ) ) {
if ( ! empty( $schema_version ) && ( empty( $current_db_version ) ) ) {
$this->wc_blocks_update_1030_blockified_product_grid_block();
}

Expand Down