[VarDumper] Add interval caster #23357

maidmaid · 2017-07-03T11:32:12Z

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)

maidmaid · 2017-07-03T11:32:40Z

First proposal:

the main public properties (y, m, d, h, i, s, and f on PHP 7.1) are merged in oneline interval virtual property;
the others public properties (invert and days) are unmodified;
and the public properties not officially documentated (weekday, weekday_behavior, first_last_day_of, special_type, special_amount, have_weekday_relative and have_special_relative) are removed (see the Class synopsis).

Before	After

stof · 2017-07-03T12:03:49Z

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

+
+    public static function castInterval(\DateInterval $i, array $a, Stub $stub, $isNested, $filter)
+    {
+        $prefix = Caster::PREFIX_VIRTUAL;


no need to define a variable, as it is used only once

stof · 2017-07-03T12:04:42Z

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

+            .($i->d ? '%dd ' : '')
+        ;
+
+        if (PHP_VERSION_ID >= 70100) {


please use \PHP_VERSION_ID, to let the version comparison be applied at compile time (not qualifying the constant requires doing it at runtime to check the constant first in the current namespace)

stof · 2017-07-03T12:05:34Z

src/Symfony/Component/VarDumper/Tests/Caster/DateCasterTest.php

+
+    public function provideIntervals()
+    {
+        $ms = function () { return PHP_VERSION_ID >= 70100 ? '.000000' : ''; };


no need to use a function. Use a simple variable, as this is purely static.

ogizanagi · 2017-07-04T18:44:15Z

I agree with @nicolas-grekas's original comment (#22431 (comment)), so I'm not convinced by the new suggested output.
Perhaps we could just exclude non-officially documented public properties and add your interval virtual property. Then, potentially exclude public properties merged in the interval virtual properties when used with the EXCLUDE_VERBOSE filter, so it won't be shown within the form and validator data collectors for instance, but still shown when dumping around elsewhere.

Also, why showing the invert field when it's already part of the interval? According to the above suggestions, I'll hide it too when using EXCLUDE_VERBOSE.

Would it be interesting to also output it as the time_interval spec?
https://github.com/jaroslavtyc/granam-date-interval/blob/master/Granam/DateInterval/DateInterval.php#L175-L200

WDYT?

maidmaid · 2017-07-04T20:00:27Z

Thank you for your thoughts @ogizanagi. I would say this:

Why do not you like removing time properties? This deletion is in favor of interval that is a full representation of this props.
About invert prop, you are right.
About time_interval spec, I think that interval virtual prop is a good replacement and more intuitive to read (P1Y2M3DT4H5M6S vs + 1y 2m 3d 04:05:06). Moreover, it does not handle milliseconds and the negative intervals.

ogizanagi · 2017-07-04T20:14:36Z

Why do not you like removing time properties? This deletion is in favor of interval that is a full representation of this props.

Just quoting @nicolas-grekas:

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

I'm using psysh quite often, to quickly test or demonstrate things, and despite built-in introspection/doc commands like ls -al, show, doc, ... I like the usage of the var-dumper component to output everything and the fact it shows real internals, not representations. I wish it'd stay this way, but I'm not against adding some helpful representations in addition.

nicolas-grekas · 2017-07-11T06:29:43Z

Same opinion as @ogizanagi.
Maybe display the short output when the EXCLUDE_VERBOSE flag is set (the default on psysh)?

maidmaid · 2017-07-11T10:24:34Z

Ok, here the new result according to your comments:

normal	`EXCLUDE_VERBOSE`

nicolas-grekas · 2017-07-11T13:39:11Z

"normal" should display all existing properties, even undocumented ones. Also, why keep "days" in compact mode?

maidmaid · 2017-07-11T17:08:59Z

It's updated: "normal" displays all, and only interval prop is kept in compact mode.

nicolas-grekas

with some suggestions

nicolas-grekas · 2017-07-11T20:56:27Z

src/Symfony/Component/VarDumper/Tests/Caster/DateCasterTest.php

+    /**
+     * @dataProvider provideIntervals
+     */
+    public function testCastInterval($interval_spec, $invert, $expected)


should be $intervalSpec

Fixed (in official doc, this argument is named $interval_spec).

nicolas-grekas · 2017-07-11T20:58:03Z

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

+
+    public static function castInterval(\DateInterval $interval, array $a, Stub $stub, $isNested, $filter)
+    {
+        $i = array(Caster::PREFIX_VIRTUAL.'interval' => self::formatInterval($interval));


what about using a ConstStub here that gives the number of seconds on hover (html mode):
new ConstStub(self::formatInterval($interval), $numberOfSeconds);

Good idea. Number of seconds added.

maidmaid · 2017-07-12T10:19:59Z

How to fix kind of HHVM failure?

1) Symfony\Component\VarDumper\Tests\Caster\DateCasterTest::testDumpInterval with data set #0 ('PT0S', 0, '0s', '0s')
Failed asserting that format description matches text.
--- Expected
+++ Actual
@@ @@
 DateInterval {
   interval: 0s
 }
-}

nicolas-grekas · 2017-07-12T11:17:21Z

diff --git a/src/Symfony/Component/VarDumper/Caster/DateCaster.php b/src/Symfony/Component/VarDumper/Caster/DateCaster.php
index c9edc10..1ecd962 100644
--- a/src/Symfony/Component/VarDumper/Caster/DateCaster.php
+++ b/src/Symfony/Component/VarDumper/Caster/DateCaster.php
@@ -58,7 +58,7 @@ class DateCaster
             .($i->d ? '%dd ' : '')
         ;
 
-        if (\PHP_VERSION_ID >= 70100) {
+        if (isset($i->f)) {
             $format .= $i->h || $i->i || $i->s || $i->f ? '%H:%I:%S.%F' : '';
         } else {
             $format .= $i->h || $i->i || $i->s ? '%H:%I:%S' : '';
diff --git a/src/Symfony/Component/VarDumper/Tests/Caster/DateCasterTest.php b/src/Symfony/Component/VarDumper/Tests/Caster/DateCasterTest.php
index 9db333e..c4c1263 100644
--- a/src/Symfony/Component/VarDumper/Tests/Caster/DateCasterTest.php
+++ b/src/Symfony/Component/VarDumper/Tests/Caster/DateCasterTest.php
@@ -97,8 +97,7 @@ EODUMP;
         $xDump = <<<EODUMP
 DateInterval {
   interval: $expected
-%a
-}
+%A}
 EODUMP;
 
         $this->assertDumpMatchesFormat($xDump, $interval);
@@ -162,7 +161,8 @@ EODUMP;
 
     public function provideIntervals()
     {
-        $ms = \PHP_VERSION_ID >= 70100 ? '.000000' : '';
+        $interval = new \DateInterval('PT0S');
+        $ms = isset($interval->f);
 
         return array(
             array('PT0S', 0, '0s', '0s'),

maidmaid · 2017-07-12T12:50:14Z

Mmh, isset($interval->f) triggers a notice in 7.0, see https://3v4l.org/hJB5A. I use PHP_VERSION_ID?

nicolas-grekas · 2017-07-12T12:53:07Z

but my HHVM fails when PHP_VERSION_ID is used...

nicolas-grekas · 2017-07-12T12:53:28Z

you should report the notice as a bug I guess

nicolas-grekas · 2017-07-12T12:55:08Z

using both PHP_VERSION_ID and the isset check should do it

maidmaid · 2017-07-12T13:59:33Z

There was no error related with PHP_VERSION_ID previously. Tests seem fine now (failure seems unrelated).

maidmaid · 2017-07-12T14:03:32Z

you should report the notice as a bug

https://bugs.php.net/bug.php?id=74911 ;)

nicolas-grekas · 2017-07-12T14:16:30Z

was no error related with PHP_VERSION_ID previously

that's because the behavior of hhvm changes from version to version and Travis doesn't have the latest. But trust me, this check is not enough, you need the isset one also :)

nicolas-grekas · 2017-07-12T14:16:51Z

(or install hhvm to see by yourself ;) )

maidmaid · 2017-07-12T18:03:20Z

Updated: used PHP_VERSION_ID + the isset check.

maidmaid · 2017-07-18T09:08:21Z

Time to merge? :)

nicolas-grekas · 2017-07-19T05:50:18Z

Thank you @maidmaid.

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

…interval caster (maidmaid) This PR was merged into the 4.0-dev branch. Discussion ---------- [VarDumper] Remove low PHP version and hhvm compat in interval caster | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23357 (comment) | License | MIT | Doc PR | / Commits ------- ccca65b Remove hhvm compat

carsonbot added Status: Needs Review VarDumper labels Jul 3, 2017

stof requested changes Jul 3, 2017

View reviewed changes

maidmaid force-pushed the datecaster-interval-2 branch from 00552e1 to fcafa3a Compare July 3, 2017 12:17

nicolas-grekas added this to the 3.4 milestone Jul 11, 2017

maidmaid force-pushed the datecaster-interval-2 branch from fcafa3a to 79e7909 Compare July 11, 2017 10:18

maidmaid force-pushed the datecaster-interval-2 branch from 79e7909 to 1971d78 Compare July 11, 2017 17:06

nicolas-grekas approved these changes Jul 11, 2017

View reviewed changes

maidmaid force-pushed the datecaster-interval-2 branch 2 times, most recently from a123ea0 to 2a64de6 Compare July 12, 2017 10:07

maidmaid force-pushed the datecaster-interval-2 branch from 2a64de6 to 95a51e7 Compare July 12, 2017 12:11

maidmaid force-pushed the datecaster-interval-2 branch 2 times, most recently from 264e264 to ee7bcf5 Compare July 12, 2017 13:47

Add interval caster

a73522c

maidmaid force-pushed the datecaster-interval-2 branch from ee7bcf5 to a73522c Compare July 12, 2017 18:01

nicolas-grekas merged commit a73522c into symfony:3.4 Jul 19, 2017

maidmaid mentioned this pull request Jul 21, 2017

[VarDumper] Remove low PHP version and hhvm compat in interval caster #23614

Merged

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VarDumper] Add interval caster #23357

[VarDumper] Add interval caster #23357

maidmaid commented Jul 3, 2017

maidmaid commented Jul 3, 2017 •

edited

Loading

stof Jul 3, 2017

maidmaid Jul 3, 2017

stof Jul 3, 2017

maidmaid Jul 3, 2017

stof Jul 3, 2017

maidmaid Jul 3, 2017

ogizanagi commented Jul 4, 2017 •

edited

Loading

maidmaid commented Jul 4, 2017

ogizanagi commented Jul 4, 2017 •

edited

Loading

nicolas-grekas commented Jul 11, 2017

maidmaid commented Jul 11, 2017

nicolas-grekas commented Jul 11, 2017

maidmaid commented Jul 11, 2017 •

edited

Loading

nicolas-grekas left a comment

nicolas-grekas Jul 11, 2017

maidmaid Jul 12, 2017

nicolas-grekas Jul 11, 2017

maidmaid Jul 12, 2017

maidmaid commented Jul 12, 2017

nicolas-grekas commented Jul 12, 2017

maidmaid commented Jul 12, 2017

nicolas-grekas commented Jul 12, 2017

nicolas-grekas commented Jul 12, 2017

nicolas-grekas commented Jul 12, 2017

maidmaid commented Jul 12, 2017 •

edited

Loading

maidmaid commented Jul 12, 2017

nicolas-grekas commented Jul 12, 2017 •

edited

Loading

nicolas-grekas commented Jul 12, 2017

maidmaid commented Jul 12, 2017 •

edited

Loading

maidmaid commented Jul 18, 2017

nicolas-grekas commented Jul 19, 2017

[VarDumper] Add interval caster #23357

[VarDumper] Add interval caster #23357

Conversation

maidmaid commented Jul 3, 2017

maidmaid commented Jul 3, 2017 • edited Loading

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

ogizanagi commented Jul 4, 2017 • edited Loading

maidmaid commented Jul 4, 2017

ogizanagi commented Jul 4, 2017 • edited Loading

nicolas-grekas commented Jul 11, 2017

maidmaid commented Jul 11, 2017

nicolas-grekas commented Jul 11, 2017

maidmaid commented Jul 11, 2017 • edited Loading

nicolas-grekas left a 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

maidmaid commented Jul 12, 2017

nicolas-grekas commented Jul 12, 2017

maidmaid commented Jul 12, 2017

nicolas-grekas commented Jul 12, 2017

nicolas-grekas commented Jul 12, 2017

nicolas-grekas commented Jul 12, 2017

maidmaid commented Jul 12, 2017 • edited Loading

maidmaid commented Jul 12, 2017

nicolas-grekas commented Jul 12, 2017 • edited Loading

nicolas-grekas commented Jul 12, 2017

maidmaid commented Jul 12, 2017 • edited Loading

maidmaid commented Jul 18, 2017

nicolas-grekas commented Jul 19, 2017

maidmaid commented Jul 3, 2017 •

edited

Loading

ogizanagi commented Jul 4, 2017 •

edited

Loading

ogizanagi commented Jul 4, 2017 •

edited

Loading

maidmaid commented Jul 11, 2017 •

edited

Loading

maidmaid commented Jul 12, 2017 •

edited

Loading

nicolas-grekas commented Jul 12, 2017 •

edited

Loading

maidmaid commented Jul 12, 2017 •

edited

Loading