[Form] Add support for the calendar option in DateType
#57960
Conversation
887fbc9 to
da570c0
Compare
|
@alexandre-daubois In #53894 I raised the question if we really need to be able to change the calendar. For the initial issue being able to control the locale was sufficient. Do you see other use cases where we have a need for being able to control the calendar? |
|
Yes. I'm not sure there's a need to switch between Gregorian and Julian calendars. But there's also other calendars that may be used like the Arabic Calendar for instance. I also think the test cases reflect (at least) why I would use this option the most: it is particularly useful when dealing with different first day of week, i.e. Sunday in the USA, Monday in Europe, etc. I think that if you couple this with #57908, this definitely makes even more sense. Overall, I think it brings more flexibility so anyone is up to use the calendar of their choice, without being limited by the locale. It doesn't bring that much complexity to the code. I think handling a locale instead of a calendar would require more boilerplate than there is now for less flexibility. |
|
About locales, it looks like they can embed the calendar? e.g. |
|
When using a locale like At the end of the day, it requires on more option to support and less flexibility than being able to pass an IntlCalendar directly. I think that the current solution is more flexible, as well as easier to understand with less boilerplate. |
da570c0 to
d855ab5
Compare
...mfony/Component/Form/Extension/Core/DataTransformer/DateTimeToLocalizedStringTransformer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Form/Tests/Extension/Core/Type/DateTypeTest.php
Outdated
Show resolved
Hide resolved
d855ab5 to
75dc4db
Compare
| 'input' => 'array', | ||
| 'calendar' => $calendar, | ||
| ]); | ||
| $form->submit('2024-03-31 2024w14'); |
There was a problem hiding this comment.
Should we add failing tests when submitting 2024-03-31 2024w13 with this calendar and 2024-03-31 2024w14 with the calendar configured on line 1179?
There was a problem hiding this comment.
I'm not sure, which case would you like to test exactly by doing this?
There was a problem hiding this comment.
I am not sure what would happen then. Would the form accept such an invalid behaviour? In the best case the transformer would issue a TransformationFailedException and we could test for the form being invalid.
There was a problem hiding this comment.
I had a bit of trouble with the weeks. I reworked the tests instead to use another calendar instead of playing on the edge of weeks (which also didn't seem to be really supported by this formtype). What do you think of the new version?
| @@ -320,6 +321,7 @@ public function configureOptions(OptionsResolver $resolver): void | |||
| $resolver->setAllowedTypes('months', 'array'); | |||
| $resolver->setAllowedTypes('days', 'array'); | |||
| $resolver->setAllowedTypes('input_format', 'string'); | |||
| $resolver->setAllowedTypes('calendar', ['null', \IntlCalendar::class]); | |||
There was a problem hiding this comment.
Could you also add some brief info about this new option? $resolver->setInfo('calendar', '...')
There was a problem hiding this comment.
Added! Is the message good to you?
...onent/Form/Tests/Extension/Core/DataTransformer/DateTimeToLocalizedStringTransformerTest.php
Outdated
Show resolved
Hide resolved
...onent/Form/Tests/Extension/Core/DataTransformer/DateTimeToLocalizedStringTransformerTest.php
Outdated
Show resolved
Hide resolved
75dc4db to
855ed6d
Compare
| 'format' => 'y-MM-dd', | ||
| 'html5' => false, | ||
| 'input' => 'array', | ||
| 'calendar' => \IntlCalendar::createInstance(locale: 'zh_TW@calendar=roc'), |
There was a problem hiding this comment.
IMO the use of this calendar deserves some kind of comment. I believe it's not so obvious when reading the test why we submit a year like 113 but expect 2024 as the final result.
There was a problem hiding this comment.
I added a comment. Is it clearer to you now?
855ed6d to
fca95c5
Compare
|
Thank you @alexandre-daubois. |
| @@ -320,6 +321,9 @@ public function configureOptions(OptionsResolver $resolver): void | |||
| $resolver->setAllowedTypes('months', 'array'); | |||
| $resolver->setAllowedTypes('days', 'array'); | |||
| $resolver->setAllowedTypes('input_format', 'string'); | |||
| $resolver->setAllowedTypes('calendar', ['null', \IntlCalendar::class]); | |||
|
|
|||
| $resolver->setInfo('calendar', 'The calendar to use for formatting and parsing the date. The value should be one of the \IntlDateFormatter calendar constants or an instance of the \IntlCalendar to use.'); | |||
There was a problem hiding this comment.
This information looks wrong. The allowed types don't allow the integer constants
There was a problem hiding this comment.
I'm having a look. Thanks for raising this issue!
…e` (alexandre-daubois) This PR was merged into the 7.2 branch. Discussion ---------- [Form] Allow integer for the `calendar` option of `DateType` | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT Underlying mechanisms support integers for the calendar, so integers should be allowed in the form type option. Spotted by `@stof` in #57960 (comment) Commits ------- 5e75bed [Form] Allow integer for the `calendar` option of `DateType`
Supersedes #53895 which seems abandoned for 6 months. I genuinely think this feature worth it especially when dealing with calendars with different first day of week.