Old WC versions: try loading WCPay if the account is connected

[htdat]

Context: paJDYF-2Dr-p2#comment-8197

Changes proposed in this Pull Request

Handle the case new WCPay versions do not support current WC core versions:

• Continue loading WCPay if the account is connected. Note: at this stage, no main plugin file is loaded, therefore I can not use WC_Payments_Account::is_stripe_connected. Instead, I use option_name wcpay_account_data (can not use this constant too due to the same loading issue).
• (Extra) add an additional message to suggest merchants using the previous version of WooCommerce Payments.

Before	After

Testing instructions

• Ensure that WCPay account is still connected on your site.
• Edit this line to a version higher in your current Woo version. For example, 5.8, 6.0, etc.

woocommerce-payments/woocommerce-payments.php

Line 11 in 87c34e4

* WC requires at least: 4.4

• Expect: the Before screenshot above, AND no WCPay menu items is loading.
• Apply this PR.
• Expect: the After screenshot above, AND WCPay menu items are still loading.

---

• Added changelog entry (or does not apply)
• Covered with tests (or have a good reason not to test in description ☝️)
• Tested on mobile (or does not apply)

Post merge

• Added testing instructions to the release testing doc (or does not apply)

[marcinbot]

I wonder, could we move the initialization of self::$api_client and self::$account before the call to check_plugin_dependencies so that here we could just do something like self::$api_client->is_stripe_connected()?

There might not always be an account cached from the previous version at this point, so it would be good to still try to issue a request to check it in the server.

[htdat]

Yeah, ideally we still use self::$api_client->is_stripe_connected().

I wonder, could we move the initialization of self::$api_client and self::$account before the call to check_plugin_dependencies [...]

I will explore this path and see if that's possible.

[htdat]

I wonder, could we move the initialization of self::$api_client and self::$account before the call to check_plugin_dependencies [...]

The flow of self::$api_client->is_stripe_connected() looks like the following:

• WC_Payments_Account->is_stripe_connected()
• WC_Payments_Account->try_is_stripe_connected()
• WC_Payments_Account->get_cached_account_data()
• WC_Payments_API_Client->get_account_data()
• WC_Payments::get_gateway()->is_in_dev_mode()

For the last call above, it's tricky as it requires WC_Payments::$card_gateway assigned value, which is only available very late:

woocommerce-payments/includes/class-wc-payments.php

Lines 267 to 269 in 2d76e58

	self::$card_gateway = new $upe_class( self::$api_client, self::$account, self::$customer_service, self::$token_service, self::$action_scheduler_service, $payment_methods, self::$failed_transaction_rate_limiter );
	} else {
	self::$card_gateway = new $card_class( self::$api_client, self::$account, self::$customer_service, self::$token_service, self::$action_scheduler_service, self::$failed_transaction_rate_limiter );

If we move this gateway code before the call to check_plugin_dependencies, it will create a fatal error for the case WooCommerce does not exist.

[htdat]

I've checked my current code, and found out that it will yell fatal errors in this case:

• WooCommerce is not active.
• WCPay cache account exists.

I tried to change this behavior around the check for old WC versions but could not find a proper way due to the dual purposes of the current check_plugin_dependencies code. May you have some other ideas?

That does not mean this change is not possible but it requires more thought and careful testing.

All in all, we may not change anything here now as it might create more issues with this change.

[marcinbot]

Thank you for digging into this. In general, it feels like the test/dev mode checks shouldn't belong in the payment gateway settings anymore because our plugin is no longer just a payment gateway. The dual purpose of check_plugin_dependencies is also something that has been causing slight headaches for a while. But refactoring either of those is way out of scope of this PR.

I also tested and it looks like we manage to populate the account before deactivating the plugin, so maybe auto-update will have the same effect: it would populate the cache and then update, so things can keep working.

So how about this:

• Move the whole get_option and isset checks into their own function that returns a boolean
  • I'd really like that function to be a part of WC_Payments_Account but requiring it might cause some issues down the line so probably fine to keep it in this file
  • Just isset( $account_data['account'] ) might not be sufficient, as the account can be a string. So we need to change it to isset( $account_data['account'] ) && is_array( $account_data['account'] )

So something like:

private static function has_cached_account_connection(): bool {
	$account_data = get_option( 'wcpay_account_data' );
	return isset( $account_data['account'] ) && is_array( $account_data['account'] );
}

• Call that function inside check_plugin_dependencies rather than after it
• Inside check_plugin_dependencies if any of the dependencies is not there at all always return false.
• If the dependency is present but outdated, instead of returning false, return the value of has_cached_account_connection()
  • So I think we'd only do that here and here

Edit: but that would not render the notices for people who are connected but out of date 🤔 Maybe it is time to split the check_plugin_dependencies into two - one for returning true/false and one for rendering the notice.

[htdat]

In general, it feels like the test/dev mode checks shouldn't belong in the payment gateway settings anymore because our plugin is no longer just a payment gateway.

Agreed here!

I also tested and it looks like we manage to populate the account before deactivating the plugin, so maybe auto-update will have the same effect: it would populate the cache and then update, so things can keep working.

I will double-check this too.

Edit: but that would not render the notices for people who are connected but out of date 🤔 Maybe it is time to split the check_plugin_dependencies into two - one for returning true/false and one for rendering the notice.

What's about the following idea?

• check_plugin_dependencies() stops using param $silent, and returns an array including two keys: passed with a boolean value indicating whether or not and message.
• When calling check_plugin_dependencies(), we display an admin error notice for message, and continue loading the whole plugin or not based on passed. Something like this to replace those lines:

		list( $message, $passed ) = self::check_plugin_dependencies();

		if ( null !== $message ) {
			self::display_admin_error( $message );
		}

		if ( false === $passed ) {
			return;
		}

Update: With this, we do not need to change logic much inside check_plugin_dependencies() and it's still possible to decouple the decision loading the whole plugin and displaying any notice message.

[marcinbot]

Yeah, I think that's a better approach than having a function that does two things. Let's try that.

[htdat]

@marcinbot. I've implemented my idea and your suggestion:

• a453a88 - check_plugin_dependencies returns message and passed flag.
• f03f18c - use has_cached_account_connection as your suggestion.

[jrodger]

Following the test instructions give the expected behaviour. The refactoring of the version check makes sense and the messaging is clear.

I was worried that a site experiencing a temporary connection issue wouldn't attempt to reconnect, but on testing that cache value is also an array - so it's all good. I was thinking of the "no account" value of false, but that's the exact situation where we do want to disable everything 👍

If you could resolve the merge conflict and re-run the CI tests this looks ready to merge.

[htdat]

Awesome, thanks James! Fixed the changelog.
2/5 E2E tests failed but they seem not relevant so I've still merged this PR.