Refactor to support HPOS

[maxfrederiksen-mondido]

Function

(WIP) Refactor and make our plugin use latest HPOS so it still works in the future when they remove old way.

Business value

Making sure the plugins doesn't break when they deprecate current methods.

Security considerations

Not sure.

Implementation strategies

Not sure.

Rollback strategies

Rollback to latest version

Test

Tested locally connected to prod mondido admin.
Tested happy flow with cards.
Made sure syntax is correct and not breaking.

Steps taken

None yet.

Expected result

Working with new HPOS.

[nuttmeister]

@maxfrederiksen-mondido is this ready for review or still WIP?

[maxfrederiksen-mondido]

@nuttmeister I would say WIP since I can't test it yet locally. However if we can find a way to test on staging then yes.

[eric-thelin]

The conditionals can be eliminated by introducing polymorphism. This will have the following benefits:

1. The client code, i.e. the callers, will be less complex
2. All code related to a particular order storage technology (HPOS or CPT) will be in a single location and not spread out across the code base.
3. Less duplicated code

One polymorphic solution we could use is to introduce an abstract OrderStorageTechnology base class and have concrete implementations HPOSOrderStorage and CPTOrderStorage. A client could then call it like so:

OrderStorageTechnology::current()->delete_meta_data($order, '_mondido_invoice_address');
OrderStorageTechnology::current()->update_meta_data($order, '_mondido_invoice_address', $address);
OrderStorageTechnology::current()->save($order);

The CPTOrderStorage::save method would have an empty implementation since saving seems to be implied by each individual mutation. I guess this is one reason for the lower performance of the legacy API.

To reduce the duplication further we could even introduce our own OrderStorage class and use it like so:

OrderStorage::get().replace_meta_data($order, '_mondido_invoice_address', $address);

The implementation would reuse the OrderStorageTechnology:

class OrderStorage
{
	// ... Static factory method, technology field and constructor here

	public function replace_meta_data($order, $key, $value)
	{
		$this->technology->delete_meta_data($order, $key);
		$this->technology->update_meta_data($order, $key, $value);
		$this->technology->save($order);
	}

	// More useful methods would go here
}

With this implementation, the only if-statement is in OrderStorageTechnology::current(), the client code is much simpler and each participating class has a small and well defined scope.

[eric-thelin]

Generally looks good. Please consider my comments.

[eric-thelin]

Why do we remove the conditional here?

[maxfrederiksen-mondido]

This file will be removed after upgrade so no need to add any if checks.

[eric-thelin]

Is upgrading WooCommerce a prerequisite for using HPOS?

[maxfrederiksen-mondido]

Yes.

October 2023
From WooCommerce 8.2, released on October 2023, High-Performance Order Storage (HPOS) is officially flagged as stable and will be enabled by default for new installations.

[maxfrederiksen-mondido]

I have tested everything on the latest version 8.7. So ideally if we get merchants to upgrade to the same one we should be safe. Hopefully the newer versions will also work, aka 9.0, but we should encourage our merchants to use 8.7 for now.

[eric-thelin]

What will happen if the merchant uses our updated plugin without updating their WooCommerce version? Do we really want to enforce all merchants to upgrade their WooCommerce version to use the latest version of our plugin?

[maxfrederiksen-mondido]

The idea is for us to be able to support HPOS and CPT at the same time, which is what this PR is about. Our merchants should till be able to run really old woo and it will work since it's backwards compatible. However, IMHO, i don't think it's good for them to be on old versions, like 2.2 etc. So if our plan is to ask those merchants to upgrade we might as well ask everyone.

Merchants that are using versions 8.2 or later will be able to activate HPOS directly. However they will need to also do some admin work since they need to sync orders between the tables etc, it's an easy process but still should be monitored by them and if any errors should come up they would need to contact us or Woocommerce.

[eric-thelin]

I think this call should move to either OrderStorageTechnology or OrderUtil so that we have all mentions of the custom orders table in a single location. However, if we move the logic we must pass __FILE__ as an argument to preserve the behavior, e.g. OrderStorageTechnology::declare_compatibility(__FILE__);

[maxfrederiksen-mondido]

I feel like it fits here since it's something that should be added before woocommerce init.
A very similar add_action call is made further down in that function:
"// WC_Order Compatibility for WC < 3.0
add_action( 'woocommerce_init', CLASS . '::add_compatibility' );"

Which is also for a one-off compatibility file, so i feel like it fits here.

[eric-thelin]

My idea was just to move the mention of the custom orders table to either OrderStorage or OrderUtil since that is an implementation detail of HPOS. Thinking of it now, an even better location is probably HPOSOrderStorage. The call to add_action is probably best kept here.

add_action( 'before_woocommerce_init', function() {
	HPOSOrderStorage::declare_compatibility(__FILE__);
}

However, if you prefer the current solution I am fine with that.

[laura-mondido]

Please remove the "WIP" tag from title before merging. It'd confuse anyone going through commits in the future.

[jonaholsson]

Good comments by @eric-thelin. I have nothing to add

[eric-thelin]

Having Interface in the name is ugly and violates the IS-A relationship to implementing classes. For instance, HPOSOrderStorage implements OrderStorageInterface but it is wrong to say that it "is a" OrderStorageInterface. HPOSOrderStorage is not an interface, it is a kind of order storage. The same is true for CPTOrderStorage. Since both those classes are examples of order storage, it makes sense to use the name OrderStorage for the shared interface.

If the problem is that we already use the name OrderStorage as a placeholder for the current() method, my suggestion is that we merge OrderStorageInterface and OrderStorage into an abstract class named OrderStorage.

abstract class OrderStorage
{
	public static function current(): OrderStorage
	{
		if (OrderUtil::custom_orders_table_usage_is_enabled()) {
			return new HPOSOrderStorage();
		} else {
			return new CPTOrderStorage();
		}
	}

	public abstract function get_meta_data($order, $meta_key, $meta_value);
	public abstract function add_meta_data($order, $meta_key, $meta_value);
	public abstract function update_meta_data($order, $meta_key, $meta_value);
	public abstract function delete_meta_data($order, $meta_key);
	public abstract function save($order);
}

[maxfrederiksen-mondido]

That's exactly the issue I saw with clashing names. Thanks for the input, learning a lot about OOP best practices here, great stuff :)