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

Merged
merged 1 commit into from Feb 4, 2017

Projects

None yet

6 participants

@HeahDude
Contributor
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.

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

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

@HeahDude
HeahDude Dec 27, 2016 Contributor

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

@xabbuh
Member
xabbuh commented Dec 27, 2016

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

+
+ $form->submit('2010');
+
+ $this->assertDateTimeEquals(new \DateTime('2010-01-01 UTC'), $form->getData());
@HeahDude
HeahDude Dec 27, 2016 Contributor

@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.

@xabbuh
xabbuh Dec 28, 2016 Member

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.

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

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

@yceruto
Contributor
yceruto commented Dec 27, 2016

@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
Contributor
HeahDude commented Dec 27, 2016 edited

@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
Contributor
yceruto commented Dec 28, 2016

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
Contributor
yceruto commented Dec 28, 2016

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

@HeahDude
Contributor

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 :)

@HeahDude HeahDude changed the title from [Form] Fixed DateType format option to [Form] Fixed DateType format option for single text widget Dec 31, 2016
@xabbuh
Member
xabbuh commented Dec 31, 2016 edited

@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
Contributor

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).

@HeahDude HeahDude [Form] Fixed DateType format option
9e0d531
@xabbuh
Member
xabbuh commented Jan 1, 2017

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

@HeahDude
Contributor
HeahDude commented Jan 2, 2017

@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.

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Jan 2, 2017
@xabbuh
Member
xabbuh commented Jan 3, 2017

@HeahDude Can you show me where this is done?

@@ -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()
@xabbuh
xabbuh Feb 3, 2017 Member

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

@xabbuh
Member
xabbuh commented Feb 3, 2017

๐Ÿ‘

Status: Reviewed

@fabpot
Member
fabpot commented Feb 4, 2017

Thank you @HeahDude.

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@fabpot fabpot added a commit that referenced this pull request Feb 4, 2017
@fabpot fabpot bug #21063 [Form] Fixed DateType format option for single text widgetโ€ฆ
โ€ฆ (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
1d1f166
@HeahDude HeahDude deleted the HeahDude:fix/datetime_type-format branch Feb 4, 2017
This was referenced Feb 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment