search

codeigniter4 / CodeIgniter4

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
5.35k stars 1.9k forks source link

\I18n\Time setTimezone does not work as expected #1807

Closed nowackipawel closed 4 years ago

nowackipawel commented 5 years ago

I looked into \Time code and.... it does not look good at all.

Additionally setTimezone does not work as excepted .


            $t0 = new \CodeIgniter\I18n\Time($transaction->tra_created_at);
            $t1 = $t0->setTimezone('America/Los_Angeles');
            d($t0, $t1);

https://codeigniter4.github.io/CodeIgniter4/libraries/time.html?highlight=settimezone

gives me:

https://screenshots.firefox.com/QrGBqc9Is0QPTGKo/dev.zdamyto.com my $appTimezone === 'UTC'

I tried to fix it, but this code looks completely messy.

lonnieezell commented 5 years ago

Any particular concerns, or just a general thumbing your nose at the code?

lonnieezell commented 5 years ago

What exactly were you trying to fix? All times are converted internally to UTC to handle any conversions, and, IIRC, might have been how the DateTime instance that it's extending handles things internally. It's been a while since I've looked at it.

If you print the time out does it give the correct timezone? The tests seem to indicate that it's working as expected.

lonnieezell commented 5 years ago

With the current version of the code, here's what I get: 

$t0 = new Time('now', 'UTC');
$t1 = $t0->setTimezone(new \DateTimeZone('America/Chicago'));
d($t0, $t1);

// Gives: 
$t0 CodeIgniter\I18n\Time (6) 2019-03-11 03:00:40.329165+00:00 UTC
$t1 CodeIgniter\I18n\Time (6) 2019-03-11 03:00:40-05:00 CDT

and setting $appTimezone to UTC I get: 

$t0 = new Time('2016-02-23 12:14:23');
$t1 = $t0->setTimezone(new \DateTimeZone('America/Chicago'));

$t0 CodeIgniter\I18n\Time (6) 2016-02-23 12:14:23+00:00 UTC
$t1 CodeIgniter\I18n\Time (6) 2016-02-23 12:14:23-06:00 CST

Both of which act as expected.

Is your code up to date? Though we haven't made changes to that in quite a while, I don't think. If it is reasonably up to date then what environment is that running on? OS? Server? PHP version? Can you verify what the system thinks date_default_timezone_get on your environment when you're trying to run that?

nowackipawel commented 5 years ago

Any particular concerns, or just a general thumbing your nose at the code?

Mainly thumbing nose, but I cannot understand why in almost all the cases we are returning new instance instead of working on the one which was passed / using to make operation on it. Not sure about performance.

What exactly were you trying to fix? All times are converted internally to UTC to handle any conversions, and, IIRC, might have been how the DateTime instance that it's extending handles things internally. It's been a while since I've looked at it.

If you print the time out does it give the correct timezone? The tests seem to indicate that it's working as expected.

I didn't get right time when I am printing properties like created_at / updated_at of Entities or when I am using format() method. I think that dates should be casted to appTimezone and imho they didn't.

According to: your test I am working on:

The original issue was because I've passed string instead of \DateTimeZone instance - that's is because I looked many times in \I18n\Time code and internally it is using strings or \DateTimeZone interchangeably - explanation bellow.

///DO NOT READ THIS PART - BEGIN Dates in my code do not change if I modify: App->appTimezone and America/Los_Angeles in the code bellow: 

            $t0 = new \CodeIgniter\I18n\Time($transaction->tra_created_at);
            $t1 = $t0->setTimezone('America/Los_Angeles');
            d($t0, $t1);

/// DO NOT READ THIS PART - END

Hovewer I am experiencing another issue which is related to preious one and mentioned it above. Using code like this:

$t = new \DateTime('2019-03-10 00:00:00+01:00 CET'); // OR $t = new \DateTime('2019-03-10 00:00:00'); 
$t->setTimezone(new \DateTimeZone('AEDT'));
var_dump($t, $t->format('Y-m-d H:i:s'));

I get natural and expected results - time is converted to AEDT:

object(DateTime)#1 (3) {
  ["date"]=>
  string(26) "2019-03-10 18:00:00.000000"
  ["timezone_type"]=>
  int(2)
  ["timezone"]=>
  string(4) "AEDT"
}
string(19) "2019-03-10 18:00:00" // TIME IS CONVERTED TO AEDT

When I am trying to do the same with CI and \I18n\Time:

$t = new \CodeIgniter\I18n\Time('2019-03-10 00:00:00+01:00 CET'); // OR '2019-03-10 00:00:00' ; OR '2019-03-10 00:00:00', 'UTC'
$t->setTimezone(new \DateTimeZone('AEDT'));
var_dump($t, $t->format('Y-m-d H:i:s'));

I get:

object(CodeIgniter\I18n\Time)#76 (6) {
  ["timezone":protected]=>
  object(DateTimeZone)#83 (2) {
    ["timezone_type"]=>
    int(3)
    ["timezone"]=>
    string(13) "Europe/Berlin"
  }
  ["locale":protected]=>
  string(11) "en_US_POSIX"
  ["toStringFormat":protected]=>
  string(19) "yyyy-MM-dd HH:mm:ss"
  ["date"]=>
  string(26) "2019-03-10 00:00:00.000000"
  ["timezone_type"]=>
  int(1)
  ["timezone"]=>
  string(6) "+01:00"
}
string(19) "2019-03-10 00:00:00" // TIME IS NOT CONVERTED TO AEDT
lonnieezell commented 5 years ago

Mainly thumbing nose, but I cannot understand why in almost all the cases we are returning new instance instead of working on the one which was passed / using to make operation on it. Not sure about performance.

That's not a mess, it's a conscious design choice :) There's a large swath of PHP devs that feel that all objects should be immutable. While I'm not one of those, somewhere during the development of this library it started making sense for it to be an immutable class. Don't remember the specifics. That was awhile ago. I just verified and the first line of the docs does state it is immutable.

Because it's immutable and doesn't make changes on the original class, your final example won't work because you're not capturing the returned new Time instance. It would need to be like: 

$t = new \CodeIgniter\I18n\Time('2019-03-10 00:00:00+01:00 CET'); // OR '2019-03-10 00:00:00' ; OR '2019-03-10 00:00:00', 'UTC'
$t = $t->setTimezone(new \DateTimeZone('AEDT'));

The original issue was because I've passed string instead of \DateTimeZone instance - that's is because I looked many times in \I18n\Time code and internally it is using strings or \DateTimeZone interchangeably - explanation bellow.

According to the docs passing a string should work just fine. If it's not then that's a bug. I'll check into that.

nowackipawel commented 5 years ago

I didn't notice it is immutable in docs, but in fact I didn't check for it and only for specific methods explanations. However I think I write my tests in a way as you pointed it out. I will pay more attention to docs.

Thank you for wide explanation.

According to your hints about how to use it. I've checked it and:

option1:
            $t0 = new \CodeIgniter\I18n\Time('2019-03-10 02:11:11', 'UTC');
            $t1 = $t0->setTimezone(new \DateTimeZone('America/Los_Angeles'));
            var_dump($t0, $t1, $t1->format('Y-m-d H:i:s'));
option2:
            $t0 = new \CodeIgniter\I18n\Time('2019-03-10 02:11:11+01:00 CET');
            $t1 = $t0->setTimezone(new \DateTimeZone('America/Los_Angeles'));
            var_dump($t0, $t1, $t1->format('Y-m-d H:i:s'));
option3:
            $t0 = new \CodeIgniter\I18n\Time('2019-03-10 02:11:11+01:00 CET');
            $t1 = $t0->setTimezone('America/Los_Angeles'); // no \DateTimeZone
            var_dump($t0, $t1, $t1->format('Y-m-d H:i:s'));
option4:
            $t0 = new \CodeIgniter\I18n\Time('2019-03-10 02:11:11+01:00 CET');
            $t1 = $t0->setTimezone(new \DateTimeZone('Asia/Bangkok')); // or without \DateTimeZone
            var_dump($t0, $t1, $t1->format('Y-m-d H:i:s'));

gives me:

object(CodeIgniter\I18n\Time)#77 (6) {
  ["timezone":protected]=>
  object(DateTimeZone)#84 (2) {
    ["timezone_type"]=>
    int(3)
    ["timezone"]=>
    string(3) "UTC"
  }
  ["locale":protected]=>
  string(11) "en_US_POSIX"
  ["toStringFormat":protected]=>
  string(19) "yyyy-MM-dd HH:mm:ss"
  ["date"]=>
  string(26) "2019-03-10 02:11:11.000000"
  ["timezone_type"]=>
option1:  int(3) / option2&3: int(1) / 
  ["timezone"]=>
option1: "UTC" / option2&3: "+01:00" /
}
object(CodeIgniter\I18n\Time)#88 (6) {
  ["timezone":protected]=>
  object(DateTimeZone)#87 (2) {
    ["timezone_type"]=>
    int(3)
    ["timezone"]=>
    string(19) "America/Los_Angeles"
  }
  ["locale":protected]=>
  string(11) "en_US_POSIX"
  ["toStringFormat":protected]=>
  string(19) "yyyy-MM-dd HH:mm:ss"
  ["date"]=>
  string(26) "2019-03-10 03:11:11.000000"
  ["timezone_type"]=>
  int(3)
  ["timezone"]=>
  string(19) "America/Los_Angeles"
}
string(19) "2019-03-10 03:11:11"

for option4 there is more changes:

object(CodeIgniter\I18n\Time)#77 (6) {
  ["timezone":protected]=>
  object(DateTimeZone)#84 (2) {
    ["timezone_type"]=>
    int(3)
    ["timezone"]=>
    string(3) "UTC"
  }
  ["locale":protected]=>
  string(11) "en_US_POSIX"
  ["toStringFormat":protected]=>
  string(19) "yyyy-MM-dd HH:mm:ss"
  ["date"]=>
  string(26) "2019-03-10 02:11:11.000000"
  ["timezone_type"]=>
  int(1)
  ["timezone"]=>
  string(6) "+01:00"
}
object(CodeIgniter\I18n\Time)#88 (6) {
  ["timezone":protected]=>
  object(DateTimeZone)#87 (2) {
    ["timezone_type"]=>
    int(3)
    ["timezone"]=>
    string(12) "Asia/Bangkok"
  }
  ["locale":protected]=>
  string(11) "en_US_POSIX"
  ["toStringFormat":protected]=>
  string(19) "yyyy-MM-dd HH:mm:ss"
  ["date"]=>
  string(26) "2019-03-10 02:11:11.000000"
  ["timezone_type"]=>
  int(3)
  ["timezone"]=>
  string(12) "Asia/Bangkok"
}
string(19) "2019-03-10 02:11:11"

so date looks like it is converted between UTC and my "PHP set: Default timezone | Europe/Berlin" option 1,2,3 and there is no change in date for option4.

jim-parry commented 5 years ago

@nowackipawel This is a long & convoluted thread. Has the issue been resolved? Can the issue be closed?

atishhamte commented 5 years ago

@nowackipawel, any update on this?

nowackipawel commented 5 years ago

@jim-parry , @atishamte I made PR which was good in my opinion, it allows user to select type of class (DateTime or \I18\Time) for storing datetime properties (#1815) , but I have to agree with @lonnieezell doubts. The most important was that Entity class has to store datetime as \I18\Time. There are to solutions:

lonnieezell commented 5 years ago

I think the question, though, is with the 4 examples, are they all working as expected within how the framework is designed (nothing with extending, etc)...

MGatner commented 4 years ago

Most of this thread is opinions about immutable classes, which won't change. I confirmed that "Option 4" above now works as expected (changes the time). Since all other tests are passing this is officially resolved.