[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. |
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…cases to achieve 100% code coverage
|
I’ve applied the requested changes, this should be ready for a second review. |
src/Symfony/Component/Intl/Data/Generator/CurrencyDataGenerator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Intl/Data/Generator/CurrencyDataGenerator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Intl/Data/Generator/CurrencyDataGenerator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Intl/Data/Generator/CurrencyDataGenerator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Intl/Data/Generator/CurrencyDataGenerator.php
Outdated
Show resolved
Hide resolved
| ): bool { | ||
| if (!\array_key_exists('from', $currencyMetadata)) { | ||
| // Note: currencies that are not legal tender don't have often validity dates. | ||
| throw new \RuntimeException("Cannot check whether the currency $currency is active or not in $country because they are no validity dates available."); |
There was a problem hiding this comment.
@ro0NL IMO no, because it would mix the results with the currencies that are really inactives. For instance some currencies may be active but just don't have date ranges (for instance CHE currency).
There was a problem hiding this comment.
oh, got it. This is only the date check, i thought we had to account for legalTender still.
maybe isDateActive with a non-nullabel $date param
There was a problem hiding this comment.
You suggest to use the parameter $active directly in isActive instead of the actual early return in isValidInCountry, isValidInAnyCountry, forCountry ?
There was a problem hiding this comment.
In my opinion, it's not the job of the method to skip itself. It's weird from a semantic point of view.
There was a problem hiding this comment.
i suggested private static function isDateActive(string $country, string $currency, array $currencyMetadata, \DateTimeInterface $date): bool
…`UTC` is an alias Note: Milliseconds are still required to normalize date-times that are before 1970.
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
$active).