-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[VarDumper] Add time zone caster #23591
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
76fae7d
to
4d37445
Compare
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.
cool 👍
@@ -28,7 +28,8 @@ | ||
}, | ||
"suggest": { | ||
"ext-iconv": "To convert non-UTF-8 strings to UTF-8 (or symfony/polyfill-iconv in case ext-iconv cannot be used).", | ||
"ext-symfony_debug": "" | ||
"ext-symfony_debug": "", | ||
"ext-inlt": "To show region in time zone dump" |
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.
ext-intl
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.
Oops, fixed ;)
@@ -32,7 +32,7 @@ public static function castDateTime(\DateTimeInterface $d, array $a, Stub $stub, | ||
; | ||
|
||
$a = array(); | ||
$a[$prefix.'date'] = new ConstStub($d->format('Y-m-d H:i:s.u '.($location ? 'e (P)' : 'P')), $title); | ||
$a[$prefix.'date'] = new ConstStub($d->format('Y-m-d H:i:s.u ').self::formatTimeZone($d->getTimezone()), $title); |
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.
perhaps inline $location above
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.
Fixed
132df43
to
27bf394
Compare
(but tests are red) |
hhvm being weird :) |
I hate you hhvm! I add a 2nd |
Agree :) this is insane. I believe windows was failing due missing intl =/ lets see :) |
On AppVeyor, Intl component takes over and this throws an exception. How fix this? |
try/catch it? Ideally it would be implemented for |
if (($location = $timeZone->getLocation()) && method_exists('\Locale', 'getDisplayRegion')) { | ||
try { | ||
$title = \Locale::getDisplayRegion('-'.$location['country_code'], null); | ||
} catch (\Exception $e) { |
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.
i'd catch Symfony\Component\Intl\Exception\MethodNotImplementedException
explicitly here. Anything else should trow.
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.
I finally handled this with extension_loaded('intl')
but I'm not sure. Is it good for you?
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.
Sounds good 👍
@@ -111,6 +111,7 @@ | ||
|
||
'DateTimeInterface' => array('Symfony\Component\VarDumper\Caster\DateCaster', 'castDateTime'), | ||
'DateInterval' => array('Symfony\Component\VarDumper\Caster\DateCaster', 'castInterval'), | ||
'DateTimeZone' => array('Symfony\Component\VarDumper\Caster\DateCaster', 'castTimeZone'), |
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.
Im assuming next is DatePeriod for completion? That can be easily wired now.
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.
Yes! But let's first fully agree on time zone 😅
@@ -28,7 +28,8 @@ | ||
}, | ||
"suggest": { | ||
"ext-iconv": "To convert non-UTF-8 strings to UTF-8 (or symfony/polyfill-iconv in case ext-iconv cannot be used).", | ||
"ext-symfony_debug": "" | ||
"ext-symfony_debug": "", | ||
"ext-intl": "To show region in time zone dump" |
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.
To show region name in a time zone dump
?
Also perhaps move 1 line up.
$title = ''; | ||
if (($location = $timeZone->getLocation()) && method_exists('\Locale', 'getDisplayRegion')) { | ||
try { | ||
$title = \Locale::getDisplayRegion('-'.$location['country_code'], null); |
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.
null => \Locale::getDefault()
that be safest
0d2ad57
to
7f97d70
Compare
Tests are green :) |
|
||
private static function formatTimeZone(\DateTimeZone $z) | ||
{ | ||
return (new \Datetime('now', $z))->format($z->getLocation() ? 'e (P)' : 'P'); |
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 method should be removed, it forces repeated calls to getLocation that inlinfing prevented - I'm sure things will be easier to read when removed. That means all changes on the castDateTime method should be reverted to me.
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.
Updated
|
||
public static function castTimeZone(\DateTimeZone $timeZone, array $a, Stub $stub, $isNested, $filter) | ||
{ | ||
$title = ($location = $timeZone->getLocation()) && extension_loaded('intl') |
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.
$location on its own line + $title on a single line would be better imho
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.
Updated (although this line is a bit long now...)
Time to merge? |
{ | ||
$location = $timeZone->getLocation(); | ||
$formatted = (new \Datetime('now', $timeZone))->format($location ? 'e (P)' : 'P'); | ||
$title = $location && extension_loaded('intl') ? \Locale::getDisplayRegion('-'.$location['country_code'], \Locale::getDefault()) : ''; |
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.
just realized, does it make sense to use Locale::getDefault()
instead of just en
. Not sure this output needs to be localized at all 😕
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.
dump output is already locale sensitive, so this makes sense to me
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.
date format in castDateTime
is not.. which made me think this should follow.
No blocker for me or so :)
Thank you @maidmaid. |
This PR was merged into the 3.4 branch. Discussion ---------- [VarDumper] Add time zone caster | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22431 (comment) | License | MIT | Doc PR | / Commits ------- 5c4bfac Add time zone caster
This PR was merged into the 3.4 branch. Discussion ---------- [VarDumper] Add period caster | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23591 (comment) | License | MIT | Doc PR | / Result:  Commits ------- 4c4c398 Add period caster
This PR was merged into the 3.4 branch. Discussion ---------- [VarDumper] Add period caster | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/symfony#23591 (comment) | License | MIT | Doc PR | / Result:  Commits ------- 4c4c398 Add period caster
Uh oh!
There was an error while loading. Please reload this page.