Fixed DateTimeZone support in date filter. #736
Conversation
| if (!$timezone instanceof DateTimeZone) { | ||
| $timezone = new DateTimeZone($timezone); | ||
| if ($timezone instanceof DateTimeZone) { | ||
| $date->setTimezone($timezone); |
There was a problem hiding this comment.
This will have a side-effect when passing an existing DateTime object as it will change the timezone of the existing objects. Unfortunately, PHP DateTime objects are not immutable. In case of a DateTime object passed by the user (the case which was returning early previously), you need to clone the object
There was a problem hiding this comment.
You're absolutely right. I've fixed this below.
|
Can you add some unit tests to avoid regression to happen later? Thanks. |
|
@fabpot Here they are. |
|
Great! Can you add a note in the CHANGELOG file and then squash your commit before I merge? Thanks. |
|
Changelog added. First squash of my life, i cannot say if it's good :( |
|
it is not. you merged your remote branch instead of forcing the push |
Prevents the DateTime object to be updated when changing the DateTimeZone. Unit tests added. Updated changelog.
Commits ------- c53a02d Fixed DateTimeZone support in date filter. Discussion ---------- Fixed DateTimeZone support in date filter. Now $date converted to a DateTime has a DateTimeZone in every case. --------------------------------------------------------------------------- by fabpot at 2012-05-28T19:20:47Z Can you add some unit tests to avoid regression to happen later? Thanks. --------------------------------------------------------------------------- by blaugueux at 2012-05-29T07:52:58Z @fabpot Here they are. --------------------------------------------------------------------------- by fabpot at 2012-05-29T10:29:28Z Great! Can you add a note in the CHANGELOG file and then squash your commit before I merge? Thanks. --------------------------------------------------------------------------- by blaugueux at 2012-05-29T12:52:22Z Changelog added. First squash of my life, i cannot say if it's good :( --------------------------------------------------------------------------- by stof at 2012-05-29T12:55:26Z it is not. you merged your remote branch instead of forcing the push --------------------------------------------------------------------------- by blaugueux at 2012-05-29T17:12:05Z Ok @stof, thanks for your help. I think it's good now. @fabpot Is it ok for you ?
Now $date converted to a DateTime has a DateTimeZone in every case.