Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[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.

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

Already on GitHub?Sign in to your account

Merged

Conversation

@Crovitche-1623
Copy link
Contributor

@Crovitche-1623Crovitche-1623 commentedAug 15, 2025•
edited
Loading

QA
Branch?7.4
Bug fix?no
New feature?yes
Deprecations?no
IssuesClose#61365
LicenseMIT

Context

In the ICU dataset, some values are obsolete or no longer used today (e.g., the BEF currency).

Description

This PR adds several methods to filter currencies based on ICU metadata, which includes:

  • tender — whether it is a legal-tender currency
  • validity periods — from/to dates (i.e., “was it active on a given date?”)

New methods

Currencies::forCountry(    string$country,    ?bool$legalTender =true,    ?bool$active =true,    \DateTimeInterface$date =new \DateTimeImmutable('today',new \DateTimeZone('Etc/UTC'))): array

Returns a list of currency codes used in the given country (FR, CH, …), filterable by legal tender and/or by active status at a specific date.

Note

By default, only legal-tender currencies that are active today are returned.

Tip

Passingnull to the$legalTender or$active to not filter anything

Warning

If validity dates are missing from the ICU metadata, a\RuntimeException may be thrown.

Currencies::isValidInCountry(    string$country,    string$currency,    ?bool$legalTender =true,    ?bool$active =true,    \DateTimeInterface$date =new \DateTimeImmutable('today',new \DateTimeZone('Etc/UTC'))): bool

Returns true if the given currency is valid in the specified country.

Note

By default, only legal-tender currencies that are active today are returned.

Tip

Passingnull to the$legalTender or$active to not filter anything

Warning

  1. If validity dates are missing from the ICU metadata, a\RuntimeException may be thrown.
  2. An\InvalidArgumentException may be thrown if the given currency code is invalid.
Currencies::isValidInAnyCountry(    string$currency,    ?bool$legalTender =true,    ?bool$active =true,    \DateTimeInterface$date =new \DateTimeImmutable('today',new \DateTimeZone('Etc/UTC'))): bool

Returns true if the given$currency is valid in any country worldwide (useful to filter out retired ones likeBEF).

Note

By default, only legal-tender currencies that are active today are returned.

Tip

Passingnull to the$legalTender or$active to not filter anything

Warning

  1. If validity dates are missing from the ICU metadata, a\RuntimeException may be thrown.
  2. An\InvalidArgumentException may be thrown if the given currency code is invalid.

The public methods above use two new private helpers,isLegalTender() andisDateActive(), which encapsulate the tender and date logic.

Why is this useful?

  • Checkout flows: only show legal & currently active currencies for a billing country.
  • Back-office forms: hide legacy codes (for instance BEF) from selectors.
  • Historical reporting: rebuild valid currency sets as of a past date (e.g. 2010-01-01).

Limitations

  1. Some currencies are not legal tender and therefore have no date ranges. This can cause issues when checking whether a currency is active (within a date range). --> However, the exception can be caught easily like it's done in[Form][Intl] Allow the developer to choose if he want to include/exclude currencies that do not have validity dates. #62225 to skip the currencies that do not have date ranges.
  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).

fuzoh and ro0NL reacted with thumbs up emoji
@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-1623Crovitche-1623 changed the title[DRAFT] feat: AddCurrencies::isActive() to ensure the currency is active in at least 1 country[Intl] AddCurrencies::isActive() to ensure the currency is active in at least 1 countryAug 15, 2025
@Crovitche-1623Crovitche-1623 marked this pull request as ready for reviewAugust 15, 2025 09:33
@carsonbotcarsonbot added this to the7.4 milestoneAug 15, 2025
@Crovitche-1623
Copy link
ContributorAuthor

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

Copy link
Contributor

@ro0NLro0NL 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 begetCurrencyCodesForCountry(string $country, ?bool $active = null) +existsForCountry(string $country, ?bool $active = null)
?

Crovitche-1623 reacted with thumbs up emoji
@Crovitche-1623
Copy link
ContributorAuthor

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

?

I will add this ASAP 👍🏻

@Crovitche-1623
Copy link
ContributorAuthor

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-1623Crovitche-1623 changed the title[Intl] AddCurrencies::isActive() to ensure the currency is active in at least 1 country[Intl] Add methods to filter currencies more preciselyAug 15, 2025
@Crovitche-1623
Copy link
ContributorAuthor

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

Copy link
Member

@nicolas-grekasnicolas-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 ;)

Crovitche-1623 reacted with thumbs up emoji
@Crovitche-1623
Copy link
ContributorAuthor

Crovitche-1623 commentedAug 20, 2025•
edited
Loading

no, but we have a dockercompile, but im not sure from what env it's usually done. It shouldnt matter much :)

I tried running thecompile script from my machine and I had to specify the CPP compiler to version 17 (instead of the actual 20 which seems to be too newer) to make it work.

The defaultcompile was failing with the following output:

---------------------------------------------------------------------------                      ICU Resource Bundle Compilation---------------------------------------------------------------------------Starting git clone. This may take a while...Git clone to /tmp/icu-data complete.Checking out `release-77-1` for version `77`...Building genrb.Running configure...Running make...[1/6] libicudata.so...Error while running:    /tmp/icu-data/icu4c/source/stubdata$ make 2>&1 && make install 2>&1Output:---------------------------------------------------------------------------   c++   ...  stubdata.cppIn file included from ../common/unicode/platform.h:25,                 from ../common/unicode/ptypes.h:46,                 from ../common/unicode/umachine.h:46,                 from ../common/unicode/utypes.h:38,                 from stubdata.h:29,                 from stubdata.cpp:22:../common/unicode/uversion.h:105:57: warning: nested namespace definitions only available with '-std=c++17' or '-std=gnu++17' [-Wc++17-extensions]  105 | #       define U_ICU_NAMESPACE U_ICU_ENTRY_POINT_RENAME(icu)      |^~~../common/unicode/uvernum.h:121:50: note: in definition of macro 'U_DEF_ICU_ENTRY_POINT_RENAME'  121 | #       define U_DEF_ICU_ENTRY_POINT_RENAME(x,y) x## y      |^../common/unicode/uvernum.h:123:47: note: in expansion of macro 'U_DEF2_ICU_ENTRY_POINT_RENAME'  123 | #       define U_ICU_ENTRY_POINT_RENAME(x)    U_DEF2_ICU_ENTRY_POINT_RENAME(x,U_ICU_VERSION_SUFFIX)      |^~~~~~~~~~~~~~~~~~~~~~~~~~~~~../common/unicode/uversion.h:105:32: note: in expansion of macro 'U_ICU_ENTRY_POINT_RENAME'  105 | #       define U_ICU_NAMESPACE U_ICU_ENTRY_POINT_RENAME(icu)      |^~~~~~~~~~~~~~~~~~~~~~~~../common/unicode/uversion.h:180:33: note: in expansion of macro 'U_ICU_NAMESPACE'  180 | #define U_HEADER_ONLY_NAMESPACE U_ICU_NAMESPACE::U_HEADER_NESTED_NAMESPACE      |^~~~~~~~~~~~~~~../common/unicode/uversion.h:182:11: note: in expansion of macro 'U_HEADER_ONLY_NAMESPACE'  182 | namespace U_HEADER_ONLY_NAMESPACE {}      |^~~~~~~~~~~~~~~~~~~~~~~In file included from ../common/unicode/udata.h:25,                 from stubdata.h:30:../common/unicode/localpointer.h:561:26: error: parameter declared 'auto'  561 | template <typename Type, auto closeFunction>      |^~~~../common/unicode/localpointer.h:573:76: error: template argument 2 is invalid  573 |     explicit LocalOpenPointer(std::unique_ptr<Type, decltype(closeFunction)> &&p)      |^../common/unicode/localpointer.h:583:78: error: template argument 2 is invalid  583 |     LocalOpenPointer &operator=(std::unique_ptr<Type, decltype(closeFunction)> &&p) {      |^../common/unicode/localpointer.h:599:59: error: template argument 2 is invalid  599 |     operator std::unique_ptr<Type, decltype(closeFunction)> () && {      |^../common/unicode/localpointer.h:551:81: note: invalid template non-type parameter  551 |     using LocalPointerClassName = internal::LocalOpenPointer<Type, closeFunction>      |^../common/unicode/udata.h:434:1: note: in expansion of macro 'U_DEFINE_LOCAL_OPEN_POINTER'  434 | U_DEFINE_LOCAL_OPEN_POINTER(LocalUDataMemoryPointer, UDataMemory, udata_close);      |^~~~~~~~~~~~~~~~~~~~~~~~~~~*** Failed compilation command follows: ----------------------------------------------------------c++ -D_REENTRANT -DU_HAVE_ELF_H=1 -DU_HAVE_STRTOD_L=1 -DU_HAVE_XLOCALE_H=0 -I../common -DU_ALL_IMPLEMENTATION -DU_ATTRIBUTE_DEPRECATED= --std=c++0x -W -Wall -pedantic -Wpointer-arith -Wwrite-strings -Wno-long-long -c -DPIC -fPIC -o stubdata.o stubdata.cpp--- ( rebuild with "make VERBOSE=1 " to show all parameters ) --------make:*** [../config/mh-linux:51: stubdata.o] Error 1---------------------------------------------------------------------------

Here is my editedcompile file:

#!/usr/bin/env bash[[!-d /tmp/symfony/icu ]]&& mkdir -p /tmp/symfony/icu# I had to specify the --platform to make it work on my ARM machine, otherwise docker try to obtain the same architecture as the host (not found for ARM for jakzal/php-intl)docker run \    --platform=linux/amd64 \    -e CXXFLAGS="-std=c++17 -fext-numeric-literals" \    -it --rm --name symfony-intl \    -u$(id -u):$(id -g) \    -v /tmp/symfony/icu:/tmp \    -v$(pwd):/symfony \    -w /symfony \    jakzal/php-intl:8.3-74.1 \    php src/Symfony/Component/Intl/Resources/bin/update-data.php

Apart from that, the image is already on the latest version of ICU :
https://github.com/jakzal/docker-symfony-intl/blob/eb082792c4e9e935e1efab783752235a05a63b9d/Dockerfile#L2C1-L2C21

GromNaN reacted with thumbs up emoji

@Crovitche-1623
Copy link
ContributorAuthor

Should I commit this environment parameter that solves this issue in this PR?

-e CXXFLAGS="-std=c++17 -fext-numeric-literals" \

@ro0NL
Copy link
Contributor

make it work on my ARM machine

the image is an AMD one, and latest is jakzal/php-intl:8.4-77.1

if adding CXXFLAGS solves the compatibility issue, it seems fine to me. ideally some GHA job commits the latest diff to a PR, but that's another story.

@Crovitche-1623
Copy link
ContributorAuthor

Crovitche-1623 commentedAug 20, 2025•
edited
Loading

the image is an AMD one, and latest is jakzal/php-intl:8.4-77.1

Do you want me to update the image to the latest version? Don’t you think it might be better to stick with the lowest supported PHP version for the Symfony version in use (8.2 or higher for the upcoming Symfony 7.4), so we can be sure that theupdate-data.php script can run in any situation?

@ro0NL
Copy link
Contributor

ro0NL commentedAug 20, 2025•
edited
Loading

generally, for what is a compilation artifact, im curious how it compiles on latest php yes. But this could be a separate PR, another concern is the compilation thru docker is not enforced by any means. If we update a thousand language files, it's impossible to review. Hence the docker compile.

Crovitche-1623 reacted with thumbs up emoji

@Crovitche-1623
Copy link
ContributorAuthor

Crovitche-1623 commentedAug 20, 2025•
edited
Loading

Okay, in that case, apart from the updated image version, I think this PR is complete!

Thank you all for your reviews and comments.

@ro0NL
Copy link
Contributor

ro0NL commentedAug 20, 2025•
edited
Loading

sorry for noise :)

turns out we're already updated to icu 77:#60157

so the bookkeeping here done tocompile is correct 👍 even though im not sure the latest compile was done from a php8.4 image, but OK :)

we should updategetIcuStubVersion still.

nicolas-grekas added a commit that referenced this pull requestAug 29, 2025
…ovitche-1623, nicolas-grekas)This PR was merged into the 6.4 branch.Discussion----------[Intl] Add metadata about currencies' validity dates| Q             | A| ------------- | ---| Branch?       | 6.4| Bug fix?      | no| New feature?  | no| Deprecations? | no| Issues        | -| License       | MITThis backports intl data from#61431 to 6.4This also syncs the ICU compilation scripts with 7.4.This allows generating ICU data once on 6.4 and not have to care about running that again on higher branches.Commits-------4a85bb3 Sync intl scripts243f8f9 [Intl] Add metadata about currencies' validtity dates
@nicolas-grekas
Copy link
Member

Thank you@Crovitche-1623.

@nicolas-grekasnicolas-grekas merged commit545c390 intosymfony:7.4Aug 29, 2025
10 of 13 checks passed
@stof
Copy link
Member

@Crovitche-1623 it would be great to update the PR description to describe the new methods being added. This will make it easier for people seeing this PR to figure out what it actually provides (including for the documentation team when updating the documentation, and for@javiereguiluz when writing the "New in Symfony 7.4" blog posts)

Crovitche-1623 and javiereguiluz reacted with thumbs up emoji

@Crovitche-1623
Copy link
ContributorAuthor

@stof 👍🏻 I'll do that as soon as I'm back from my holidays (I do not have network and no laptop).

@Crovitche-1623
Copy link
ContributorAuthor

Done@stof

javiereguiluz reacted with heart emoji

@javiereguiluz
Copy link
Member

javiereguiluz commentedSep 9, 2025•
edited
Loading

Folks, thanks for the work done here. The new methods are useful 💪

However, I think the main issue remains: the list of currencies returned by default by Symfony (e.g. in CurrencyType) looks wrong because it includes tens and tens of currencies no longer used since decades ago. Maybe we could add a new config option toCurrencyType to only show active currencies and use these new methods to create that list? Thanks!

Crovitche-1623 reacted with heart emoji

@Crovitche-1623
Copy link
ContributorAuthor

Crovitche-1623 commentedSep 9, 2025•
edited
Loading

Thanks@javiereguiluz, I'll try to develop this ASAP in another PR to add three new options toCurrencyType, corresponding to the methods implemented in the current PR:

  1. active (bool|null): returns the currencies that are active for the givendate option (defaults tonull in 7.4 to preserve BC compatibility and keep the same results).
  2. legal_tender (bool|null): returns the currencies that are legal tender or not.null skips this filter (defaults tonull to preserve BC compatibility).
  3. date (\DateTimeInterface): useful only when used together with theactive option (defaults to "today").

Another good feature would be to add a fourth option namedcountry (orcountries if we want to allow more than one) toCurrencyType, to return only the currencies corresponding to the specified country code.

fabpot added a commit that referenced this pull requestOct 1, 2025
…_tender` options to `CurrencyType` (Crovitche-1623)This PR was squashed before being merged into the 7.4 branch.Discussion----------[Form] Add new `active_at`, `not_active_at` and `legal_tender` options to `CurrencyType`| Q             | A| ------------- | ---| Branch?       | 7.4| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Issues        | -| License       | MITAdd the options `active_at`, `not_active_at` and `legal_tender` from#61431 that give the possibility to filter the currencies more precisely. For instance:- Keep only the current ones for the given `active_at` (`today` by default)- Keep only the currencies that are [legal tender](https://en.wikipedia.org/wiki/Legal_tender)Regarding BC compatibility, if people want to have the same results as prior 7.4, they should set the `active_at` and `legal_tender` options to null explicitely.Commits-------a378fe6 [Form] Add new `active_at`, `not_active_at` and `legal_tender` options to `CurrencyType`
nicolas-grekas added a commit that referenced this pull requestOct 2, 2025
This PR was merged into the 7.4 branch.Discussion----------[Form] do not install symfony/intl < 7.4| Q             | A| ------------- | ---| Branch?       | 7.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Issues        || License       | MITthe changes done in#61837 require#61431Commits-------badf463 do not install symfony/intl < 7.4
This was referencedOct 27, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@fabpotfabpotAwaiting requested review from fabpot

+1 more reviewer

@ro0NLro0NLro0NL approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

7 participants

@Crovitche-1623@carsonbot@ro0NL@nicolas-grekas@stof@javiereguiluz@fabpot
[8]ページ先頭
©2009-2025 Movatter.jp