[VarDumper] Add date caster #22431

maidmaid · 2017-04-14T01:17:45Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	/
License	MIT
Doc PR	/

I propose to add a DateCaster that casts date (with timestamp, date, time, timezone (offset + id), literal date, delta from now and DST) and interval.

Todo:

cast date
cast interval
add tests

fabpot · 2017-04-14T14:51:49Z

/cc @nicolas-grekas

ro0NL · 2017-04-14T16:57:49Z

Btw.. about the timezone. What about putting the offset in front? this is the part i usually care about...

timezone: '+02:00 (Europe/Amsterdam)'

ro0NL · 2017-04-14T17:43:31Z

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

+            $prefix.'literal' => $d->format('l, j M Y'),
+            $prefix.'timestamp' => $d->getTimestamp(),
+            $prefix.'timezone' => $d->format('P (e)'),
+            $prefix.'Δnow' => (new DateTime())->diff($d)->format('%R %yy %mm %dd %H:%I:%S'),


Needs a setTimezone($d->getTimezone()) right?

Good catch!

maidmaid · 2017-04-14T20:33:01Z

WDYT of this variant?

ro0NL · 2017-04-16T08:30:39Z

Not sure.. it looks a bit more chaotic imo (less raw).

Compared to the first proposal, i think we should go with datetime, timezone, Δnow as a bare minimum. Ie. i could do without literal and timestamp.

maidmaid · 2017-04-30T13:14:43Z

Now, literal and timestamp are not visible if they are nested or if we flag with EXCLUDE_VERBOSE. I have too add tests.

stof · 2017-04-30T16:08:51Z

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

+ *
+ * @author Dany Maillard <danymaillard93b@gmail.com>
+ */
+class DateCaster


should be named DateTimeCaster IMO

In a next PR, I would cast other date classes like DateInterval. It's why a generic DateCaster is better IMO.

why waiting?

I agree, it would be better to have everything in the same PR.

stof · 2017-04-30T16:09:04Z

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

+namespace Symfony\Component\VarDumper\Caster;
+
+use DateTime;
+use DateTimeInterface;


We never add use statements for global classes

robfrawley · 2017-04-30T16:15:52Z

@maidmaid I was a fan of the first proposal. The unix time is almost always the most important field to me (though this could depend on the operating system one uses, I guess).

maidmaid · 2017-05-01T14:08:19Z

@robfrawley Yes, unix time can be usefull, but it's a machine format and we should favor human representation, IMO.

ogizanagi · 2017-05-01T14:12:04Z

@maidmaid : It depends on the situation, but you might be happy to have the unix time while debugging. And I'm mainly using the var-dumper component for debugging purpose 😄 (I'd say I rather have the timestamp than the delta or literal version)

maidmaid · 2017-05-01T19:04:20Z

We have lots of different interesting opinions. @nicolas-grekas, can you give your opinion?

nicolas-grekas · 2017-05-02T10:11:33Z

May I propose something different:
always display a single line that represents 100% of the state of the objects: 'Y-m-d H:i:s.u e'
then lets provide more details on hover (... from now, literal, etc as a single "title" line).
This is doable with a single virtual propery holding a ConstStub I think.

maidmaid · 2017-05-02T10:42:04Z

then lets provide more details on hover

But works it with CliDumper?

nicolas-grekas · 2017-05-02T10:43:43Z

CliDumper would display only Y-m-d H:i:s.u e, but this LGTM, that's still 100% of the state.

nicolas-grekas · 2017-05-02T10:57:34Z

For the reader: when I saw that, I first thought "hey, this is hiding the actual internal structure of DateTime objects" (by not telling about these existing public props.)
Then I ran this: https://3v4l.org/uWc54
It looks like doing var_dump/array-cast actually creates the 3 props...
So I'm even more in favor of doing something here, and I would add DateTimeZone, etc. to the list of classes that should display only virtual props.

maidmaid · 2017-05-02T11:20:59Z

When we var_dump a DateTime object, we can see 3 props in all but 2 distinct props: date and timezone. So, DateCaster should cast a date prop that represents 100% of date/time infos (with more details on hover), and a timezone prop that shows a DateTimeZone thanks to an other caster (that is already in WIP here). Right?

nicolas-grekas · 2017-05-02T11:26:04Z

worth a try yes :)

maidmaid · 2017-05-02T11:53:17Z

As discussed:

My remarks: too bad it is not possible to copy timestamp and to see extra info in CLI with this version. More, I find it strange to use ConstStub to show extra in hover, it seems as a hack.

nicolas-grekas · 2017-05-02T12:02:45Z

Personally, I like compact formats, that give me everything in a snap.
I would also remove the timezone prop in fact, and add the "e" placeholder at the end of the date string.
Nesting is an opportunity to be hit by depth limiting, better not.
Copy/pasting the timestamp looks like something I wouldn't do very often to me (especially since it's destructive compared to a DateTime object).
Of course, that's only my personal PoV :)

nicolas-grekas · 2017-05-02T12:11:34Z

What about displaying the timestamp next to the class name DateTime @123456789 { ... }? Should be just a matter of appending it to the $stub->class prop.

nicolas-grekas · 2017-05-02T12:13:45Z

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

+                "literal: %s\ntimestamp: %s\nΔnow: %s",
+                $d->format('l, j F Y'),
+                $d->getTimestamp(),
+                (new \DateTime('now', $d->getTimezone()))->diff($d)->format('%R %yy %mm %dd %H:%I:%S')


$d->getTimezone() is not required in this context

maidmaid · 2017-05-02T14:04:44Z

I putted timestamp in $stub->class and displayed timezone in date virtual prop (used P instead e because +02:00 is more informative that Europe/Zurich):

nicolas-grekas · 2017-05-02T14:09:40Z

Do you like it? I think I do :)

used P instead e because +02:00 is more informative

in fact, that's not true: Europe/Zurich contains geographical/political info that makes it very different from +02:00.
See http://stackoverflow.com/questions/17694894/different-timezone-types-on-datetime-object

maidmaid · 2017-05-02T14:21:26Z

Yes, I find this version quite elegant :)

About timezone, I think that we have to know difference to GMT when we debug a date but not its geographical/political infos. Are you agree?

My only little remark rests on ConstStub hack usage to showing extra infos when [ConstStub] Represents a PHP constant and its value.

maidmaid · 2017-05-08T15:30:09Z

More, the plus-value of literal format is to show day of week and textual month. With U.S. standard, these 2 items are in the front.

FabienPapet · 2017-05-08T18:30:51Z

Shouldn't the 'from now' be 'from server time' , using 'now' can be confusing if the server time is not the developer time

WDYT ?

maidmaid · 2017-05-08T19:09:35Z

@FabienPapet I think that dump a date is in most cases a debug step that is realized on our local machine whose time we known.

maidmaid · 2017-05-08T23:10:47Z

@nicolas-grekas I think this version is quite conclusive. What about merge this and open new PR if we have tweaks?

nicolas-grekas

👍 (To be merged when the feature freeze is over)

ogizanagi · 2017-06-26T21:03:28Z

This should be ready to be merged now :)

fabpot · 2017-06-28T07:12:03Z

If we want to support DateInterval, I think it should be done in this PR.

maidmaid · 2017-06-28T12:24:42Z

Ok, I added DateInterval caster.

Before	After

nicolas-grekas · 2017-07-03T07:16:24Z

Hum, let's remove the DateInterval caster, at least from this PR. The issue I have is accuracy and teaching. Dump should be informational about how things actually works or can be used. On a DateInterval, the listed values are actual public properties. VarDumper is about showing actual facts about objects, not alternate ones ;)
The concern does not exist with DateTime objects because there is no properties there.

BTW, I agree with the comment on the name of the caster method: should be castDateTime also IMHO.

maidmaid · 2017-07-03T08:10:01Z

Ok, I removed DateInterval caster and I renamed caster method to castDateTime.

nicolas-grekas · 2017-07-03T08:35:52Z

Thank you @maidmaid.

This PR was merged into the 3.4 branch. Discussion ---------- [VarDumper] Add date caster | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | / | License | MIT | Doc PR | / I propose to add a ``DateCaster`` that casts date (with timestamp, date, time, timezone (offset + id), literal date, delta from now and DST) and interval. Todo: - [x] cast date - [x] cast interval - [x] add tests Commits ------- 1dbdf1c Add DateCaster

This PR was merged into the 3.4 branch. Discussion ---------- [VarDumper] Add interval caster | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | / | License | MIT | Doc PR | / cf #22431 (comment) Commits ------- a73522c Add interval caster

This PR was merged into the 3.4 branch. Discussion ---------- [VarDumper] Add interval caster | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | / | License | MIT | Doc PR | / cf symfony/symfony#22431 (comment) Commits ------- a73522c Add interval caster

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

carsonbot added Status: Needs Review VarDumper Feature labels Apr 14, 2017

ro0NL reviewed Apr 14, 2017

View reviewed changes

nicolas-grekas self-requested a review April 18, 2017 16:08

nicolas-grekas added this to the 3.4 milestone Apr 20, 2017

maidmaid force-pushed the datecaster branch from 0451c8c to 964892f Compare April 30, 2017 13:09

stof requested changes Apr 30, 2017

View reviewed changes

nicolas-grekas reviewed May 2, 2017

View reviewed changes

nicolas-grekas approved these changes May 9, 2017

View reviewed changes

maidmaid force-pushed the datecaster branch from a1244ae to 5f1613a Compare May 19, 2017 13:12

maidmaid changed the base branch from master to 3.4 May 19, 2017 13:12

maidmaid force-pushed the datecaster branch 2 times, most recently from 13250cd to 2b00e4e Compare May 19, 2017 13:21

maidmaid force-pushed the datecaster branch from 2b00e4e to 3c95fb9 Compare June 5, 2017 21:23

nicolas-grekas self-assigned this Jun 26, 2017

Add DateCaster

1dbdf1c

maidmaid force-pushed the datecaster branch from 57a2696 to 1dbdf1c Compare July 3, 2017 08:05

nicolas-grekas merged commit 1dbdf1c into symfony:3.4 Jul 3, 2017

maidmaid deleted the datecaster branch July 3, 2017 08:51

maidmaid mentioned this pull request Jul 3, 2017

[VarDumper] Add interval caster #23357

Merged

javiereguiluz mentioned this pull request Jul 16, 2017

[VarDumper] Add date caster symfony/symfony-docs#8175

Closed

maidmaid mentioned this pull request Jul 19, 2017

[VarDumper] Add time zone caster #23591

Merged

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

[VarDumper] Add date caster #22431

[VarDumper] Add date caster #22431

Conversation

maidmaid commented Apr 14, 2017 • edited

fabpot commented Apr 14, 2017

ro0NL commented Apr 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maidmaid commented Apr 14, 2017

ro0NL commented Apr 16, 2017 • edited

maidmaid commented Apr 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley commented Apr 30, 2017

maidmaid commented May 1, 2017 • edited

ogizanagi commented May 1, 2017 • edited

maidmaid commented May 1, 2017

nicolas-grekas commented May 2, 2017

maidmaid commented May 2, 2017

nicolas-grekas commented May 2, 2017 • edited

nicolas-grekas commented May 2, 2017 • edited

maidmaid commented May 2, 2017

nicolas-grekas commented May 2, 2017

maidmaid commented May 2, 2017 • edited

nicolas-grekas commented May 2, 2017 • edited

nicolas-grekas commented May 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maidmaid commented May 2, 2017 • edited

nicolas-grekas commented May 2, 2017

maidmaid commented May 2, 2017

maidmaid commented May 8, 2017 • edited

FabienPapet commented May 8, 2017

maidmaid commented May 8, 2017

maidmaid commented May 8, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

ogizanagi commented Jun 26, 2017

fabpot commented Jun 28, 2017

maidmaid commented Jun 28, 2017

nicolas-grekas commented Jul 3, 2017 • edited

maidmaid commented Jul 3, 2017

nicolas-grekas commented Jul 3, 2017

maidmaid commented Apr 14, 2017 •

edited

ro0NL commented Apr 14, 2017 •

edited

ro0NL commented Apr 16, 2017 •

edited

maidmaid commented Apr 30, 2017 •

edited

maidmaid commented May 1, 2017 •

edited

ogizanagi commented May 1, 2017 •

edited

nicolas-grekas commented May 2, 2017 •

edited

nicolas-grekas commented May 2, 2017 •

edited

maidmaid commented May 2, 2017 •

edited

nicolas-grekas commented May 2, 2017 •

edited

nicolas-grekas commented May 2, 2017 •

edited

maidmaid commented May 2, 2017 •

edited

maidmaid commented May 8, 2017 •

edited

nicolas-grekas commented Jul 3, 2017 •

edited