[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
…`UTC` is an alias Note: Milliseconds are still required to normalize date-times that are before 1970.
…t value for $date in it
Do we have extra compilation processes beside the one that I used to update the |
|
no, but we have a docker |
I tried running the The default ---------------------------------------------------------------------------
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>&1
Output:
---------------------------------------------------------------------------
c++ ... stubdata.cpp
In 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 edited #!/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.phpApart from that, the image is already on the latest version of ICU : |
|
Should I commit this environment parameter that solves this issue in this PR? |
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. |
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 the |
|
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. |
|
Okay, in that case, apart from the updated image version, I think this PR is complete! Thank you all for your reviews and comments. |
|
sorry for noise :) turns out we're already updated to icu 77: #60157 so the bookkeeping here done to we should update |
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).