[VarDumper] Add period caster #23668

maidmaid · Jul 25, 2017

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:

maidmaid · Jul 25, 2017

First proposal:

ro0NL · Jul 26, 2017

If we only want start/end/interval to be nicely dumped, that would already work as is right?

Im not sure about the fancy period formatting :) though i like your creativity 👍

nicolas-grekas · Jul 26, 2017

agree with @ro0NL, not really convinced myself sorry...
maybe something like "every $period from $start to $end" (& variants)?

maidmaid · Jul 27, 2017

Ok, I made tests green and I updated format. Example with end date (1) and with recurrences (2):

1) every + 1d, from 2017-01-01 00:00:00 (included) to 2017-01-03 00:00:00
2) every + 1d, from 2017-01-01 00:00:00 (included) for 2x

I kept tooltip with the dates included in the period. I find this useful, and you?

ro0NL · Aug 1, 2017

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

+        if (\PHP_VERSION_ID >= 70107) { // see https://bugs.php.net/bug.php?id=74639
+            $format = 'Y-m-d'.('000000' === $p->getStartDate()->format('His') && '000' === $p->getDateInterval()->format('%h%i%s') ? '' : ' H:i:s');
+            foreach (clone $p as $i => $d) {
+                $dates[] = sprintf('%s) %s', $i + 1, $d->format($format));


this list can become insanely long... not sure we should do it.

Limit to x first items would be better? If yes, showing the first 12 items seem good to me.

dunno.. i would leave it out and keep the virtual period property.

Mmh, an overview of dates period can be very useful I think. Let's wait for other opinions ;)

ro0NL · Aug 1, 2017

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

+            self::formatInterval($p->getDateInterval()),
+            $p->getStartDate()->format('Y-m-d H:i:s'),
+            $p->include_start_date ? 'included' : 'excluded',
+            ($end = $p->getEndDate()) ? 'to '.$end->format('Y-m-d H:i:s') : 'for '.$p->recurrences.'x'


, recurring x time/s?

maidmaid · Aug 3, 2017

Fail not related.

ro0NL · Aug 6, 2017

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

+            foreach (clone $p as $i => $d) {
+                $dates[] = sprintf('%s) %s', $i + 1, $d->format('Y-m-d H:i:s'));
+                if (11 === $i) {
+                    $dates[] = '...';


what about x more (see iterator_count)

Yes, good idea. But iterator_count() loops on all dates, what we want to avoid. We can find x with recurrences - limit in recurrences case and (end - start - limit) / interval in date case. WDYT?

I added x more ;) I compared iterator_count() and my method with 1M of reccurences:

4160.1 ms with iterator_count()

0.1 ms with my method

ro0NL · Aug 6, 2017

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

+            self::formatInterval($p->getDateInterval()),
+            $p->getStartDate()->format('Y-m-d H:i:s'),
+            $p->include_start_date ? 'included' : 'excluded',
+            ($end = $p->getEndDate()) ? 'to '.$end->format('Y-m-d H:i:s') : 'recurring '.$p->recurrences.' time/s'


maybe add 1 === $p->recurrences ? 'recurring once' : ... case for time/s :)

I don't see the interest of this. Always having a digital format is the simplest, no?

nicolas-grekas · Aug 7, 2017

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

+            ($end = $p->getEndDate()) ? 'to '.$end->format('Y-m-d H:i:s') : 'recurring '.$p->recurrences.' time/s'
+        );
+
+        $p = array(Caster::PREFIX_VIRTUAL.'period' => new ConstStub($period, implode(PHP_EOL, $dates)));


"\n" instead of PHP_EOL

Updated (Why exactly do you ask this change?)

Purely out of interest, I too would like to know why the change. Is it simply a Symfony convention or is there an another underlying reason for it?

maidmaid · Aug 14, 2017

Can you review this PR please? :)

nicolas-grekas · Aug 16, 2017

is the screenshot up to date?

maidmaid · Aug 17, 2017

Screenshot updated in PR description.

nicolas-grekas · Aug 28, 2017

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

@@ -20,6 +20,8 @@
 */
 class DateCaster
 {
+    const PERIOD_LIMIT = 12;


why 12? I would have set 3 myself :)
the const should be removed (as we cannot make it private)

It's arbitrary but monthly periods can be displayed for a whole year with a limit of 12.

3 still looks enough to me, nobody will look at 12 values...

nicolas-grekas · Aug 28, 2017

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

@@ -80,6 +82,41 @@ public static function castTimeZone(\DateTimeZone $timeZone, array $a, Stub $stu
        return $filter & Caster::EXCLUDE_VERBOSE ? $z : $z + $a;
    }

+    public static function castPeriod(\DatePeriod $p, array $a, Stub $stub, $isNested, $filter)
+    {
+        if (defined('HHVM_VERSION_ID') || \PHP_VERSION_ID < 50605) {


what happens before 5.6.5?

getStartDate(), getDateInterval() and getEndDate() methods were added in 5.6.5.

nicolas-grekas · Aug 28, 2017

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

+        $dates = array();
+        if (\PHP_VERSION_ID >= 70107) { // see https://bugs.php.net/bug.php?id=74639
+            foreach (clone $p as $i => $d) {
+                if (self::PERIOD_LIMIT === $i) {


if (3 === $i) {

nicolas-grekas · Aug 28, 2017

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

+                    $now = new \DateTimeImmutable();
+                    $dates[] = sprintf('%s more', ($end = $p->getEndDate())
+                        ? ceil(($end->format('U.u') - $d->format('U.u')) / ($now->add($p->getDateInterval())->format('U.u') - $now->format('U.u')))
+                        : $p->recurrences - self::PERIOD_LIMIT


$p->recurrences - $i

nicolas-grekas · Aug 28, 2017

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

+        if (\PHP_VERSION_ID >= 70107) { // see https://bugs.php.net/bug.php?id=74639
+            foreach (clone $p as $i => $d) {
+                if (self::PERIOD_LIMIT === $i) {
+                    // avoids iterator_count() function which can be expensive


not sure about the comment: nobody was wondering about iterator_count until read :)

maidmaid · Aug 28, 2017

Changes done + screenshot updated.

nicolas-grekas · Aug 29, 2017

Thank you @maidmaid.

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

nicolas-grekas · Aug 29, 2017

@maidmaid this is segfaulting on PHP 5.6, see https://travis-ci.org/symfony/symfony/jobs/269610022
Can you submit a fix please?

maidmaid · Aug 29, 2017

Ok, I check that now.

This PR was merged into the 3.4 branch. Discussion ---------- [VarDumper] Fix segfault in period caster | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23668 (comment) | License | MIT | Doc PR | / [This segfault](https://bugs.php.net/bug.php?id=71635) was fixed in [5.6.20](http://www.php.net/ChangeLog-5.php#5.6.20) and in [7.0.5](http://www.php.net/ChangeLog-7.php#7.0.5). Issue in action here: https://3v4l.org/DhOdT#v565 Commits ------- d84e9c8 Fix segfault in period caster

This PR was merged into the 4.0-dev branch. Discussion ---------- [VarDumper] Prepare period caster for 4.0 | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23668 | License | MIT | Doc PR | / Commits ------- fd7352e Prepare period caster for 4.0

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

maidmaid force-pushed the vardumper-period branch from 13d4018 to 3e25472 Compare July 25, 2017 20:36

maidmaid changed the title ~~[VarDumper] Add period caster~~ [WIP] [VarDumper] Add period caster Jul 25, 2017

maidmaid force-pushed the vardumper-period branch 2 times, most recently from 4c9a9ee to cce0530 Compare July 25, 2017 21:34

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

maidmaid force-pushed the vardumper-period branch 8 times, most recently from a22f12d to 0f377e5 Compare July 27, 2017 17:33

maidmaid changed the title ~~[WIP] [VarDumper] Add period caster~~ [VarDumper] Add period caster Jul 27, 2017

ro0NL reviewed Aug 1, 2017

View reviewed changes

maidmaid force-pushed the vardumper-period branch from 0f377e5 to 8d25558 Compare August 3, 2017 12:45

maidmaid force-pushed the vardumper-period branch from 8d25558 to 39f45a2 Compare August 5, 2017 18:34

ro0NL approved these changes Aug 6, 2017

View reviewed changes

nicolas-grekas reviewed Aug 7, 2017

View reviewed changes

maidmaid force-pushed the vardumper-period branch 2 times, most recently from 84f73dd to 915fd4a Compare August 7, 2017 11:20

nicolas-grekas reviewed Aug 28, 2017

View reviewed changes

Add period caster

4c4c398

maidmaid force-pushed the vardumper-period branch from 915fd4a to 4c4c398 Compare August 28, 2017 18:44

nicolas-grekas approved these changes Aug 28, 2017

View reviewed changes

nicolas-grekas merged commit 4c4c398 into symfony:3.4 Aug 29, 2017

maidmaid mentioned this pull request Aug 29, 2017

[VarDumper] Fix segfault in period caster #24023

Merged

maidmaid deleted the vardumper-period branch August 29, 2017 15:32

maidmaid mentioned this pull request Aug 30, 2017

[VarDumper] Prepare period caster for 4.0 #24034

Merged

javiereguiluz mentioned this pull request Sep 10, 2017

[VarDumper] Added a date and time period caster symfony/symfony-docs#8376

Closed

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 period caster #23668

[VarDumper] Add period caster #23668

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ro0NL commented Jul 26, 2017

Uh oh!

nicolas-grekas commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maidmaid commented Jul 27, 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!

maidmaid Aug 3, 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!

Choose a reason for hiding this comment

Uh oh!

maidmaid commented Aug 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maidmaid Aug 6, 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!

ro0NL Aug 6, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robfrawley Aug 15, 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!

maidmaid commented Aug 14, 2017

Uh oh!

nicolas-grekas commented Aug 16, 2017

Uh oh!

maidmaid commented Aug 17, 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!

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 Aug 28, 2017

maidmaid commented Jul 25, 2017 •

edited

Loading

maidmaid commented Jul 25, 2017 •

edited

Loading

nicolas-grekas commented Jul 26, 2017 •

edited

Loading

maidmaid Aug 3, 2017 •

edited

Loading

maidmaid Aug 6, 2017 •

edited

Loading

ro0NL Aug 6, 2017 •

edited

Loading

robfrawley Aug 15, 2017 •

edited

Loading