[Form][FrameworkBundle][Bridge] Add a DateInterval form type #15030
Conversation
There was a problem hiding this comment.
is the header copied from other files?
i think you should be named here @MisatoTremor
There was a problem hiding this comment.
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.
ok, i didn't know.
thank you!
|
🆒 !!! When that will be integrated into the 2.8? |
There was a problem hiding this comment.
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.
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.
@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.
Simplifying the DateTimeToStringTransformer in another PR makes sense yeah. but it should indeed be a separate PR
MisatoTremor
force-pushed
the
add_dateinterval_type_2_8
branch
from
b7498b3 to
8045bc5
Compare
Also add dateinterval widget to twig templates.
MisatoTremor
force-pushed
the
add_dateinterval_type_2_8
branch
from
0d8f42e to
f235fef
Compare
|
@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.
This can be made static as it actually never changes.
…rvalToArrayTransformer static
|
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#4817 Replaces #15030 Commits ------- f7669be [Form] Add a DateInterval form type Also add dateinterval widget to twig templates.
6 participants
Replaces #13390