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][FrameworkBundle][Bridge] Add a DateInterval form type#15030
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
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.
is the header copied from other files?
i think you should be named here@MisatoTremor
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.
Yes, this is the standard copyright header.
I've put my name at the class author PHPDoc as usual. ;)
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.
ok, i didn't know.
thank you!
jewome62 commentedOct 5, 2015
🆒 !!! When that will be integrated into the 2.8? |
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.
IMO, this if/else should be outside the try/catch, avoiding the need to catch TransformationFailedException as is (and it should use an early return)
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.
Good catch, makes absolute sense to put it outside.
But is it really OK to just return null if the value is something the transformer does not expect?
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.
@stof
Just saw again that this throw > catch > throw pattern is also used in DateTimeToStringTransformer. Most probably I got it originally from there.
So maybe both should be changed (DateTimeToStringTransformer in another PR then) or just be left as is.
What do you think?
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.
Simplifying the DateTimeToStringTransformer in another PR makes sense yeah. but it should indeed be a separate PR
b7498b3 to8045bc5CompareAlso add dateinterval widget to twig templates.
0d8f42e tof235fefCompareMisatoTremor commentedOct 14, 2015
@stof Ok, simplified the transformer and also moved the format check out of the try/catch as this also was a throw > catch > throw. AppVeyor build failure seems because of something else, as it also fails for the same tests in other PRs, etc. and Form component test passes. |
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.
This can be made static as it actually never changes.
…rvalToArrayTransformer static
MisatoTremor commentedDec 2, 2015
Rebased on master |
…m type (MisatoTremor)This PR was merged into the 3.2-dev branch.Discussion----------[Form][FrameworkBundle][Bridge] Add a DateInterval form type| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#13389| License | MIT| Doc PR |symfony/symfony-docs#4817Replaces#15030Commits-------f7669be [Form] Add a DateInterval form type Also add dateinterval widget to twig templates.
Replaces#13390