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

Bug: Time::createFromTimestamp() uses default timezone, not UTC for timestamp #3951

Closed
pilky opened this issue Dec 1, 2020 · 5 comments · Fixed by #4668
Closed

Bug: Time::createFromTimestamp() uses default timezone, not UTC for timestamp #3951

pilky opened this issue Dec 1, 2020 · 5 comments · Fixed by #4668
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@pilky
Copy link

pilky commented Dec 1, 2020

Describe the bug
UNIX timestamps are all based on the UTC timezone. However, Time::createFromTimestamp() uses the default timezone. This means that created dates can be off by several hours

CodeIgniter 4 version
4.0.4

Affected module(s)
I18n\Time

Expected behavior, and steps to reproduce if appropriate
The timestamp should assume it is UTC, then create the date using the supplied timezone. Instead the time is offset. For example, the following code should output "2020-12-01 12:00:00". Instead it outputs "2020-12-01 06:00:00" (due to the -6 offset for Chicago).

use CodeIgniter\I18n\Time;
date_default_timezone_set('America/Chicago');
$date = Time::createFromTimestamp(1606824000, 'utc');
echo $date->format('Y-m-d H:i:s');

I suspect the problem is that the timestamp is being converted to a date format by just using date()

Context

  • OS: macOS 10.15.7 locally & Linux web server
  • Web server: Apache 2.4.46 locally, 2.4.43 on server
  • PHP version: 7.4.9 locally, 7.4.5 on server
@pilky pilky added the bug Verified issues on the current code behavior or pull requests that will fix them label Dec 1, 2020
@iRedds
Copy link
Collaborator

iRedds commented Dec 3, 2020

The Time::createFromTimestamp() method does not convert the timestamp to the new time zone.
It just returns a DateTime instance with new timezone.

To convert use DateTime::setTimezone()

$date = Time::createFromTimestamp(1606824000);
$date->setTimezone(new DateTimeZone('utc'));

@pilky
Copy link
Author

pilky commented Dec 4, 2020

The Time::createFromTimestamp() method has a timezone argument. Going

$date = Time::createFromTimestamp(1606824000, new DateTimeZone('utc'));

should be the same as

$date = Time::createFromTimestamp(1606824000);
$date = $date->setTimezone(new DateTimeZone('utc'));

(turns out setTimezone() doesn't set the time zone of date but creates a new instance with the new timezone).

However, they give different results (the latter does give a correct result by converting the result back to UTC from the default timezone, but the former just sets the timezone property to supplied value without converting the actual value).

@MGatner
Copy link
Member

MGatner commented May 8, 2021

After some research I agree that this is a problem. We need to support int timestamps at any timezone but since this method accepts a "UNIX timestamp" it must assume the incoming int is UTC (https://en.m.wikipedia.org/wiki/Unix_time).

I believe the original assessment is correct, that conversion using date() is the culprit. We should probably use DateTime::createFromFormat('U').

Passing this a timezone is a separate issue. The provided timezone (same with create and all the static functions that rely on it) is "the timezone for the parameters", in other words, "this int timestamp is in this timezone already". So this method IMO should not be converting anything based on timezone, just passing them on (with the default being UTC instead of null).

@paulbalandan
Copy link
Member

How about using gmdate() instead of date()?

@MGatner
Copy link
Member

MGatner commented May 8, 2021

Yes that is probably the most straightforward.

MGatner added a commit to MGatner/CodeIgniter4 that referenced this issue May 11, 2021
@MGatner MGatner mentioned this issue May 11, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants