Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding to a time provides an inconsistent result #88

Closed
gregrobson opened this issue Jan 29, 2014 · 2 comments · Fixed by #548
Closed

Adding to a time provides an inconsistent result #88

gregrobson opened this issue Jan 29, 2014 · 2 comments · Fixed by #548

Comments

@gregrobson
Copy link

Firstly I love the library (especially as part of Laravel) and this is a PHP issue more than a Carbon issue.

When adding/subtracting times for a value against a Timezone other than UTC, the add/subtract features do not take daylight saving into consideration

<?php
// Clocks go forward on this day: 00:59:59 > 02:00:00
$a = new Carbon('2014-03-30 00:00:00', 'Europe/London'); 
$b = $a->copy();
$b->addHours(24);
echo $b; // Produces 2014-03-31 00:00:00, expected 2014-03-31 01:00:00
echo $a->diffInHours($b); // Produces 23, expected 24.

It seems that PHP's modify() function ignores daylight saving values entirely. The documentation often mentions that comparisons are always done in UTC, so I would expect the additions to add UTC units of time as well.

This also leads the possibility of time loops!
http://uk3.php.net/manual/en/datetime.modify.php#113549

@gregrobson
Copy link
Author

My fix for this

<?php
$a = new Carbon('2014-03-30 00:00:00', 'Europe/London'); 
$b = $a->copy();

$b->setTimezone('UTC'); // Change to UTC for correct addition
$b->addHours(24);
$b->setTimezone('Europe/London'); // Change back for correct display value.

echo $b; // Produces 2014-03-31 01:00:00
echo $a->diffInHours($b); // Produces 24

kylekatarnls added a commit to kylekatarnls/Carbon that referenced this issue Jan 31, 2014
[Issue 88 : Adding to a time provides an inconsistent result](briannesbitt#88)
@kylekatarnls
Copy link
Collaborator

It seems to be an error due to the PHP DateTime object, so I propose to put you hack in a overrided modify method.
Suggested pull request

Ovsyanka added a commit to Ovsyanka/Carbon that referenced this issue Jan 15, 2018
The realisation of initial issue briannesbitt#88 is wrong. The problem does not relies to `local` / `not local` timezone. Php handles date changes similar for both cases. So in fact changes was made 'fixes' non-local dates but local dates works same as before.
And it 'fixes' the problem and not actually fixes it because in fact the PHP handling of DST changing in non-UTC dates is quite right. If developer don't want to adjusting timestamp to DST change he should use UTC timezone.

Next commits reverted by hand because conflicts in automatic revert.

* Commit 2cc6988  Fix CS	07.12.2015 13:04	Lucas Michot
* Commit 5394301  Fix issue 88	07.12.2015 12:55	Lucas Michot
Ovsyanka added a commit to Ovsyanka/Carbon that referenced this issue Jan 15, 2018
The realisation of initial issue briannesbitt#88 is wrong. The problem does not relies to `local` / `not local` timezone. Php handles date changes similar for both cases. So in fact changes was made 'fixes' non-local dates but local dates works same as before.
And it 'fixes' the problem and not actually fixes it because in fact the PHP handling of DST changing in non-UTC dates is quite right. If developer don't want to adjusting timestamp to DST change he should use UTC timezone.

Next commits reverted by hand because conflicts in automatic revert.

* Commit 2cc6988  Fix CS	07.12.2015 13:04	Lucas Michot
* Commit 5394301  Fix issue 88	07.12.2015 12:55	Lucas Michot
Ovsyanka added a commit to Ovsyanka/Carbon that referenced this issue Jan 15, 2018
The realisation of initial issue briannesbitt#88 is wrong. The problem does not relies to `local` / `not local` timezone. Php handles date changes similar for both cases. So in fact changes was made 'fixes' non-local dates but local dates works same as before.
And it 'fixes' the problem and not actually fixes it because in fact the PHP handling of DST changing in non-UTC dates is quite right. If developer don't want to adjusting timestamp to DST change he should use UTC timezone.

Next commits reverted by hand because conflicts in automatic revert.

* Commit 2cc6988  Fix CS	07.12.2015 13:04	Lucas Michot
* Commit 5394301  Fix issue 88	07.12.2015 12:55	Lucas Michot
Ovsyanka added a commit to Ovsyanka/Carbon that referenced this issue Jan 15, 2018
…about issue briannesbitt#88

The realisation of initial issue briannesbitt#88 is wrong. The problem does not relies to `local` / `not local` timezone. Php handles date changes similar for both cases. So in fact changes was made 'fixes' non-local dates but local dates works same as before.
And it 'fixes' the problem and not actually fixes it because in fact the PHP handling of DST changing in non-UTC dates is quite right. If developer don't want to adjusting timestamp to DST change he should use UTC timezone.

Next commits reverted by hand because conflicts in automatic revert.

* Commit 2cc6988  Fix CS	07.12.2015 13:04	Lucas Michot
* Commit 5394301  Fix issue 88	07.12.2015 12:55	Lucas Michot
Ovsyanka added a commit to Ovsyanka/Carbon that referenced this issue Feb 13, 2018
…about issue briannesbitt#88

The realisation of initial issue briannesbitt#88 is wrong. The problem does not relies to `local` / `not local` timezone. Php handles date changes similar for both cases. So in fact changes was made 'fixes' non-local dates but local dates works same as before.
And it 'fixes' the problem and not actually fixes it because in fact the PHP handling of DST changing in non-UTC dates is quite right. If developer don't want to adjusting timestamp to DST change he should use UTC timezone.

Next commits reverted by hand because conflicts in automatic revert.

* Commit 2cc6988  Fix CS	07.12.2015 13:04	Lucas Michot
* Commit 5394301  Fix issue 88	07.12.2015 12:55	Lucas Michot
Ovsyanka added a commit to Ovsyanka/Carbon that referenced this issue Feb 13, 2018
…about issue briannesbitt#88

The realisation of initial issue briannesbitt#88 is wrong. The problem does not relies to `local` / `not local` timezone. Php handles date changes similar for both cases. So in fact changes was made 'fixes' non-local dates but local dates works same as before.
And it 'fixes' the problem and not actually fixes it because in fact the PHP handling of DST changing in non-UTC dates is quite right. If developer don't want to adjusting timestamp to DST change he should use UTC timezone.

The unit tests added to assure default php behavior.

Next commits reverted by hand because conflicts in automatic revert.

* Commit 2cc6988  Fix CS	07.12.2015 13:04	Lucas Michot
* Commit 5394301  Fix issue 88	07.12.2015 12:55	Lucas Michot
Ovsyanka added a commit to Ovsyanka/Carbon that referenced this issue Feb 13, 2018
…about issue briannesbitt#88

The realisation of initial issue briannesbitt#88 is wrong. The problem does not relies to `local` / `not local` timezone. Php handles date changes similar for both cases. So in fact changes was made 'fixes' non-local dates but local dates works same as before.
And it 'fixes' the problem and not actually fixes it because in fact the PHP handling of DST changing in non-UTC dates is quite right. If developer don't want to adjusting timestamp to DST change he should use UTC timezone.

The unit tests added to assure default php behavior.

Next commits reverted by hand because conflicts in automatic revert.

* Commit 2cc6988  Fix CS	07.12.2015 13:04	Lucas Michot
* Commit 5394301  Fix issue 88	07.12.2015 12:55	Lucas Michot
Ovsyanka added a commit to Ovsyanka/Carbon that referenced this issue Feb 26, 2018
…about issue briannesbitt#88

The realisation of initial issue briannesbitt#88 is wrong. The problem does not relies to `local` / `not local` timezone. Php handles date changes similar for both cases. So in fact changes was made 'fixes' non-local dates but local dates works same as before.
And it 'fixes' the problem and not actually fixes it because in fact the PHP handling of DST changing in non-UTC dates is quite right. If developer don't want to adjusting timestamp to DST change he should use UTC timezone.

Next commits reverted by hand because conflicts in automatic revert.

* Commit 2cc6988  Fix CS	07.12.2015 13:04	Lucas Michot
* Commit 5394301  Fix issue 88	07.12.2015 12:55	Lucas Michot
Ovsyanka added a commit to Ovsyanka/Carbon that referenced this issue Feb 26, 2018
…about issue briannesbitt#88

The realisation of initial issue briannesbitt#88 is wrong. The problem does not relies to `local` / `not local` timezone. Php handles date changes similar for both cases. So in fact changes was made 'fixes' non-local dates but local dates works same as before.
And it 'fixes' the problem and not actually fixes it because in fact the PHP handling of DST changing in non-UTC dates is quite right. If developer don't want to adjusting timestamp to DST change he should use UTC timezone.

The unit tests added to assure default php behavior.

Next commits reverted by hand because conflicts in automatic revert.

* Commit 2cc6988  Fix CS	07.12.2015 13:04	Lucas Michot
* Commit 5394301  Fix issue 88	07.12.2015 12:55	Lucas Michot
Ovsyanka added a commit to Ovsyanka/Carbon that referenced this issue Feb 26, 2018
…about issue briannesbitt#88

The realisation of initial issue briannesbitt#88 is wrong. The problem does not relies to `local` / `not local` timezone. Php handles date changes similar for both cases. So in fact changes was made 'fixes' non-local dates but local dates works same as before.
And it 'fixes' the problem and not actually fixes it because in fact the PHP handling of DST changing in non-UTC dates is quite right. If developer don't want to adjusting timestamp to DST change he should use UTC timezone.

The unit tests added to assure default php behavior.

Next commits reverted by hand because conflicts in automatic revert.

* Commit 2cc6988  Fix CS	07.12.2015 13:04	Lucas Michot
* Commit 5394301  Fix issue 88	07.12.2015 12:55	Lucas Michot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants