NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:3.4frommaidmaid:vardumper-tz

Jul 21, 2017

Merged

[VarDumper] Add time zone caster#23591

nicolas-grekas merged 1 commit intosymfony:3.4frommaidmaid:vardumper-tz

Jul 21, 2017

Conversation

Copy link

Contributor

maidmaid commentedJul 19, 2017•
edited
Loading

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	/

carsonbot added Status: Needs Review VarDumper Feature labels

Jul 19, 2017

Copy link

ContributorAuthor

maidmaid commentedJul 19, 2017

Proposal (exlude verbose mode):

maidmaid force-pushed thevardumper-tz branch 2 times, most recently from76fae7d to4d37445Compare

July 19, 2017 14:05

ro0NL reviewed

Jul 19, 2017

View reviewed changes

Copy link

Contributor

ro0NL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

cool 👍

src/Symfony/Component/VarDumper/composer.json Outdated

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

ext-intl

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Oops, fixed ;)

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

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

perhaps inline $location above

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Fixed

maidmaid force-pushed thevardumper-tz branch 3 times, most recently from132df43 to27bf394Compare

July 19, 2017 14:25

ro0NL approved these changes

Jul 19, 2017

View reviewed changes

Copy link

Member

nicolas-grekas commentedJul 19, 2017

(but tests are red)

Copy link

Contributor

ro0NL commentedJul 19, 2017

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

maidmaid force-pushed thevardumper-tz branch from27bf394 to21efa4eCompare

July 19, 2017 14:42

Copy link

ContributorAuthor

maidmaid commentedJul 19, 2017

I hate you hhvm! I add a 2ndnull arg.

Copy link

Contributor

ro0NL commentedJul 19, 2017

Agree :) this is insane.

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

maidmaid force-pushed thevardumper-tz branch from21efa4e to6d85ae4Compare

July 19, 2017 14:55

Copy link

ContributorAuthor

maidmaid commentedJul 19, 2017

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Sounds good 👍

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

Copy link

Contributor

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.

Copy link

ContributorAuthor

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 😅

src/Symfony/Component/VarDumper/composer.json Outdated

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

Copy link

Contributor

ro0NLJul 19, 2017•
edited
Loading

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.

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

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

Copy link

Contributor

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

seehttps://3v4l.org/eAigW

maidmaid force-pushed thevardumper-tz branch 2 times, most recently from0d2ad57 to7f97d70Compare

July 20, 2017 07:46

Copy link

ContributorAuthor

maidmaid commentedJul 20, 2017

Tests are green :)

ro0NL approved these changes

Jul 20, 2017

View reviewed changes

nicolas-grekas reviewed

Jul 20, 2017

View reviewed changes

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


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

Copy link

Member

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.

Copy link

ContributorAuthor

maidmaidJul 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Updated

nicolas-grekas reviewed

Jul 20, 2017

View reviewed changes

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


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

Copy link

Member

nicolas-grekasJul 20, 2017

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

Copy link

ContributorAuthor

maidmaidJul 20, 2017

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...)

maidmaid force-pushed thevardumper-tz branch from7f97d70 to4dcc5edCompare

July 20, 2017 17:28

Add time zone caster

5c4bfac

maidmaid force-pushed thevardumper-tz branch from4dcc5ed to5c4bfacCompare

July 21, 2017 12:36

Copy link

ContributorAuthor

maidmaid commentedJul 21, 2017

Time to merge?

ro0NL reviewed

Jul 21, 2017

View reviewed changes

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

Copy link

Contributor

ro0NLJul 21, 2017

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 useLocale::getDefault() instead of justen. Not sure this output needs to be localized at all 😕

Copy link

Member

nicolas-grekasJul 21, 2017

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

Copy link

Contributor

ro0NLJul 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

No blocker for me or so :)

nicolas-grekas approved these changes

Jul 21, 2017

View reviewed changes

Copy link

Member

nicolas-grekas commentedJul 21, 2017

Thank you@maidmaid.

nicolas-grekas merged commit5c4bfac intosymfony:3.4

Jul 21, 2017

nicolas-grekas added a commit that referenced this pull request

Jul 21, 2017

feature#23591[VarDumper] Add time zone caster (maidmaid)

04f3e60

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

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

nicolas-grekas added a commit that referenced this pull request

Aug 29, 2017

feature#23668[VarDumper] Add period caster (maidmaid)

660fecc

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

symfony-splitter pushed a commit to symfony/var-dumper that referenced this pull request

Aug 29, 2017

feature #23668 [VarDumper] Add period caster (maidmaid)

87300ea

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

This was referencedOct 18, 2017

Release v3.4.0-BETA1#24609

Merged

Release v4.0.0-BETA1#24612

Merged

Labels

Feature Status: Needs Review VarDumper

4 participants

Movatterモバイル変換

Uh oh!

[VarDumper] Add time zone caster#23591

[VarDumper] Add time zone caster#23591

Uh oh!

Conversation

maidmaid commentedJul 19, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

maidmaid commentedJul 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 commentedJul 19, 2017

Uh oh!

ro0NL commentedJul 19, 2017

Uh oh!

maidmaid commentedJul 19, 2017

Uh oh!

ro0NL commentedJul 19, 2017

Uh oh!

maidmaid commentedJul 19, 2017

Uh oh!

ro0NL commentedJul 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!

ro0NLJul 19, 2017• editedLoading 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 commentedJul 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 commentedJul 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 commentedJul 21, 2017

Uh oh!

Uh oh!

maidmaid commentedJul 19, 2017•
edited
Loading

ro0NLJul 19, 2017•
edited
Loading