Add rudimentary try catch for all remote endpoint spec evaluators

[ilyasfoo]

Changes proposed in this Pull Request:

In light of a recent fatal due to faulty rule in PaymentGatewaySuggestions, this PR aims to add rudimentary and broad try-catch in order to prevent future fatals.

How to test the changes in this Pull Request:

In order to simulate errors either:

1. Cherry-pick the commit 52f8fa8 from this branch
2. Or manually set the following APIs
   • PaymentGatewaySuggestionsDataSourcePoller (file): https://api.npoint.io/83d63d29d478ececdbc5
   • DataSourcePoller (file): https://api.npoint.io/099094352a530a673fef
   • RemoteFreeExtensionsDataSourcePoller (file): https://api.npoint.io/34e72d8b75957e47ff1e
   • WCPayPromotionDataSourcePoller (file): https://api.npoint.io/6eed3f7093fa2ca8fd1f

Testing

1. In your environment, use PHP 8.1+
2. If using an existing site, clear transients by running wp transient delete --all
3. Ensure WooCommerce > Settings > Advanced > Woo.com > Display suggestions within WooCommerce is checked
4. Go to Setup Wizard, /wp-admin/admin.php?page=wc-admin&path=/setup-wizard
5. Click continue
6. Select I'm just starting my business and continue
7. Select Australia — Australian Capital Territory and continue
8. Skip plugin installation
9. Observe no fatal when loading homescreen
10. Go to Tools > WCA Test Helper > Tools > Run wc_admin_daily job
11. Ensure the job is run successfully

[github-actions]

Hi @chihsuan, @moon0326, @woocommerce/ghidorah

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

[github-actions]

Test Results Summary

Commit SHA: 7ea67ef

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 37s
E2E Tests	275	0	0	21	0	296	10m 1s

---

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

[psealock]

No fatals occur and the wc admin job runs successfully.

I am seeing the error Warning: strpos() expects parameter 1 to be string, array given in... at the top of the page, but I believe this is to be expected.

I also confirmed there are no unchecked instances of evaluate in the codebase for RIN evaluation.

The only objection I have is this PR should target trunk and ultimately cherry-picked to release/8.5

[ilyasfoo]

Thanks, @psealock!

I am seeing the error Warning: strpos() expects parameter 1 to be string, array given in... at the top of the page, but I believe this is to be expected.

I wasn't able to get this, do you have the full warning message?

Also, can you confirm if you're using PHP 8.1+? I forgot to include that in the testing instructions.

I've also updated the branch and fixed lint, please take another look!

[adrianduffell]

Thanks for your work on this @ilyasfoo ❤️

This is just a nitpick: I'd like to see generic exception catch-alls handled higher up in the call stack, as there is still the potential for exceptions to be raised in other lines of code elsewhere (in any line of code really). I could see perhaps having a single catch-all within each feature (like the remote inbox), that wraps all code that gets executed within it.

For deeper level code like this, I think it's better to catch only specific exceptions that could be reasonably anticipated and handling those appropriately, e.g. with logging or a warning to the user, so that problems are more visible when they arise.

[ilyasfoo]

This is just a nitpick: I'd like to see generic exception catch-alls handled higher up in the call stack, as there is still the potential for exceptions to be raised in other lines of code elsewhere (in any line of code really). I could see perhaps having a single catch-all within each feature (like the remote inbox), that wraps all code that gets executed within it.

@adrianduffell Good suggestion, but I believe the try-catch blocks added here are at the highest level we can get. For example, PaymentGatewaySuggestion is called with a static function, thus it can be called from anywhere in the code.

Also, the benefit with adding the try-catch blocks in each individual evaluation will limit evaluation failures to individual spec, instead of throwing out the entire data.

I generally agree that we may benefit from having multiple try-catch levels, where the highest is at feature-level, and logs error. However, I believe that requires a lot more changes, and we should focus on minimal changes with the PR since we also need to cherry-pick these changes into previous versions.

[chihsuan]

Looks good and confirmed no errors are present in a PHP 8.2 Site. 👍

However, I noticed the payment task doesn't display properly. But I agree we can create a follow-up issue for further changes.

[ilyasfoo]

Thanks, @chihsuan!

However, I noticed the payment task doesn't display properly. But I agree we can create a follow-up issue for further changes.

This is expected when you're using the faulty spec since there is only 1 spec defined, and it will result in an empty list. Still, I agree that we should handle payments task better when there's no suggestions available.