Add ENUM class for country codes #7925
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: 0 B Total Size: 1.27 MB ℹ️ View Unchanged
|
| * | ||
| * @psalm-immutable | ||
| */ | ||
| class Country_Codes extends Base_Constant { |
There was a problem hiding this comment.
Can we use Country instead of Country_Codes?
It would be easier to read e.g. Country::JAPAN === $account_country
There was a problem hiding this comment.
I've considered your point, but there's a potential for confusion. Some might expect Country::JAPAN to return 'Japan' or its equivalent in the currently configured language. To avoid this confusion, I opted for Country_Code.
There was a problem hiding this comment.
Makes sense, so I guess we can use Country_Code?
There was a problem hiding this comment.
Country_Code makes more sense, +1
There was a problem hiding this comment.
Good point. Refactored in d3ce810
| * | ||
| * @psalm-immutable | ||
| */ | ||
| class Country_Codes extends Base_Constant { |
There was a problem hiding this comment.
Country_Code makes more sense, +1
| */ | ||
| class Country_Codes extends Base_Constant { | ||
| const AFGHANISTAN = 'AF'; | ||
| const ALAND_ISLANDS = 'AX'; |
There was a problem hiding this comment.
Can we write a test for the output of this class? If that test breaks devs will know that some change has been made here willingly or not
There was a problem hiding this comment.
Added tests here: 0942465
|
@zmaglica do you plan to replace country code in |
The linked file, is taken from the WooCommerce Repository (link), which serves as a fallback for older WooCommerce installations lacking locale-info.php at that time. Given its introduction three years ago and its specific use-case, I excluded it from the country codes refactor, since all our supported versions of WooCommerce uses one provided by WooCommerce plugin. |
Can you please link the file where this logic is present? |
@zmaglica is it enough to verify If this change is working or do you think we need some manual testing as well? |
There are 2 places where this logic can be found and only in these two places, the mentioned One of the places is located here: woocommerce-payments/includes/class-wc-payments-localization-service.php Lines 113 to 118 in 6610e65 If you check the code, you will notice that the Another instance where this particular file is loaded can be found here: woocommerce-payments/includes/admin/class-wc-payments-admin.php Lines 795 to 799 in 6610e65 Basically, this code check is WooCommerce version below 6.0, and if it is, it loads And if you take a look inside the WooCommerce repository, the locale-info.php is added in 5.7.0 version In terms of content, these file from WooCommerce and WooPayments are identical. |
Unit tests cover this topic very well, so we don't have to do manual testing. This is a static class, and if it not included or used somewhere, the unit tests will fail. |
Co-authored-by: Naman Malhotra <[email protected]>
Fixes #7886
Changes proposed in this Pull Request
This pull request introduces a class that functions similarly to an enum class, specifically designed for managing country codes.
Testing instructions
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge