Skip to content

[Intl] Add methods to filter currencies more precisely #61431

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 27 commits into
base: 7.4
Choose a base branch
from
Open

[Intl] Add methods to filter currencies more precisely #61431

wants to merge 27 commits into from
+2,532 −0

Conversation

Crovitche-1623
Copy link

@Crovitche-1623 Crovitche-1623 commented Aug 15, 2025 •
edited
Loading

Q A
Branch? 7.4
Bug fix? no
New feature? yes
Deprecations? no
Issues Close #61365
License MIT

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

  1. There are currencies that do not have legal tender and therefore no available date ranges. That can cause problem when we check if the currency is active (within the date range).
  2. If we have a case in the future where a currency is no longer valid for a given date range but becomes valid again in the future, this will not work with the current date check (parameter $active).

@carsonbot
Copy link

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

@Crovitche-1623 Crovitche-1623 changed the title [DRAFT] feat: Add Currencies::isActive() to ensure the currency is active in at least 1 country [Intl] Add Currencies::isActive() to ensure the currency is active in at least 1 country Aug 15, 2025
@Crovitche-1623 Crovitche-1623 marked this pull request as ready for review August 15, 2025 09:33
@carsonbot carsonbot added this to the 7.4 milestone Aug 15, 2025
@Crovitche-1623
Copy link
Author

I think it's pretty much ready for a first review @javiereguiluz.

ro0NL
ro0NL reviewed Aug 15, 2025
View reviewed changes
Copy link
Contributor

@ro0NL ro0NL left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps an alternative API could be getCurrencyCodesForCountry(string $country, ?bool $active = null) + existsForCountry(string $country, ?bool $active = null)
?

@Crovitche-1623
Copy link
Author

perhaps an alternative API could be getCurrencyCodesForCountry(string $country, ?bool $active = null) + existsForCountry(string $country, ?bool $active = null)

?

I will add this ASAP 👍🏻

@Crovitche-1623
Copy link
Author

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.

@Crovitche-1623 Crovitche-1623 changed the title [Intl] Add Currencies::isActive() to ensure the currency is active in at least 1 country [Intl] Add methods to filter currencies more precisely Aug 15, 2025
ro0NL
ro0NL reviewed Aug 15, 2025
View reviewed changes
@Crovitche-1623
Copy link
Author

I’ve applied the requested changes, this should be ready for a second review.

ro0NL
ro0NL reviewed Aug 18, 2025
View reviewed changes
fabpot
fabpot requested changes Aug 19, 2025
View reviewed changes
@Crovitche-1623 Crovitche-1623 requested a review from fabpot August 19, 2025 07:33
nicolas-grekas
nicolas-grekas reviewed Aug 19, 2025
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks (but CS things).
I didn't comment on all places where my comments apply, you should be able to infer the rest ;)

ro0NL
ro0NL reviewed Aug 19, 2025
View reviewed changes
ro0NL
ro0NL reviewed Aug 20, 2025
View reviewed changes
Copy link
Contributor

@ro0NL ro0NL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice :) should we run the compilation again as a final step?

@Crovitche-1623
Copy link
Author

nice :) should we run the compilation again as a final step?

Do we have extra compilation processes beside the one that I used to update the meta.php file (Resources/bin/update-data.php)? BTW, the data are up to date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@fabpot fabpot Awaiting requested review from fabpot

+1 more reviewer

@ro0NL ro0NL ro0NL approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Feature Intl Status: Reviewed
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.

[Intl] Methods to check if a given currency is obsolete/active
5 participants
@Crovitche-1623 @carsonbot @fabpot @nicolas-grekas @ro0NL