Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
HeahDude commentedApr 14, 2016
| 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 | ~ |
| * of \DateTime. | ||
| */ | ||
| publicfunctiontransform($value) | ||
| publicfunctiontransform($dateTime) |
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.
-1 for this renaming. It will make it harder to merge branches together for no real benefit
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.
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 ?
xabbuh commentedApr 14, 2016
Can you add a new test to prevent regressions? |
HeahDude commentedApr 14, 2016
@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 ? |
HeahDude commentedApr 14, 2016
After a deeper look into this, I don't understand why this |
HeahDude commentedApr 14, 2016
| thrownewTransformationFailedException($e->getMessage(),$e->getCode(),$e); | ||
| } | ||
| if ($this->outputTimezone !==$dateTime->getTimezone()->getName()) { |
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.
Thisif condition looks wrong, shouldn't it test the input's timezone ?
| $dateTime =clone$dateTime; | ||
| if (!$dateTimeinstanceof \DateTimeImmutable) { | ||
| $dateTime =clone$dateTime; | ||
| } |
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 should always clone to not modify the input data, shouldn't we?
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.
@xabbuh, I've no strong opinion on this. I just did it because I noticedDateTimeImmutable was not cloned in other transformers needing to clone the value, refDateTimeToArrayTransformer andDateTimeToStringTransformer.
Should we change this behavior there too ?
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 point is that theDataTransformerInterface 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.
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 aDateTime object will be returned byreverseTransform() 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.
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.
HeahDude commentedApr 19, 2016
Ok updated afterStof's comment. If you agree with thiscondition change, this one should be finished. Thanks for the reviews. |
fabpot commentedJun 15, 2016
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