[Yaml] Support parsing YAML timestamps as DateTime #14420
Conversation
remi-blaise
commented
Apr 20, 2015
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | yes |
| Tests pass? | yes |
| Fixed tickets | #11838 |
| License | MIT |
| Doc PR |
|
Hey ! |
| @@ -104,6 +108,18 @@ public static function dump($value, $exceptionOnInvalidType = false, $objectSupp | |||
|
|
|||
| return 'null'; | |||
| case is_object($value): | |||
| if ($value instanceof \DateTime) { | |||
There was a problem hiding this comment.
The DateTimeInterface is not available in PHP 5.3.
There was a problem hiding this comment.
So actually we should check for both concrete DateTimeInterface implementations:
if ($value instanceof \DateTime || $value instanceof \DateTimeImmutable) {
// ...
}|
2.7 is feature frozen, so the target branch should be 2.8 |
| deprecated in favor of the new `getPath()` method. | ||
|
|
||
|
|
||
| Yaml |
There was a problem hiding this comment.
This info should be in the CHANGELOG.md file of the Yaml component.
There was a problem hiding this comment.
Ha ok. I didn't understand why only fappot writte the CHANGELOG so I didn't touch it. I didn't know there were CHANGELOG for each component.
|
@dosten How can I change the target branch ? |
|
@Zzortell you don't need to do anything, if your PR gets accepted, a merger will merge this in 2.8 instead of 2.7. |
|
Oki |
| @@ -104,6 +108,18 @@ public static function dump($value, $exceptionOnInvalidType = false, $objectSupp | |||
|
|
|||
| return 'null'; | |||
| case is_object($value): | |||
| if ($value instanceof \DateTime || $value instanceof \DateTimeImmutable) { | |||
There was a problem hiding this comment.
use DateTimeInterface rather than DateTimeImmutable. This way, the DateTimeInterface check covers all supported cases for PHP 5.5+ (and we can drop the DateTime check in 3.0 when dropping the PHP 5.4 support)
| @@ -104,6 +108,18 @@ public static function dump($value, $exceptionOnInvalidType = false, $objectSupp | |||
|
|
|||
| return 'null'; | |||
| case is_object($value): | |||
| if ($value instanceof \DateTime || $value instanceof \DateTimeImmutable) { | |||
| if ($value->getTimezone()->getName() === date_default_timezone_get()) { | |||
There was a problem hiding this comment.
I'm not sure about this. The code parsing the date might have a different default timezone. It is better to always include all info IMO
There was a problem hiding this comment.
Well, if the TimeZone has been intentionally defined as for exemple +1 (in the YAML file parsed or in the PHP datas), it dumps it (because the server server timestamp is for example Europe/Paris and not +1).
If it was not defined manually, it has the default TimeZone and so the Yaml component doesn't dump the timestamp (letting it with default value in some ways).
I think personally it's the best behaviour.
There was a problem hiding this comment.
What if it was explicitly defined as UTC and the server default timezone is also UTC ? You have no way to know whether the UTC timezone was explicitly chosen or no. But even worse, you have no idea how this will be parsed. So IMO, you should not drop information when dumping, assuming that the server parsing your YAML will have the same config
|
Now that we have flag support in YAML (as of 3.1), this one could be reconsidered. What do you think @xabbuh? |
|
I agree. @Zzortell Do you think you will be able to finish here or should I take over? |
|
closing in favour of #17836 |
…buh) This PR was merged into the 3.1-dev branch. Discussion ---------- [Yaml] support to parse and dump DateTime objects | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #6275, #8580, #11838, #14420 | License | MIT | Doc PR | TODO Commits ------- 7e1c6c4 [Yaml] support to parse and dump DateTime objects