[Form] minor fixes in DateTime transformers #18548
Conversation
| Q | A |
|---|---|
| Branch? | 2.3+ |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | ~ |
| License | MIT |
| Doc PR | ~ |
| */ | ||
| public function transform($value) | ||
| public function transform($dateTime) |
There was a problem hiding this comment.
-1 for this renaming. It will make it harder to merge branches together for no real benefit
There was a problem hiding this comment.
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 ?
|
Can you add a new test to prevent regressions? |
|
@xabbuh I'd like to, but there is no test for this exception in other transformers. I don't know how to make it fail. Any ideas ? |
|
After a deeper look into this, I don't understand why this |
7f3c1f1 to
24fd49f
Compare
| @@ -58,12 +75,8 @@ public function reverseTransform($rfc3339) | |||
| throw new TransformationFailedException($e->getMessage(), $e->getCode(), $e); | |||
| } | |||
|
|
|||
| if ($this->outputTimezone !== $dateTime->getTimezone()->getName()) { | |||
There was a problem hiding this comment.
This if condition looks wrong, shouldn't it test the input's timezone ?
| $dateTime = clone $dateTime; | ||
| if (!$dateTime instanceof \DateTimeImmutable) { | ||
| $dateTime = clone $dateTime; | ||
| } |
There was a problem hiding this comment.
We should always clone to not modify the input data, shouldn't we?
There was a problem hiding this comment.
@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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@xabbuh technically, nothing was changing the input in the proposal. A DateTimeImmutable is immutable.
24fd49f to
a35c28c
Compare
a35c28c to
d82c804
Compare
|
Ok updated after Stof's comment. If you agree with this condition change, this one should be finished. Thanks for the reviews. |
|
Thank you @HeahDude. |
This PR was submitted for the 2.3 branch but it was merged into the 2.7 branch instead (closes #18548). Discussion ---------- [Form] minor fixes in DateTime transformers | Q | A | ------------- | --- | Branch? | 2.3+ | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ Commits ------- b91008f [Form] fixed DateTime transformers