[Form] Add calendar DateType option #53895

jan-pintr · 2024-02-10T14:35:58Z

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	Fix #53894
License	MIT

Recently I required to render DateType in format with week of year [1] (week 13 of year 2024: 2024-03-31 2024w13) with independent IntlCalendar specification than provided from currently set Locale.

For example when project (default) locale is en_US (en). But you need to output in "European's format" it is not possible with current implementation. You would get 2024-03-31 2024w14, because first day of week configuration is taken from currently set locale.

This pull request adds calendar option to DateType.

$calendar = \IntlCalendar::createInstance();
$calendar->setFirstDayOfWeek(\IntlCalendar::DOW_MONDAY);
$calendar->setMinimalDaysInFirstWeek(4);

$form->add('with_week', DateType::class, [
    'format' => "y-MM-dd y'w'w",
    'calendar' => $calendar,          
]);

[1] https://unicode.org/reports/tr35/tr35-dates.html#Date_Patterns_Week_Of_Year

carsonbot · 2024-02-10T14:36:03Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 7.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

OskarStark · 2024-02-11T07:45:40Z

src/Symfony/Component/Form/Tests/Extension/Core/Type/DateTypeTest.php

+        $form->submit('2024-03-31 2024w13');
+
+        $this->assertEquals($output, $form->getData());
+        $this->assertEquals('2024-03-31 2024w13', $form->getViewData());


Can we use asserSame ?

Same for the other tests

I have the change ready on local, but I'm not a big fan of it as other tests in changed files use assertEquals. Could somebody else give us an opinion?

opinion: never rely on assertEquals, same as never rely on == instead of ===

You can change it to assertSame across the whole file 👍

nicolas-grekas

Here are random comments :)

nicolas-grekas · 2024-02-14T16:16:36Z

src/Symfony/Component/Form/Extension/Core/Type/DateType.php

@@ -49,7 +49,7 @@ public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        $dateFormat = \is_int($options['format']) ? $options['format'] : self::DEFAULT_FORMAT;
        $timeFormat = \IntlDateFormatter::NONE;
-        $calendar = \IntlDateFormatter::GREGORIAN;
+        $calendar = null !== $options['calendar'] ? $options['calendar'] : \IntlDateFormatter::GREGORIAN;


Suggested change

$calendar = null !== $options['calendar'] ? $options['calendar'] : \IntlDateFormatter::GREGORIAN;

$calendar = $options['calendar'] ?? \IntlDateFormatter::GREGORIAN;

nicolas-grekas · 2024-02-14T16:18:22Z

...onent/Form/Tests/Extension/Core/DataTransformer/DateTimeToLocalizedStringTransformerTest.php

+            $pattern,
+        );
+
+        $this->assertEquals('2024-03-31 2024w13', $transformer->transform($dateTime));


assertSame everywhere please

nicolas-grekas · 2024-02-14T16:19:15Z

...onent/Form/Tests/Extension/Core/DataTransformer/DateTimeToLocalizedStringTransformerTest.php

+            null,
+            $calendar,
+            $pattern,
+        );


we prefer long lines, maybe using named args could help? our put this on one line otherwise

nicolas-grekas · 2024-02-14T16:19:23Z

...onent/Form/Tests/Extension/Core/DataTransformer/DateTimeToLocalizedStringTransformerTest.php

+
+        $pattern = "y-MM-dd y'w'w";
+
+        $transformer = new DateTimeToLocalizedStringTransformer(


oneliners FTW

nicolas-grekas · 2024-02-14T16:19:32Z

...onent/Form/Tests/Extension/Core/DataTransformer/DateTimeToLocalizedStringTransformerTest.php

+        $calendar->setFirstDayOfWeek(\IntlCalendar::DOW_MONDAY);
+        $calendar->setMinimalDaysInFirstWeek(4);
+
+        $transformer = new DateTimeToLocalizedStringTransformer(


nicolas-grekas · 2024-04-12T12:10:25Z

@jan-pintr are you interested in finishing this PR?

xabbuh · 2024-08-09T11:21:22Z

closing in favour of #57960, thank you for starting the work @jan-pintr

…ype` (alexandre-daubois) This PR was merged into the 7.2 branch. Discussion ---------- [Form] Add support for the `calendar` option in `DateType` | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | Fix #53894 | License | MIT 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. Commits ------- fca95c5 [Form] Add support for the `calendar` option in `DateType`

Add calendar DateType option

285f638

jan-pintr requested review from xabbuh and yceruto as code owners February 10, 2024 14:35

carsonbot added Status: Needs Review Feature labels Feb 10, 2024

carsonbot added this to the 7.1 milestone Feb 10, 2024

Jan Pintr added 2 commits February 10, 2024 15:48

fix coding style

252a4af

fix transformer test

6daff1c

OskarStark reviewed Feb 11, 2024

View reviewed changes

nicolas-grekas added the Form label Feb 14, 2024

carsonbot changed the title ~~Add calendar DateType option~~ [Form] Add calendar DateType option Feb 14, 2024

nicolas-grekas reviewed Feb 14, 2024

View reviewed changes

xabbuh modified the milestones: 7.1, 7.2 May 15, 2024

alexandre-daubois mentioned this pull request Aug 9, 2024

[Form] Add support for the calendar option in DateType #57960

Merged

xabbuh closed this Aug 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] Add calendar DateType option #53895

[Form] Add calendar DateType option #53895

jan-pintr commented Feb 10, 2024 •

edited by OskarStark

Loading

carsonbot commented Feb 10, 2024

OskarStark Feb 11, 2024

jan-pintr Feb 11, 2024

keradus Feb 12, 2024

OskarStark Feb 12, 2024

nicolas-grekas left a comment

nicolas-grekas Feb 14, 2024

nicolas-grekas Feb 14, 2024

nicolas-grekas Feb 14, 2024

nicolas-grekas Feb 14, 2024

nicolas-grekas Feb 14, 2024

nicolas-grekas commented Apr 12, 2024

xabbuh commented Aug 9, 2024

	$calendar = null !== $options['calendar'] ? $options['calendar'] : \IntlDateFormatter::GREGORIAN;
	$calendar = $options['calendar'] ?? \IntlDateFormatter::GREGORIAN;


		$pattern = "y-MM-dd y'w'w";

		$transformer = new DateTimeToLocalizedStringTransformer(

[Form] Add calendar DateType option #53895

[Form] Add calendar DateType option #53895

Conversation

jan-pintr commented Feb 10, 2024 • edited by OskarStark Loading

carsonbot commented Feb 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Apr 12, 2024

xabbuh commented Aug 9, 2024

jan-pintr commented Feb 10, 2024 •

edited by OskarStark

Loading