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

[VarDumper] Add period caster#23668

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-period

Aug 29, 2017

Merged

[VarDumper] Add period caster#23668

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

Aug 29, 2017

Conversation

Copy link

Contributor

maidmaid commentedJul 25, 2017•
edited
Loading

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:

carsonbot added Status: Needs Review VarDumper Feature labels

Jul 25, 2017

Copy link

ContributorAuthor

maidmaid commentedJul 25, 2017•
edited
Loading

First proposal:

maidmaid force-pushed thevardumper-period branch from13d4018 to3e25472Compare

July 25, 2017 20:36

maidmaid changed the title~~[VarDumper] Add period caster~~[WIP] [VarDumper] Add period caster

Jul 25, 2017

maidmaid force-pushed thevardumper-period branch 2 times, most recently from4c9a9ee tocce0530Compare

July 25, 2017 21:34

nicolas-grekas added this to the3.4 milestone

Jul 26, 2017

Copy link

Contributor

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

Copy link

Member

nicolas-grekas commentedJul 26, 2017•
edited
Loading

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

maidmaid force-pushed thevardumper-period branch 8 times, most recently froma22f12d to0f377e5Compare

July 27, 2017 17:33

Copy link

ContributorAuthor

maidmaid commentedJul 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:002) 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?

maidmaid changed the title~~[WIP] [VarDumper] Add period caster~~[VarDumper] Add period caster

Jul 27, 2017

ro0NL reviewed

Aug 1, 2017

View reviewed changes

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

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

Copy link

Contributor

ro0NLAug 1, 2017

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

maidmaidAug 2, 2017

Choose a reason for hiding this comment

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

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

Copy link

Contributor

ro0NLAug 2, 2017

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

maidmaidAug 3, 2017•
edited
Loading

Choose a reason for hiding this comment

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

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

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

		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'

Copy link

Contributor

ro0NLAug 1, 2017

Choose a reason for hiding this comment

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

, recurring x time/s?

Copy link

ContributorAuthor

maidmaidAug 3, 2017

Choose a reason for hiding this comment

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

Updated

maidmaid force-pushed thevardumper-period branch from0f377e5 to8d25558Compare

August 3, 2017 12:45

Copy link

ContributorAuthor

maidmaid commentedAug 3, 2017

Fail not related.

maidmaid force-pushed thevardumper-period branch from8d25558 to39f45a2Compare

August 5, 2017 18:34

ro0NL approved these changes

Aug 6, 2017

View reviewed changes

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

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

Copy link

Contributor

ro0NLAug 6, 2017

Choose a reason for hiding this comment

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

what aboutx more (see iterator_count)

Copy link

ContributorAuthor

maidmaidAug 6, 2017•
edited
Loading

Choose a reason for hiding this comment

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

Yes, good idea. Butiterator_count() loops on all dates, what we want to avoid. We can findx withrecurrences - limit in recurrences case and(end - start - limit) / interval in date case. WDYT?

Copy link

ContributorAuthor

maidmaidAug 7, 2017

Choose a reason for hiding this comment

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

I addedx more ;) I comparediterator_count() and my method with 1M of reccurences:

4160.1 ms withiterator_count()
0.1 ms with my method

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'

Copy link

Contributor

ro0NLAug 6, 2017•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

maidmaidAug 6, 2017

Choose a reason for hiding this comment

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

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

nicolas-grekas reviewed

Aug 7, 2017

View reviewed changes

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

		($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)));

Copy link

Member

nicolas-grekasAug 7, 2017

Choose a reason for hiding this comment

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

"\n" instead of PHP_EOL

Copy link

ContributorAuthor

maidmaidAug 7, 2017

Choose a reason for hiding this comment

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

Updated (Why exactly do you ask this change?)

Copy link

Contributor

robfrawleyAug 15, 2017•
edited
Loading

Choose a reason for hiding this comment

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

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 force-pushed thevardumper-period branch 2 times, most recently from84f73dd to915fd4aCompare

August 7, 2017 11:20

Copy link

ContributorAuthor

maidmaid commentedAug 14, 2017

Can you review this PR please? :)

Copy link

Member

nicolas-grekas commentedAug 16, 2017

is the screenshot up to date?

Copy link

ContributorAuthor

maidmaid commentedAug 17, 2017

Screenshot updated in PR description.

nicolas-grekas reviewed

Aug 28, 2017

View reviewed changes

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

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

Copy link

Member

nicolas-grekasAug 28, 2017

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

maidmaidAug 28, 2017

Choose a reason for hiding this comment

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

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

Copy link

Member

nicolas-grekasAug 28, 2017

Choose a reason for hiding this comment

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

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

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

Copy link

Member

nicolas-grekasAug 28, 2017

Choose a reason for hiding this comment

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

what happens before 5.6.5?

Copy link

ContributorAuthor

maidmaidAug 28, 2017

Choose a reason for hiding this comment

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

getStartDate(),getDateInterval() andgetEndDate() methods were added in 5.6.5.

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

		$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) {

Copy link

Member

nicolas-grekasAug 28, 2017

Choose a reason for hiding this comment

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

if (3 === $i) {

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

		$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

Copy link

Member

nicolas-grekasAug 28, 2017

Choose a reason for hiding this comment

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

$p->recurrences - $i

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

		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

Copy link

Member

nicolas-grekasAug 28, 2017

Choose a reason for hiding this comment

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

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

Add period caster

4c4c398

maidmaid force-pushed thevardumper-period branch from915fd4a to4c4c398Compare

August 28, 2017 18:44

Copy link

ContributorAuthor

maidmaid commentedAug 28, 2017

Changes done + screenshot updated.

nicolas-grekas approved these changes

Aug 28, 2017

View reviewed changes

Copy link

Member

nicolas-grekas commentedAug 29, 2017

Thank you@maidmaid.

nicolas-grekas merged commit4c4c398 intosymfony:3.4

Aug 29, 2017

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

Copy link

Member

nicolas-grekas commentedAug 29, 2017

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

Copy link

ContributorAuthor

maidmaid commentedAug 29, 2017

Ok, I check that now.

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

nicolas-grekas added a commit that referenced this pull request

Aug 29, 2017

minor#24023[VarDumper] Fix segfault in period caster (maidmaid)

cba3393

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#v565Commits-------d84e9c8 Fix segfault in period caster

maidmaid mentioned this pull request

Aug 30, 2017

[VarDumper] Prepare period caster for 4.0#24034

Merged

nicolas-grekas added a commit that referenced this pull request

Aug 31, 2017

minor#24034[VarDumper] Prepare period caster for 4.0 (maidmaid)

d3630c1

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

javiereguiluz mentioned this pull request

Sep 10, 2017

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

Closed

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

5 participants

Movatterモバイル変換

Uh oh!

[VarDumper] Add period caster#23668

[VarDumper] Add period caster#23668

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ro0NL commentedJul 26, 2017

Uh oh!

nicolas-grekas commentedJul 26, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

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

maidmaidAug 3, 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!

Choose a reason for hiding this comment

Uh oh!

maidmaid commentedAug 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maidmaidAug 6, 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!

ro0NLAug 6, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robfrawleyAug 15, 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!

maidmaid commentedAug 14, 2017

Uh oh!

nicolas-grekas commentedAug 16, 2017

Uh oh!

maidmaid commentedAug 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 commentedAug 28, 2017

maidmaid commentedJul 25, 2017•
edited
Loading

maidmaid commentedJul 25, 2017•
edited
Loading

nicolas-grekas commentedJul 26, 2017•
edited
Loading

maidmaidAug 3, 2017•
edited
Loading

maidmaidAug 6, 2017•
edited
Loading

ro0NLAug 6, 2017•
edited
Loading

robfrawleyAug 15, 2017•
edited
Loading