Skip to content

Commit

Permalink
Merge pull request #1129 from kylekatarnls/remove-modify
Browse files Browse the repository at this point in the history
Remove modify and fix diffInSeconds
  • Loading branch information
kylekatarnls authored Feb 28, 2018
2 parents bee0721 + 187f9bb commit 4a874a3
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 23 deletions.
29 changes: 6 additions & 23 deletions src/Carbon/Carbon.php
Original file line number Diff line number Diff line change
Expand Up @@ -2801,9 +2801,13 @@ public function diffInMinutes(self $dt = null, $abs = true)
public function diffInSeconds(self $dt = null, $abs = true)
{
$dt = $dt ?: static::now($this->getTimezone());
$value = $dt->getTimestamp() - $this->getTimestamp();
$diff = $this->diff($dt);
$value = $diff->days * static::HOURS_PER_DAY * static::MINUTES_PER_HOUR * static::SECONDS_PER_MINUTE +
$diff->h * static::MINUTES_PER_HOUR * static::SECONDS_PER_MINUTE +
$diff->i * static::SECONDS_PER_MINUTE +
$diff->s;

This comment has been minimized.

Copy link
@loonytoons

loonytoons Mar 6, 2018

I think this has caused an issue for leap years. Just investigating though

This comment has been minimized.

Copy link
@kylekatarnls

kylekatarnls Mar 6, 2018

Author Collaborator

Hi, removing modify override was necessary because it caused timezone big problems. The change you point align diffs on PHP DateTime native results after modify calls, it have an impact on diffInSeconds, diffInMinutes and diffInHours (none other methods results). It can give unexpected results in edge cases (daylight saving time change days in timezones with DST).

That's why I plan to add dedicated methods for differences calculated based on timestamps: #1148

But leap year should not have problem with that except if you do some "bad" things like adding/subtracting multi-days range by adding/subtracting seconds to timestamps.


return $abs ? abs($value) : $value;
return $abs || !$diff->invert ? $value : -$value;
}

/**
Expand Down Expand Up @@ -3383,27 +3387,6 @@ public function isBirthday($dt = null)
return $this->isSameAs('md', $dt);
}

/**
* Consider the timezone when modifying the instance.
*
* @param string $modify
*
* @return static
*/
public function modify($modify)
{
if ($this->local) {
return parent::modify($modify);
}

$timezone = $this->getTimezone();
$this->setTimezone('UTC');
$instance = parent::modify($modify);
$this->setTimezone($timezone);

return $instance;
}

/**
* Return a serialized string of the instance.
*
Expand Down
66 changes: 66 additions & 0 deletions tests/Carbon/ModifyNearDSTChangeTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
<?php

/*
* This file is part of the Carbon package.
*
* (c) Brian Nesbitt <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Tests\Carbon;

use Carbon\Carbon;
use Tests\AbstractTestCase;

class ModifyNearDSTChangeTest extends AbstractTestCase
{
/**
* Tests transition through DST change hour in non default timezone
*
* @dataProvider getTransitionTests
*/
public function testTransitionInNonDefaultTimezone($dateString, $addHours, $expected)
{
date_default_timezone_set('Europe/london');
$date = Carbon::parse($dateString, 'America/New_York');
$date->addHours($addHours);
$this->assertSame($expected, $date->format('c'));
}

/**
* Tests transition through DST change hour in default timezone
*
* @dataProvider getTransitionTests
*/
public function testTransitionInDefaultTimezone($dateString, $addHours, $expected)
{
date_default_timezone_set('America/New_York');
$date = Carbon::parse($dateString, 'America/New_York');
$date->addHours($addHours);
$this->assertSame($expected, $date->format('c'));
}

public function getTransitionTests()
{

// arguments:
// - Date string to Carbon::parse in America/New_York.
// - Hours to add
// - Resulting string in 'c' format
$tests = array(
// testForwardTransition
// When standard time was about to reach 2010-03-14T02:00:00-05:00 clocks were turned forward 1 hour to
// 2010-03-14T03:00:00-04:00 local daylight time instead
array('2010-03-14T00:00:00', 24, '2010-03-15T00:00:00-04:00'),

// testBackwardTransition
// When local daylight time was about to reach 2010-11-07T02:00:00-04:00 clocks were turned backward 1 hour
// to 2010-11-07T01:00:00-05:00 local standard time instead
array('2010-11-07T00:00:00', 24, '2010-11-08T00:00:00-05:00'),
);

return $tests;
}
}
3 changes: 3 additions & 0 deletions tests/Carbon/ModifyTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,8 @@ public function testTimezoneModify()
$b = $a->copy();
$b->addHours(24);
$this->assertSame(24, $a->diffInHours($b));
$this->assertSame(24, $a->diffInHours($b, false));
$this->assertSame(24, $b->diffInHours($a));
$this->assertSame(-24, $b->diffInHours($a, false));
}
}

0 comments on commit 4a874a3

Please sign in to comment.