-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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::now() does not respect timezone when using setTestNow() #6752
Conversation
system/I18n/Time.php
Outdated
$timezone = $timezone ?: static::$testNow->getTimezone(); | ||
$time = static::$testNow->format('Y-m-d H:i:s'); | ||
if ($timezone !== null) { | ||
$testNow = static::$testNow->setTimezone($timezone); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Time
extends DateTime
, not DateTimeImmutable
, so it is mutable. In this case setTimezone
is an internal method that returns a new instance, but I had to go look that up to be sure. I would still prefer to see this cloned for clarity, since the parent DateTime::setTimezone()
would actually effect a change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CodeIgniter provides a fully-localized, immutable, date/time class that is built on PHP’s DateTime object, but uses the Intl extension’s features to convert times across timezones and display the output correctly for different locales. https://codeigniter4.github.io/CodeIgniter4/libraries/time.html
Time is immutable. If not, it is a bug. We should fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not. Any of the following methods will mutate the current object:
- __construct()
- add()
- modify()
- setDate()
- setISODate()
- setTime()
- setTimestamp()
- sub()
setTimezone()
is an exception because we explicitly override the native method, but this is not apparent to a reviewer without looking at the class API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, no! It is a big bug.
setTimestamp()
is also overridden. But other methods are not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Time
should have extended DateTimeImmutable
if it was going to be immutable. Seems like a pretty big oversight there, but maybe immutability was added later to the User Guide? This all predates me.
For now I guess we change the User Guide? Changing the underlying class at this point seems to big a break. Alternatively we could override all the mutable methods but that could also be breaking and seems silly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been stated immutable since v4.0.0-alpha.1.
CodeIgniter provides a fully-localized, immutable, date/time class that is built on PHP's DateTime object, but uses the Intl extension's features to convert times across timezones and display the output correctly for different locales. This class is the Time class and lives in the CodeIgniter\I18n namespace.
https://github.com/codeigniter4/CodeIgniter4/blob/v4.0.0-alpha.1/user_guide_src/source/libraries/time.rst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though it is a breaking change, but it seems we must fix it.
The current Time has methods of immutable and methods of mutable.
It is extremely bad design and difficult to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I guess we change the User Guide?
It is not good. Because all methods implemented in Time
are immutable.
I sent a PR for true immutable Time: #6771
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fix this in 4.3
that Time is fully immutable.
9e883d7
to
f34f8e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That took quite a detour 😅
Description
Checklist: