Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form] Fixed DateType format option for single text widget#21063
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
cc7a6b2 to6ce7601Compare| $this->factory->create('date',null,array( | ||
| 'months' =>array(6,7), | ||
| 'format' =>'yy', | ||
| 'format' =>'wrong', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You will now have to change the name of the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I didn't change it on purpose as this is aboutand butor was previously tested. Should I change it anyway?
xabbuh commentedDec 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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| $this->factory->create('date',null,array( | ||
| 'months' =>array(6,7), | ||
| 'format' =>'yy', | ||
| 'format' =>'wrong', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I didn't change it on purpose as this is aboutand butor was previously tested. Should I change it anyway?
yceruto commentedDec 27, 2016
@HeahDude I've tested your changes and it looks wrong for I think we need pass the right date fields to |
HeahDude commentedDec 27, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@yceruto AFAIK EDIT |
yceruto commentedDec 28, 2016
Do you plan to implement this one to all available widget here? It would be great to have custom/partial format for The |
yceruto commentedDec 28, 2016
If something helps you:yceruto@5e0d86c it is my quick proof to enable this feature on whole type. WDYT? |
fbb55e7 toa8f8fb2CompareHeahDude commentedDec 31, 2016
I've updated the PR to fix the bug when using the 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 commentedDec 31, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@HeahDude I do not understand why the pattern should not contain all the subpatterns when the |
HeahDude commentedDec 31, 2016
Because in this case the For other widgets each part has to be extracted from it (refhttps://github.com/symfony/symfony/tree/2.7/src/Symfony/Component/Form/Extension/Core/Type/DateType.php#L90 andhttps://github.com/symfony/symfony/tree/2.7/src/Symfony/Component/Form/Extension/Core/Type/DateType.php#L277). |
a8f8fb2 to9e0d531Comparexabbuh commentedJan 1, 2017
@HeahDude But then the stricter test is only needed when the |
HeahDude commentedJan 2, 2017
@xabbuh No it looks like it's not, the |
xabbuh commentedJan 3, 2017
@HeahDude Can you show me where this is done? |
| /** | ||
| * @expectedException \Symfony\Component\OptionsResolver\Exception\InvalidOptionsException | ||
| * @expectedExceptionMessage The "format" option should contain the letters "y", "M" or "d". Its current value is "wrong". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would include the fact that this is testing the specific behaviour for thesingle_text widget (something liketestThrowExceptionIfFormatMissesYearMonthAndDayWithSingleTextWidget()).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done inad8f189
xabbuh commentedFeb 3, 2017
👍 Status: Reviewed |
fabpot commentedFeb 4, 2017
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
It's currently not possible to use a custom format with
DateTypewhen 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.