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

fix: Time::createFromTimestamp() sets incorrect time when specifying timezone #5588

Merged
merged 3 commits into from
Jan 19, 2022

Conversation

totoprayogo1916
Copy link
Contributor

@totoprayogo1916 totoprayogo1916 commented Jan 17, 2022

Description
previously not updating date even though time zone has been set

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@totoprayogo1916 totoprayogo1916 force-pushed the update/Time branch 2 times, most recently from a2bd954 to 7602cb4 Compare January 17, 2022 16:36
@kenjis kenjis added tests needed Pull requests that need tests breaking change Pull requests that may break existing functionalities and removed breaking change Pull requests that may break existing functionalities labels Jan 18, 2022
@kenjis
Copy link
Member

kenjis commented Jan 18, 2022

@totoprayogo1916 I'm not sure what the intent of this PR is.
What do you mean Update Time::createFromTimestamp() ?
Can you add test code?

@totoprayogo1916
Copy link
Contributor Author

case:

$time = new Time('now');
$timestamp = $time->timestamp;

$new_time = Time::createFromTimestamp($timestamp);
$new_time_timezone = Time::createFromTimestamp($timestamp, config('App')->appTimezone);

var_dump(
    $time->toDateTimeString(),
    $new_time->toDateTimeString(),
    $new_time_timezone->toDateTimeString()
);

and the results like:
image

The date doesn't change even though the timezone has been added.

@kenjis
Copy link
Member

kenjis commented Jan 18, 2022

@totoprayogo1916 This is complicated timezone issue.
You must add test code to show the specification.

Note, we expect all code changes or bug-fixes to be accompanied by one or more tests added to our test suite to prove the code works. If pull requests are not accompanied by relevant tests, they will likely be closed.
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#contributions

The date doesn't change even though the timezone has been added.

What's wrong with it?

@totoprayogo1916
Copy link
Contributor Author

What's wrong with it?

Test added

@kenjis
Copy link
Member

kenjis commented Jan 18, 2022

The time is changed when specify timezone.

$time = new Time('2022/01/18 00:00:00');
$new_time = Time::createFromTimestamp($time->timestamp);
$new_time_timezone = Time::createFromTimestamp($time->timestamp, 'Asia/Jakarta');

var_dump(
    $time->getTimestamp(),
    $new_time->getTimestamp(),
    $new_time_timezone->getTimestamp()
);
int(1642485600)
int(1642485600)
int(1642460400) // <- changed

@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them and removed breaking change Pull requests that may break existing functionalities labels Jan 18, 2022
@totoprayogo1916
Copy link
Contributor Author

int(1642485600)
int(1642485600)
int(1642460400) // <- changed

timestamp changed, but Date not.
I posted yesterday here: https://forum.codeigniter.com/thread-81033.html

@kenjis kenjis changed the title Update Time::createFromTimestamp() fix: Time::createFromTimestamp() sets incorrect time when specifying timezone Jan 18, 2022
@totoprayogo1916
Copy link
Contributor Author

var_dump(
    $time,
    $time->timestamp,
    $new_time,
    $new_time->timestamp,
    $new_time_timezone,
    $new_time_timezone->timestamp
);

image

system/I18n/Time.php Outdated Show resolved Hide resolved
@kenjis
Copy link
Member

kenjis commented Jan 18, 2022

@totoprayogo1916

timestamp changed, but Date not.

I got what you mean when I saw your var_dump output.
Thanks.

@kenjis
Copy link
Member

kenjis commented Jan 18, 2022

The timestamp is always based on UTC, so the current behavior is a bug, right?

@kenjis kenjis removed the tests needed Pull requests that need tests label Jan 18, 2022
previously not updating date even though time zone has been set
Toto Prayogo and others added 2 commits January 18, 2022 17:36
apply suggestion

Co-authored-by: kenjis <kenji.uui@gmail.com>
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 this pull request may close these issues.

3 participants