fix: let the exceptions be thrown

[leogermani]

All Submissions:

• Have you followed the Newspack Contributing guideline?
• Does your code follow the WordPress' coding standards and VIP Go coding standards?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

See #1690 (review)

This PR reverts #1636

The pattern we use in Newsletters is not something we use in other plugins and we keep forgetting about it.

The pattern is that instead of returning errors, the methods will throw exceptions that we need to catch. This is the basis of the validate method so I followed the same thing for the whole Mailchimp Cache class.

In #1636, we are supressing this errors, and the result is that they are not being surfaced when they should.

The thing that we were solving in #1636 was not an issue, it was expected behavior, you can see by the methods doc blocks that they are expected to throw.

How to test the changes in this Pull Request:

Smoke test

Other information:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes, as applicable?
• Have you successfully ran tests with your changes locally?

[codecov]

Codecov Report

Attention: Patch coverage is 12.50000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 20.46%. Comparing base (b7fd7e6) to head (e0c700b).
Report is 17 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...ass-newspack-newsletters-mailchimp-cached-data.php	12.50%	7 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk    #1698      +/-   ##
============================================
+ Coverage     20.35%   20.46%   +0.10%     
+ Complexity     2670     2660      -10     
============================================
  Files            48       48              
  Lines         10640    10614      -26     
============================================
+ Hits           2166     2172       +6     
+ Misses         8474     8442      -32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dkoo]

Hmm, on this branch after changing the Mailchimp API key to something invalid and refreshing the cache, I don't see any errors in the Settings page after the SURFACE_ERRORS_AFTER amount of time has passed. I also see some warnings in the debug log that seem related:

[11-Nov-2024 23:14:17 UTC] PHP Warning:  Undefined array key "lists" in /newspack-repos/newspack-newsletters/includes/service-providers/mailchimp/class-newspack-newsletters-mailchimp-cached-data.php on line 542
[11-Nov-2024 23:14:17 UTC] PHP Warning:  Undefined array key "lists" in /newspack-repos/newspack-newsletters/includes/service-providers/mailchimp/class-newspack-newsletters-mailchimp-cached-data.php on line 546
[11-Nov-2024 23:14:17 UTC] PHP Warning:  foreach() argument must be of type array|object, null given in /newspack-repos/newspack-newsletters/includes/service-providers/mailchimp/class-newspack-newsletters-mailchimp.php on line 629

The pattern we use in Newsletters is not something we use in other plugins and we keep forgetting about it.

The pattern is that instead of returning errors, the methods will throw exceptions that we need to catch. This is the basis of the validate method so I followed the same thing for the whole Mailchimp Cache class.

I wonder if we should retain this pattern since it keeps breaking our brains? Could we refactor the validate method to use WP_Errors instead of exceptions that we need to catch and handle manually? Maybe outside the scope of this PR, but I feel this pattern has anecdotally bitten us more than helped us.

[leogermani]

@dkoo I can't reproduce what you described. This branch was outdated, so it might have contributed.

Here is what I see:

• Having a healthy connection
• I change the mailchimp api key to an invalid one
• I see the error right away (because the cache is invalidated)
• I go to line 304 and comment the if out, so the errors are stored right away
• trigger the cron to refresh the cache (wp cron event run newspack_nl_mailchimp_refresh_cache)
• I see that persistent error alert in all pages

Also, I noticed that we were never clearing the lists error. Fixed in e0c700b

[leogermani]

I wonder if we should retain this pattern since it keeps breaking our brains? Could we refactor the validate method to use WP_Errors instead of exceptions that we need to catch and handle manually? Maybe outside the scope of this PR, but I feel this pattern has anecdotally bitten us more than helped us.

ah, and about this. You're probably right. We should use a pattern we use everywhere else... but I personally don't want to engage in this refactor right now :)

[dkoo]

This is working for me after the refresh. Thanks, @leogermani!

[github-actions]

Hey @leogermani, good job getting this PR merged! 🎉

Now, the needs-changelog label has been added to it.

Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label.

If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label.

Thank you! ❤️

[matticbot]

🎉 This PR is included in version 3.6.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[matticbot]

🎉 This PR is included in version 3.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀