[Form] Fixed DateType format option for single text widget #21063

HeahDude · 2016-12-27T14:50:27Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	~
License	MIT
Doc PR	~

It's currently not possible to use a custom format with DateType when not using one of the three values day, month or year (i.e in my case "MM/yyyy").

The formatter handles it, it looks like this option check is wrong, this PR fixes it.

xabbuh · 2016-12-27T18:08:13Z

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

@@ -342,7 +358,7 @@ public function testThrowExceptionIfFormatDoesNotContainYearMonthAndDay()
    {
        $this->factory->create('date', null, array(
            'months' => array(6, 7),
-            'format' => 'yy',
+            'format' => 'wrong',


You will now have to change the name of the method.

I didn't change it on purpose as this is about and but or was previously tested. Should I change it anyway?

xabbuh · 2016-12-27T18:09:07Z

But what is the expected value for the omitted parts of the date?

HeahDude · 2016-12-27T21:43:54Z

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

+
+        $form->submit('2010');
+
+        $this->assertDateTimeEquals(new \DateTime('2010-01-01 UTC'), $form->getData());


@xabbuh The expected value for day and month is tested here with first of each. For the year I'd say that the default is the current but I can add a test for it.

I am not convinced that this really is a bug fix. IMO being able to omit parts of a date is a new feature and you should then also be able to configure the values of the omitted parts.

HeahDude · 2016-12-27T21:44:40Z

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

@@ -342,7 +358,7 @@ public function testThrowExceptionIfFormatDoesNotContainYearMonthAndDay()
    {
        $this->factory->create('date', null, array(
            'months' => array(6, 7),
-            'format' => 'yy',
+            'format' => 'wrong',


I didn't change it on purpose as this is about and but or was previously tested. Should I change it anyway?

yceruto · 2016-12-27T22:08:44Z

@HeahDude I've tested your changes and it looks wrong for 'format' => 'MM/yyyy' and 'widget' => 'choice'. It display two choice inputs (Month and Year) correctly, but on submit shows:

This value is not valid.

I think we need pass the right date fields to DateTimeToArrayTransformer (3rd param) when some field was ignored in format option (e.g. For 'format' => 'MM/yyyy' should we pass array('year', 'month') only), otherwise it expects the three date fields always.

HeahDude · 2016-12-27T23:16:20Z

@yceruto AFAIK format option is meant to be used with the single_text widget since it is used to format the one field text only. This is also documented this way http://symfony.com/doc/current/reference/forms/types/date.html#format.

EDIT
Ok but then we don't have the proper exception, I'm gonna move this throwing in the if/else below to keep both checks, thanks!

yceruto · 2016-12-28T02:49:14Z

Ok but then we don't have the proper exception, I'm gonna move this throwing in the if/else below to keep both checks.

Do you plan to implement this one to all available widget here? It would be great to have custom/partial format for choice widget too ;)

The format option (later $pattern) affects both widgets, and the choice widget wasn't ready to accept custom/partial format. That's why this seems more as a new feature rather than a bug (IMHO).

yceruto · 2016-12-28T14:19:49Z

If something helps you: yceruto@5e0d86c it is my quick proof to enable this feature on whole type. WDYT?

HeahDude · 2016-12-31T14:39:27Z

I've updated the PR to fix the bug when using the single_text widget only, because IMHO it should only work as is, the only thing preventing it is a wrong configuration exception preventing to build the form.

If we want to handle this for other widgets that would imply a new way to configure the type so yeah I agree this is a new feature, @yceruto feel free to open a PR then :)

xabbuh · 2016-12-31T14:52:50Z

@HeahDude I do not understand why the pattern should not contain all the subpatterns when the single_text widget is used. What is so different compared to other widgets?

HeahDude · 2016-12-31T15:46:36Z

why the pattern should not contain all there subpatterns when the single_text widget is used

Because in this case the pattern get from here is simply used to format the date thanks to the transformer.

For other widgets each part has to be extracted from it (ref https://github.com/symfony/symfony/tree/2.7/src/Symfony/Component/Form/Extension/Core/Type/DateType.php#L90 and https://github.com/symfony/symfony/tree/2.7/src/Symfony/Component/Form/Extension/Core/Type/DateType.php#L277).

xabbuh · 2017-01-01T23:16:31Z

@HeahDude But then the stricter test is only needed when the widget option is choice, right?

HeahDude · 2017-01-02T10:58:29Z

@xabbuh No it looks like it's not, the text widget also uses the DateTimeToArrayTransformer which fails to transform the submitted value on missing parts.

xabbuh · 2017-01-03T17:43:51Z

@HeahDude Can you show me where this is done?

HeahDude · 2017-01-03T17:46:44Z

@xabbuh https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Core/DataTransformer/DateTimeToArrayTransformer.php#L140

xabbuh · 2017-02-03T14:20:16Z

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

@@ -348,6 +365,18 @@ public function testThrowExceptionIfFormatDoesNotContainYearMonthAndDay()

    /**
     * @expectedException \Symfony\Component\OptionsResolver\Exception\InvalidOptionsException
+     * @expectedExceptionMessage The "format" option should contain the letters "y", "M" or "d". Its current value is "wrong".
+     */
+    public function testThrowExceptionIfFormatDoesNotContainYearMonthOrDay()


I would include the fact that this is testing the specific behaviour for the single_text widget (something like testThrowExceptionIfFormatMissesYearMonthAndDayWithSingleTextWidget()).

done in ad8f189

xabbuh · 2017-02-03T14:20:31Z

👍

Status: Reviewed

fabpot · 2017-02-04T16:35:58Z

Thank you @HeahDude.

… (HeahDude) This PR was merged into the 2.7 branch. Discussion ---------- [Form] Fixed DateType format option for single text widget | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ It's currently not possible to use a custom format with `DateType` when not using one of the three values day, month or year (i.e in my case "MM/yyyy"). The formatter handles it, it looks like this option check is wrong, this PR fixes it. Commits ------- 9e0d531 [Form] Fixed DateType format option

carsonbot added Status: Needs Review Form Bug labels Dec 27, 2016

HeahDude force-pushed the fix/datetime_type-format branch from cc7a6b2 to 6ce7601 Compare December 27, 2016 14:57

xabbuh reviewed Dec 27, 2016

View reviewed changes

HeahDude commented Dec 27, 2016

View reviewed changes

HeahDude force-pushed the fix/datetime_type-format branch 2 times, most recently from fbb55e7 to a8f8fb2 Compare December 31, 2016 14:38

HeahDude changed the title ~~[Form] Fixed DateType format option~~ [Form] Fixed DateType format option for single text widget Dec 31, 2016

[Form] Fixed DateType format option

9e0d531

HeahDude force-pushed the fix/datetime_type-format branch from a8f8fb2 to 9e0d531 Compare December 31, 2016 15:53

nicolas-grekas added this to the 2.7 milestone Jan 2, 2017

xabbuh reviewed Feb 3, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 3, 2017

fabpot merged commit 9e0d531 into symfony:2.7 Feb 4, 2017

HeahDude deleted the fix/datetime_type-format branch February 4, 2017 17:03

This was referenced Feb 6, 2017

Release v2.7.24 #21541

Merged

Release v2.8.17 #21543

Merged

Release v3.2.3 #21544

Merged

yceruto mentioned this pull request Jun 5, 2017

[Form] DateType widget choice - always render months as numbers #23064

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] Fixed DateType format option for single text widget #21063

[Form] Fixed DateType format option for single text widget #21063

HeahDude commented Dec 27, 2016

xabbuh Dec 27, 2016

HeahDude Dec 27, 2016

xabbuh commented Dec 27, 2016

HeahDude Dec 27, 2016

xabbuh Dec 28, 2016

HeahDude Dec 27, 2016

yceruto commented Dec 27, 2016

HeahDude commented Dec 27, 2016 •

edited

yceruto commented Dec 28, 2016

yceruto commented Dec 28, 2016

HeahDude commented Dec 31, 2016

xabbuh commented Dec 31, 2016 •

edited

HeahDude commented Dec 31, 2016

xabbuh commented Jan 1, 2017

HeahDude commented Jan 2, 2017

xabbuh commented Jan 3, 2017

HeahDude commented Jan 3, 2017

xabbuh Feb 3, 2017

fabpot Feb 4, 2017

xabbuh commented Feb 3, 2017

fabpot commented Feb 4, 2017


		$form->submit('2010');

		$this->assertDateTimeEquals(new \DateTime('2010-01-01 UTC'), $form->getData());

[Form] Fixed DateType format option for single text widget #21063

[Form] Fixed DateType format option for single text widget #21063

Conversation

HeahDude commented Dec 27, 2016

xabbuh Dec 27, 2016

Choose a reason for hiding this comment

HeahDude Dec 27, 2016

Choose a reason for hiding this comment

xabbuh commented Dec 27, 2016

HeahDude Dec 27, 2016

Choose a reason for hiding this comment

xabbuh Dec 28, 2016

Choose a reason for hiding this comment

HeahDude Dec 27, 2016

Choose a reason for hiding this comment

yceruto commented Dec 27, 2016

HeahDude commented Dec 27, 2016 • edited

yceruto commented Dec 28, 2016

yceruto commented Dec 28, 2016

HeahDude commented Dec 31, 2016

xabbuh commented Dec 31, 2016 • edited

HeahDude commented Dec 31, 2016

xabbuh commented Jan 1, 2017

HeahDude commented Jan 2, 2017

xabbuh commented Jan 3, 2017

HeahDude commented Jan 3, 2017

xabbuh Feb 3, 2017

Choose a reason for hiding this comment

fabpot Feb 4, 2017

Choose a reason for hiding this comment

xabbuh commented Feb 3, 2017

fabpot commented Feb 4, 2017

HeahDude commented Dec 27, 2016 •

edited

xabbuh commented Dec 31, 2016 •

edited