8000 [Form] minor fixes in DateTime transformers by HeahDude · Pull Request #18548 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] minor fixes in DateTime transformers #18548

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[Form] minor fixes in DateTime transformers #18548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions src/Symfony/Component/Form/Extension/Core/DataTransformer/DateTimeToArrayTransformer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ public function __construct($inputTimezone = null, $outputTimezone = null, array
* @return array Localized date.
*
* @throws TransformationFailedException If the given value is not an
* instance of \DateTime or if the
* output timezone is not supported.
* instance of \DateTime or \DateTimeInterface
*/
public function transform($dateTime)
{
Expand All @@ -81,11 +80,7 @@ public function transform($dateTime)
$dateTime = clone $dateTime;
}

try {
$dateTime = $dateTime->setTimezone(new \DateTimeZone($this->outputTimezone));
} catch (\Exception $e) {
throw new TransformationFailedException($e->getMessage(), $e->getCode(), $e);
}
$dateTime = $dateTime->setTimezone(new \DateTimeZone($this->outputTimezone));
}

$result = array_intersect_key(array(
Expand Down Expand Up @@ -118,8 +113,6 @@ public function transform($dateTime)
*
* @throws TransformationFailedException If the given value is not an array,
* if the value could not be transformed
* or if the input timezone is not
* supported.
*/
public function reverseTransform($value)
{
Expand Down
13 changes: 4 additions & 9 deletions ...ny/Component/Form/Extension/Core/DataTransformer/DateTimeToLocalizedStringTransformer.php
View file
Open in desktop
8000
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,8 @@ public function __construct($inputTimezone = null, $outputTimezone = null, $date
* @return string|array Localized date string/array.
*
* @throws TransformationFailedException If the given value is not an instance
* of \DateTime or if the date could not
* be transformed.
* of \DateTime or \DateTimeInterface or
* if the date could not be transformed.
*/
public function transform($dateTime)
{
Expand Down Expand Up @@ -105,8 +105,7 @@ public function transform($dateTime)
* @return \DateTime Normalized date
*
* @throws TransformationFailedException if the given value is not a string,
* if the date could not be parsed or
* if the input timezone is not supported
* if the date could not be parsed
*/
public function reverseTransform($value)
{
Expand All @@ -132,11 +131,7 @@ public function reverseTransform($value)
}

if ('UTC' !== $this->inputTimezone) {
try {
$dateTime->setTimezone(new \DateTimeZone($this->inputTimezone));
} catch (\Exception $e) {
throw new TransformationFailedException($e->getMessage(), $e->getCode(), $e);
}
$dateTime->setTimezone(new \DateTimeZone($this->inputTimezone));
}

return $dateTime;
Expand Down
31 changes: 22 additions & 9 deletions src/Symfony/Component/Form/Extension/Core/DataTransformer/DateTimeToRfc3339Transformer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,14 @@
class DateTimeToRfc3339Transformer extends BaseDateTimeTransformer
{
/**
* {@inheritdoc}
* Transforms a normalized date into a localized date.
*
* @param \DateTime|\DateTimeInterface $dateTime A DateTime object
*
* @return string The formatted date.
*
* @throws TransformationFailedException If the given value is not an
* instance of \DateTime or \DateTimeInterface
*/
public function transform($dateTime)
{
Expand All @@ -32,15 +39,25 @@ public function transform($dateTime)
}

if ($this->inputTimezone !== $this->outputTimezone) {
$dateTime = clone $dateTime;
if (!$dateTime instanceof \DateTimeImmutable) {
$dateTime = clone $dateTime;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should always clone to not modify the input data, shouldn't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xabbuh, I've no strong opinion on this. I just did it because I noticed DateTimeImmutable was not cloned in other transformers needing to clone the value, ref DateTimeToArrayTransformer and DateTimeToStringTransformer.

Should we change this behavior there too ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is that the DataTransformerInterface just states that the given input should be transformed and then being returned. To me this does not include that the input is allowed to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes even more sense when we consider that a DateTime object will be returned by reverseTransform() anyway, and actually it sets the output timezone on the input data...

So I guess I should change it in all three, thanks @xabbuh for pointing this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xabbuh technically, nothing was changing the input in the proposal. A DateTimeImmutable is immutable.


$dateTime = $dateTime->setTimezone(new \DateTimeZone($this->outputTimezone));
}

return preg_replace('/\+00:00$/', 'Z', $dateTime->format('c'));
}

/**
* {@inheritdoc}
* Transforms a formatted string following RFC 3339 into a normalized date.
*
* @param string $rfc3339 Formatted string
*
* @return \DateTime Normalized date
*
* @throws TransformationFailedException If the given value is not a string,
* if the value could not be transformed
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if condition looks wrong, shouldn't it test the input's timezone ?

public function reverseTransform($rfc3339)
{
Expand All @@ -58,12 +75,8 @@ public function reverseTransform($rfc3339)
throw new TransformationFailedException($e->getMessage(), $e->getCode(), $e);
}

if ($this->outputTimezone !== $dateTime->getTimezone()->getName()) {
try {
$dateTime->setTimezone(new \DateTimeZone($this->inputTimezone));
} catch (\Exception $e) {
throw new TransformationFailedException($e->getMessage(), $e->getCode(), $e);
}
if ($this->inputTimezone !== $dateTime->getTimezone()->getName()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any opinion on this condition? ping @stof @xabbuh

$dateTime->setTimezone(new \DateTimeZone($this->inputTimezone));
}

if (preg_match('/(\d{4})-(\d{2})-(\d{2})/', $rfc3339, $matches)) {
Expand Down
28 changes: 11 additions & 17 deletions src/Symfony/Component/Form/Extension/Core/DataTransformer/DateTimeToStringTransformer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,35 +90,30 @@ public function __construct($inputTimezone = null, $outputTimezone = null, $form
* Transforms a DateTime object into a date string with the configured format
* and timezone.
*
* @param \DateTime|\DateTimeInterface $value A DateTime object
* @param \DateTime|\DateTimeInterface $dateTime A DateTime object
*
* @return string A value as produced by PHP's date() function
*
* @throws TransformationFailedException If the given value is not a \DateTime
* instance or if the output timezone
* is not supported.
* @throws TransformationFailedException If the given value is not an
* instance of \DateTime or \DateTimeInterface
*/
public function transform($value)
public function transform($dateTime)
{
if (null === $value) {
if (null === $dateTime) {
return '';
}

if (!$value instanceof \DateTime && !$value instanceof \DateTimeInterface) {
if (!$dateTime instanceof \DateTime && !$dateTime instanceof \DateTimeInterface) {
throw new TransformationFailedException('Expected a \DateTime or \DateTimeInterface.');
}

if (!$value instanceof \DateTimeImmutable) {
$value = clone $value;
if (!$dateTime instanceof \DateTimeImmutable) {
$dateTime = clone $dateTime;
}

try {
$value = $value->setTimezone(new \DateTimeZone($this->outputTimezone));
} catch (\Exception $e) {
throw new TransformationFailedException($e->getMessage(), $e->getCode(), $e);
}
$dateTime = $dateTime->setTimezone(new \DateTimeZone($this->outputTimezone));

return $value->format($this->generateFormat);
return $dateTime->format($this->generateFormat);
}

/**
Expand All @@ -129,8 +124,7 @@ public function transform($value)
* @return \DateTime An instance of \DateTime
*
* @throws TransformationFailedException If the given value is not a string,
* if the date could not be parsed or
* if the input timezone is not supported.
* or could not be transformed
*/
public function reverseTransform($value)
{
Expand Down
13 changes: 6 additions & 7 deletions src/Symfony/Component/Form/Extension/Core/DataTransformer/DateTimeToTimestampTransformer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,20 +29,19 @@ class DateTimeToTimestampTransformer extends BaseDateTimeTransformer
* @return int A timestamp
*
* @throws TransformationFailedException If the given value is not an instance
* of \DateTime or if the output
* timezone is not supported.
* of \DateTime or \DateTimeInterface
*/
public function transform($value)
public function transform($dateTime)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 for this renaming. It will make it harder to merge branches together for no real benefit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be fixed in a way to match the current name in the doc block, otherwise the doc block name should be changed. I did it so it's consistent with all the other DateTime transformers.

I don't understand why do you say it makes it harder to merge if it can be applied in all current maintained branches ?

{
if (null === $value) {
if (null === $dateTime) {
return;
}

if (!$value instanceof \DateTime && !$value instanceof \DateTimeInterface) {
if (!$dateTime instanceof \DateTime && !$dateTime instanceof \DateTimeInterface) {
throw new TransformationFailedException('Expected a \DateTime or \DateTimeInterface.');
}

return $value->getTimestamp();
return $dateTime->getTimestamp();
}

/**
Expand All @@ -53,7 +52,7 @@ public function transform($value)
* @return \DateTime A \DateTime object
*
* @throws TransformationFailedException If the given value is not a timestamp
* or if the given timestamp is invalid.
* or if the given timestamp is invalid
*/
public function reverseTransform($value)
{
Expand Down
0