Skip to content

[Form] Add new active_at, not_active_at and legal_tender options to CurrencyType #61837

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
Merged
fabpot merged 1 commit into from
Oct 1, 2025
Merged

[Form] Add new active_at, not_active_at and legal_tender options to CurrencyType #61837

fabpot merged 1 commit into from
Oct 1, 2025

Conversation

@Crovitche-1623
Copy link
Contributor

@Crovitche-1623 Crovitche-1623 commented Sep 24, 2025 •
edited
Loading

Q A
Branch? 7.4
Bug fix? no
New feature? yes
Deprecations? no
Issues -
License MIT

Add 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

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.

@carsonbot carsonbot added this to the 7.4 milestone Sep 24, 2025
@Crovitche-1623
Copy link
Contributor Author

Crovitche-1623 commented Sep 24, 2025

Is it necessary to add more tests given that the filtering method isValidInAnyCountry has already been tested in the Intl component ?

@symfony symfony deleted a comment from carsonbot Sep 25, 2025
nicolas-grekas
nicolas-grekas reviewed Sep 25, 2025
View reviewed changes
@Crovitche-1623 Crovitche-1623 changed the title [Form] Add new active, legal_tender and date options to CurrencyType [Form] Add new active, legal_tender and active_or_not_at options to CurrencyType Sep 25, 2025
@ro0NL
Copy link
Contributor

ro0NL commented Sep 25, 2025

should we force the new defaults in a deprecated manner?

@Crovitche-1623
Copy link
Contributor Author

Crovitche-1623 commented Sep 25, 2025

should we force the new defaults in a deprecated manner?

@ro0NL
We could using an additional option that the developer had to toggle to explicitely specify to disable the deprecation but IMHO, I'm not even sure that most people use obsolete currencies nowadays. This BC will affect 1% or less. The choice is yours...

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 26, 2025

I don't understand why we need active and active_or_not_at? That's to cover the need of listing only non-active currencies at a given date? Is that a common enough usecase?

@Crovitche-1623
Copy link
Contributor Author

Crovitche-1623 commented Sep 26, 2025 •
edited
Loading

I don't understand why we need active and active_or_not_at? That's to cover the need of listing only non-active currencies at a given date? Is that a common enough usecase?

IMHO yes it could be a common use case to list only the non-active currencies for a given date, for the following reasons:

  1. For example, if your app needs to search invoices that were issued with the wrong currencies and show a dropdown of those invalid currencies, you would still need two parameters (active = false and active_or_not_at = the previous year).
  2. We have no guarantee that certains symfony applications will be updated within 4 years (the duration of an LTS version). Therefore, the CLDR registry will not necessarily be updated during this period if symfony is not updated. Over this time, currency validity may change, and interfaces (with form using CurrencyType) may be created to retroactively correct ‘incorrect’ currencies.

WDYT @nicolas-grekas ?

@Crovitche-1623 Crovitche-1623 changed the title [Form] Add new active, legal_tender and active_or_not_at options to CurrencyType [Form] Add new active_at, active_or_not_at and legal_tender options to CurrencyType Sep 26, 2025
@Crovitche-1623 Crovitche-1623 changed the title [Form] Add new active_at, active_or_not_at and legal_tender options to CurrencyType [Form] Add new active_at, not_active_at and legal_tender options to CurrencyType Sep 26, 2025
nicolas-grekas
nicolas-grekas reviewed Sep 26, 2025
View reviewed changes
nicolas-grekas
nicolas-grekas approved these changes Sep 26, 2025
View reviewed changes
@Crovitche-1623
Copy link
Contributor Author

Crovitche-1623 commented Sep 26, 2025

@nicolas-grekas A part from that I think I forgot to process the eventual tz and to-tz attributes from the CLDR. I don't know if it'll change the generated data though but I'll try to fix this point ASAP. see https://github.com/unicode-org/cldr/blob/2cac2a547276f5253e0556dc3fb0b8c77e46ab40/common/supplemental/supplementalData.xml#L369-L370

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 26, 2025

If we want to account for the TZ, maybe we should put timestamps in the metadata instead of date strings? (if that's what we have right now)

@Crovitche-1623
Copy link
Contributor Author

Crovitche-1623 commented Sep 26, 2025 •
edited
Loading

If we want to account for the TZ, maybe we should put timestamps in the metadata instead of date strings? (if that's what we have right now)

Yes, or we could convert the from and to directly into UTC during the processing to see if the Y-m-d date changes. However, we would lose the time at which the change took place... This is a very specific case. Which solution do you prefer?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 26, 2025

it might be more readable to have Y-m-d H:i:s UTC strings indeed

@Crovitche-1623
Copy link
Contributor Author

Crovitche-1623 commented Sep 26, 2025 •
edited
Loading

it might be more readable to have Y-m-d H:i:s UTC strings indeed

Ok 👍🏻 Do I create another PR for this or can I do the required changes in the current one ?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 26, 2025 •
edited
Loading

It should be a separate PR targetting branch 6.4

Crovitche-1623 added a commit to Crovitche-1623/symfony that referenced this pull request Sep 26, 2025
@Crovitche-1623
feat: support timezone in generated data in currencies validity
2d23f8e 
see symfony#61837 (comment)
@Crovitche-1623 Crovitche-1623 mentioned this pull request Sep 26, 2025
Merged
@Crovitche-1623
Copy link
Contributor Author

Crovitche-1623 commented Sep 26, 2025 •
edited
Loading

I will change the format to retrieve time once #61859 is merged. 👍🏻 Done, the tests will pass once #61859 will be merged.

stof
stof requested changes Sep 26, 2025
View reviewed changes
@stof stof mentioned this pull request Sep 26, 2025
Open
@Crovitche-1623 Crovitche-1623 requested a review from stof September 29, 2025 11:32
Crovitche-1623
Crovitche-1623 commented Sep 29, 2025
View reviewed changes
src/Symfony/Component/Form/Extension/Core/Type/CurrencyType.php
return array_flip($filteredCurrencyNames);
},
),
$choiceTranslationLocale.($activeAt ?? $notActiveAt)?->format('Y-m-d\TH:i:s').$legalTenderCacheKey,
Copy link
Contributor Author

@Crovitche-1623 Crovitche-1623 Sep 29, 2025

Choose a reason for hiding this comment

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

Note: Some timezones use unusual offsets, with the smallest official step being 15 minutes. I used the seconds as the smallest units but we could use only minutes if we wanted to.

fabpot added a commit that referenced this pull request Oct 1, 2025
@fabpot
minor #61859 [Intl] Support time in generated data in currencies vali…
d7186d2 
…dity (Crovitche-1623)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[Intl] Support time in generated data in currencies validity

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | -
| License       | MIT

`@nicolas`-grekas

see #61837 (comment)

Note: This PR is a prerequisite of #61837

Commits
-------

ed53287 [Intl] Support time in generated data in currencies validity
@fabpot fabpot force-pushed the currency-type-with-active-currencies branch from dcf0f08 to a378fe6 Compare October 1, 2025 06:11
@fabpot
Copy link
Member

fabpot commented Oct 1, 2025

Thank you @Crovitche-1623.

@fabpot fabpot merged commit c8e4bf8 into symfony:7.4 Oct 1, 2025
11 of 13 checks passed
@fabpot fabpot mentioned this pull request Oct 1, 2025
Closed
@xabbuh xabbuh mentioned this pull request Oct 2, 2025
Merged
nicolas-grekas added a commit that referenced this pull request Oct 2, 2025
@nicolas-grekas
minor #61920 [Form] do not install symfony/intl < 7.4 (xabbuh)
3dd5b6d 
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       | MIT

the changes done in #61837 require #61431

Commits
-------

badf463 do not install symfony/intl < 7.4
This was referenced Oct 27, 2025
Merged
Merged
@javiereguiluz javiereguiluz mentioned this pull request Oct 29, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@javiereguiluz javiereguiluz javiereguiluz left review comments

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

@fabpot fabpot fabpot approved these changes

@xabbuh xabbuh Awaiting requested review from xabbuh xabbuh is a code owner

@yceruto yceruto Awaiting requested review from yceruto yceruto is a code owner

@stof stof Awaiting requested review from stof

Assignees

No one assigned

Labels

Feature Form Status: Reviewed

Projects

None yet

Milestone

7.4

Development

Successfully merging this pull request may close these issues.

7 participants

@Crovitche-1623 @ro0NL @nicolas-grekas @fabpot @javiereguiluz @stof @carsonbot