[Intl] Add methods to filter currencies more precisely #61431
Conversation
|
Hey! To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done? Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review. Cheers! Carsonbot |
Currencies::isActive() to ensure the currency is active in at least 1 countryCurrencies::isActive() to ensure the currency is active in at least 1 country
|
I think it's pretty much ready for a first review @javiereguiluz. |
| @@ -139,6 +139,40 @@ public static function forNumericCode(int $numericCode): array | |||
| return self::readEntry(['NumericToAlpha3', (string) $numericCode], 'meta'); | |||
| } | |||
|
|
|||
| public static function isActive(string $tenderCurrency): bool | |||
There was a problem hiding this comment.
shouldnt we check by country code and currency code? eg. USD is active, but not in all countries.
There was a problem hiding this comment.
Yes we could do that but that would require the country code in every situation (which is not always desirable).
There was a problem hiding this comment.
The original goal of this PR was to exclude obsolete currencies (not at least in 1 country). But I can add a parameter to specify the country if it's necessary.
There was a problem hiding this comment.
fair enough, but im wondering what real life usecase that would solve?
with getCurrencyCodesForCountry we could put the preferred countries on top in e.g. a dropdown menu (given a country is known)
There was a problem hiding this comment.
For example, in a select with all the currencies available (without the obsolete ones).
E.g. you may want to define prices in a specific currency without specifying the country you want to sell in.
But I think the best is to add all methods to handle all the use cases:
Currencies::existsInAtLeastOneRegion(string $currency, bool $isActive = false): boolCurrencies::existsInRegion(string $currency, string $region, bool $isActive = false): boolCurrencies::forRegion(string $region, bool $isActive = false): array--> return an array of string for the given region
WDYT ?
There was a problem hiding this comment.
sounds reasonable 👍 i would avoid "region" terminology, the surfacing public api uses "country (code)"
There was a problem hiding this comment.
Ok. Even though the ICU dataset uses region tags for countries ? Because there is a difference between regions and countries. Countries are juridically organisation (for instance France) but regions are for territorial areas (for instance the Martinique Island (MQ) that belongs to France)
There was a problem hiding this comment.
yes, we have Countries and Timezones::forCountryCode already
I will add this ASAP 👍🏻 |
|
I added the requested methods + tender filter but I need to add fews tests to ensure all uses cases are covered. I also need to update the CHANGELOG. I'll continue this ASAP. |
Currencies::isActive() to ensure the currency is active in at least 1 country| string $country, | ||
| string $currency, | ||
| bool $activeOnly = true, | ||
| string $date = 'today', |
There was a problem hiding this comment.
?\DateTimeImmutable $date = null?
providing new \DateTimeImmutable('today') is little effort, while providing more flexibility
| public static function existsInCountry( | ||
| string $country, | ||
| string $currency, | ||
| bool $activeOnly = true, |
There was a problem hiding this comment.
nullable boolean $active, to account for both active + inactive?
There was a problem hiding this comment.
In this case, just use false. I named the parameter activeOnly because I thought I would make no sense to retrieve the inactive ones.
WDYT ?
There was a problem hiding this comment.
However, I'm also fine with the nullable parameter.
| * @param bool $activeOnly exclude currencies that are not active on the given $date in countries | ||
| * @param string $date the date on which the check will be performed if $activeOnly is set to true | ||
| */ | ||
| public static function existsInAtLeastOneCountry( |
There was a problem hiding this comment.
this feels a bit weird naming compared to exists(), which should also checks existence in at least 1 country
in that sense im still fine with public API existsActive (or adding nullable boolean $active parameter to current exists() method)
There was a problem hiding this comment.
Ok, renaming the method to exists is also fine for me 👍
| * | ||
| * @throws MissingResourceException if the given $country does not exist | ||
| * | ||
| * @return array<non-empty-string, array{from?: non-empty-string, to?: non-empty-string, tender?: bool}> |
There was a problem hiding this comment.
im not sure this is the proper return type compared to getCurrencyCodes and forNumeric, unexpected at least i think
Description
In the ICU dataset, there are values that are no longer relevant / no longer used today (e.g. the BEF currency). This PR add a method that check the metadata in the ICU metadata to check if the currency is active in at least 1 country.
Limitations
There are currencies that do not have legal tender and therefore no available date ranges. That's why only the currencies that has a legal value within the country borders can be checked.