[VarDumper] Add time zone caster #23591

maidmaid · Jul 19, 2017

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	/

maidmaid · Jul 19, 2017

Proposal (exlude verbose mode):

ro0NL

cool 👍

ro0NL · Jul 19, 2017

src/Symfony/Component/VarDumper/composer.json

@@ -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"


Oops, fixed ;)

ro0NL · Jul 19, 2017

src/Symfony/Component/VarDumper/Caster/DateCaster.php

@@ -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);


perhaps inline $location above

nicolas-grekas · Jul 19, 2017

(but tests are red)

ro0NL · Jul 19, 2017

hhvm being weird :) Locale::getDisplayRegion() expects exactly 2 parameters, 1 given

maidmaid · Jul 19, 2017

I hate you hhvm! I add a 2nd null arg.

ro0NL · Jul 19, 2017

Agree :) this is insane.

I believe windows was failing due missing intl =/ lets see :)

maidmaid · Jul 19, 2017

On AppVeyor, Intl component takes over and this throws an exception. How fix this?

ro0NL · Jul 19, 2017

try/catch it? Ideally it would be implemented for en locale i guess.

ro0NL · Jul 19, 2017

src/Symfony/Component/VarDumper/Caster/DateCaster.php

+        if (($location = $timeZone->getLocation()) && method_exists('\Locale', 'getDisplayRegion')) {
+            try {
+                $title = \Locale::getDisplayRegion('-'.$location['country_code'], null);
+            } catch (\Exception $e) {


i'd catch Symfony\Component\Intl\Exception\MethodNotImplementedException explicitly here. Anything else should trow.

I finally handled this with extension_loaded('intl') but I'm not sure. Is it good for you?

Sounds good 👍

ro0NL · Jul 19, 2017

src/Symfony/Component/VarDumper/Cloner/AbstractCloner.php

@@ -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'),


Im assuming next is DatePeriod for completion? That can be easily wired now.

Yes! But let's first fully agree on time zone 😅

ro0NL · Jul 19, 2017

src/Symfony/Component/VarDumper/composer.json

@@ -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"


To show region name in a time zone dump?

Also perhaps move 1 line up.

ro0NL · Jul 19, 2017

src/Symfony/Component/VarDumper/Caster/DateCaster.php

+        $title = '';
+        if (($location = $timeZone->getLocation()) && method_exists('\Locale', 'getDisplayRegion')) {
+            try {
+                $title = \Locale::getDisplayRegion('-'.$location['country_code'], null);


null => \Locale::getDefault() that be safest

see https://3v4l.org/eAigW

maidmaid · Jul 20, 2017

Tests are green :)

nicolas-grekas · Jul 20, 2017

src/Symfony/Component/VarDumper/Caster/DateCaster.php

+
+    private static function formatTimeZone(\DateTimeZone $z)
+    {
+        return (new \Datetime('now', $z))->format($z->getLocation() ? 'e (P)' : 'P');


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.

nicolas-grekas · Jul 20, 2017

src/Symfony/Component/VarDumper/Caster/DateCaster.php

+
+    public static function castTimeZone(\DateTimeZone $timeZone, array $a, Stub $stub, $isNested, $filter)
+    {
+        $title = ($location = $timeZone->getLocation()) && extension_loaded('intl')


$location on its own line + $title on a single line would be better imho

Updated (although this line is a bit long now...)

maidmaid · Jul 21, 2017

Time to merge?

ro0NL · Jul 21, 2017

src/Symfony/Component/VarDumper/Caster/DateCaster.php

+    {
+        $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()) : '';


just realized, does it make sense to use Locale::getDefault() instead of just en. Not sure this output needs to be localized at all 😕

dump output is already locale sensitive, so this makes sense to me

date format in castDateTime is not.. which made me think this should follow.

No blocker for me or so :)

nicolas-grekas · Jul 21, 2017

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: ![](https://user-images.githubusercontent.com/4578773/29788181-fce3eb32-8c31-11e7-9da4-72c038d5a14e.png) 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: ![](https://user-images.githubusercontent.com/4578773/29788181-fce3eb32-8c31-11e7-9da4-72c038d5a14e.png) Commits ------- 4c4c398 Add period caster

carsonbot added Status: Needs Review VarDumper Feature labels Jul 19, 2017

maidmaid force-pushed the vardumper-tz branch 2 times, most recently from 76fae7d to 4d37445 Compare July 19, 2017 14:05

ro0NL reviewed Jul 19, 2017

View reviewed changes

maidmaid force-pushed the vardumper-tz branch 3 times, most recently from 132df43 to 27bf394 Compare July 19, 2017 14:25

ro0NL approved these changes Jul 19, 2017

View reviewed changes

maidmaid force-pushed the vardumper-tz branch from 27bf394 to 21efa4e Compare July 19, 2017 14:42

maidmaid force-pushed the vardumper-tz branch from 21efa4e to 6d85ae4 Compare July 19, 2017 14:55

maidmaid force-pushed the vardumper-tz branch from 6d85ae4 to 566dc2e Compare July 19, 2017 15:19

ro0NL reviewed Jul 19, 2017

View reviewed changes

maidmaid force-pushed the vardumper-tz branch 2 times, most recently from 0d2ad57 to 7f97d70 Compare July 20, 2017 07:46

ro0NL approved these changes Jul 20, 2017

View reviewed changes

nicolas-grekas reviewed Jul 20, 2017

View reviewed changes

maidmaid force-pushed the vardumper-tz branch from 7f97d70 to 4dcc5ed Compare July 20, 2017 17:28

Add time zone caster

5c4bfac

maidmaid force-pushed the vardumper-tz branch from 4dcc5ed to 5c4bfac Compare July 21, 2017 12:36

ro0NL reviewed Jul 21, 2017

View reviewed changes

nicolas-grekas approved these changes Jul 21, 2017

View reviewed changes

nicolas-grekas merged commit 5c4bfac into symfony:3.4 Jul 21, 2017

maidmaid deleted the vardumper-tz branch July 21, 2017 13:45

maidmaid mentioned this pull request Jul 25, 2017

[VarDumper] Add period caster #23668

Merged

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

[VarDumper] Add time zone caster #23591

[VarDumper] Add time zone caster #23591

Uh oh!

Conversation

maidmaid commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maidmaid commented Jul 19, 2017

Uh oh!

ro0NL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jul 19, 2017

Uh oh!

ro0NL commented Jul 19, 2017

Uh oh!

maidmaid commented Jul 19, 2017

Uh oh!

ro0NL commented Jul 19, 2017

Uh oh!

maidmaid commented Jul 19, 2017

Uh oh!

ro0NL commented Jul 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NL Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maidmaid commented Jul 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maidmaid commented Jul 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jul 21, 2017

Uh oh!

Uh oh!

maidmaid commented Jul 19, 2017 •

edited

Loading

ro0NL Jul 19, 2017 •

edited

Loading