-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] Add support for DateTimeImmutable #19889
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Form] Add support for DateTimeImmutable #19889
Conversation
namespace Symfony\Component\Form\Extension\Core\DataTransformer; | ||
|
||
/** | ||
* @package Symfony\Component\Form\Extension\Core\DataTransformer |
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.
We don't use @package
in symfony core
Sorry, something went wrong.
All reactions
Much better :) |
All reactions
Sorry, something went wrong.
I would like to have an option to use custom DateTime class (extending |
All reactions
Sorry, something went wrong.
@@ -108,7 +109,7 @@ public function transform($dateTime) | |||
* | |||
* @param array $value Localized date | |||
* | |||
* @return \DateTime Normalized date | |||
* @return \DateTimeInterface Normalized date |
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.
the hint should be explicit here IMO: @return \DateTime|DateTimeImmutable
since no other implementation is used.
Sorry, something went wrong.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describ 8000 e this comment to others. Learn more.
I think leaving it as \DateTimeInterface
is still semantically correct, and is more concise
Sorry, something went wrong.
All reactions
@gharlan I think this is a topic for another PR. |
All reactions
Sorry, something went wrong.
4062815
to
8d60d72
Compare
@fabpot @webmozart what are the chances of this making it into 3.2 before the end of the development phase? |
All reactions
Sorry, something went 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.
The tests are failing because of issues in SecurityBundle or the VarDumper Component, and a (purposefully) malformed ini for hhvm.
If you could prepare the docs PR, I think this is quite ready to be merged.
Sorry, something went wrong.
All reactions
I'm away for the next few days so I'll work on the docs PR when I return. |
All reactions
Sorry, something went wrong.
@jameshalsall I hope you returned 😆 |
All reactions
Sorry, something went wrong.
@kaiwa Haha, I completely forgot about this - I'll add it to my TODO list to create the docs PR |
All reactions
Sorry, something went wrong.
This one needs a rebase :) |
All reactions
Sorry, something went wrong.
I would not merge this one as is, in favor of more work to address the last (and most important IMHO) part of the related issue:
|
All reactions
Sorry, something went wrong.
Makes sense. So, I'm closing this one as Symfony 3 does require PHP 5.5+. |
All reactions
Sorry, something went wrong.
I have an idea of what to do for this, but some other issues must be addressed first. |
All reactions
Sorry, something went wrong.
In the mean time, it's really easy to implement a model transformer in user land if needed. |
All reactions
Sorry, something went wrong.
Wish you'd have said that back in September ;) |
All reactions
Sorry, something went wrong.
Successfully merging this pull request may close these issues.
This PR is an initial concept for adding support for
\DateTimeImmutable
in the Form component.The
DateType
,TimeType
andDateTimeType
now support a new "input" option called "datetimeimmutable". When this is set the returned values from$form->getData()
will be instances of\DateTimeImmutable
instead of\DateTime
(which is the case for an "input" value of "datetime" - the default).The existing
DateTime
transformers now take an additional constructor argument indicating whether theDateTime
object is immutable, affecting the reverse-transformed valueI'm looking for feedback on this approach before I finalise it.