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] Support DateTimeImmutable in transform() #14895
Conversation
@@ -72,14 +72,15 @@ public function transform($dateTime) | |||
), array_flip($this->fields)); | |||
} | |||
|
|||
if (!$dateTime instanceof \DateTime) { | |||
throw new TransformationFailedException('Expected a \DateTime.'); | |||
// DateTimeInterface was introduced in PHP 5.5.0. |
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.
no need to write this everywhere
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.
@Tobion The comment explains why we have to check for both DateTime and DateTimeInterface. DateTime implements DateTimeInterface, so one would think that checking for DateTimeInterface is enough. Don't you think this deserves a comment? Or are you just suggesting that I only add it in one (which?) of the five classes?
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.
it doesn't deserve a comment because we already have this logic in many other places without a comment.
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. Fixed.
bc5ef34
to
2d1b43b
Compare
} | ||
|
||
$dateTime = clone $dateTime; | ||
if ($this->inputTimezone !== $this->outputTimezone) { | ||
try { | ||
$dateTime->setTimezone(new \DateTimeZone($this->outputTimezone)); | ||
$dateTime = $dateTime->setTimezone(new \DateTimeZone($this->outputTimezone)); |
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.
Isn't it a BC break?
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.
no, it just ensures that immutable datetime works the same as the mutable one (i.e. we want to explicitly set the timezone).
2d1b43b
to
17346c5
Compare
👍 |
Thank you @c960657. |
This PR was merged into the 2.3 branch. Discussion ---------- [Form] Support DateTimeImmutable in transform() | Q | A | ------------- | --- | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | When passing a DateTimeImmutable instance to DateTimeToLocalizedStringTransformer::transform($dateTime), it throws an exception, `TransformationFailedException('Expected a \DateTime.')`. The method just converts a date-time object into a string, so there is no reason that it should not support all DateTimeInterface implementations. DateTimeInterface was added in PHP 5.5, so in order to support earlier versions, we need to do instanceof checks for both DateTime and DateTimeInterface. When Symfony requires PHP 5.5 or larger, we can remove the DateTime check and only check for DateTimeInterface. This was originally submitted as a PR against the 2.7 branch in #14676. Commits ------- 17346c5 [Form] Support DateTimeImmutable in transform()
When passing a DateTimeImmutable instance to DateTimeToLocalizedStringTransformer::transform($dateTime), it throws an exception,
TransformationFailedException('Expected a \DateTime.')
.The method just converts a date-time object into a string, so there is no reason that it should not support all DateTimeInterface implementations.
DateTimeInterface was added in PHP 5.5, so in order to support earlier versions, we need to do instanceof checks for both DateTime and DateTimeInterface. When Symfony requires PHP 5.5 or larger, we can remove the DateTime check and only check for DateTimeInterface.
This was originally submitted as a PR against the 2.7 branch in #14676.