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::setTimestamp()'s different behavior than DateTime #9106

Merged
merged 4 commits into from
Aug 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions system/I18n/TimeLegacy.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
namespace CodeIgniter\I18n;

use DateTime;
use Exception;
use ReturnTypeWillChange;

/**
* Legacy Time class.
Expand Down Expand Up @@ -47,4 +49,21 @@
class TimeLegacy extends DateTime
{
use TimeTrait;

/**
* Returns a new instance with the date set to the new timestamp.
*
* @param int $timestamp
*
* @return static
*
* @throws Exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the docs for the setTimestamp method, it mentions that an Exception may be thrown, but the method itself does not handle any exceptions directly. Should an appropriate exception be thrown?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parent class (DateTimeImmutable) throws Exception.

CI is kind of thin wrapper for PHP, so I don't think it's necessary to change PHP's behavior unless there is a need to do so.

*/
#[ReturnTypeWillChange]
public function setTimestamp($timestamp)
{
$time = date('Y-m-d H:i:s', $timestamp);

return static::parse($time, $this->timezone, $this->locale);
}
}
17 changes: 0 additions & 17 deletions system/I18n/TimeTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -685,23 +685,6 @@ public function setTimezone($timezone)
return static::createFromInstance($this->toDateTime()->setTimezone($timezone), $this->locale);
}

/**
* Returns a new instance with the date set to the new timestamp.
*
* @param int $timestamp
*
* @return static
*
* @throws Exception
*/
#[ReturnTypeWillChange]
public function setTimestamp($timestamp)
{
$time = date('Y-m-d H:i:s', $timestamp);

return static::parse($time, $this->timezone, $this->locale);
}

// --------------------------------------------------------------------
// Add/Subtract
// --------------------------------------------------------------------
Expand Down
26 changes: 21 additions & 5 deletions tests/system/I18n/TimeTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use CodeIgniter\Test\CIUnitTestCase;
use Config\App;
use DateTime;
use DateTimeImmutable;
use DateTimeZone;
use IntlDateFormatter;
use Locale;
Expand Down Expand Up @@ -719,13 +720,28 @@ public function testSetTimezone(): void

public function testSetTimestamp(): void
{
$time = Time::parse('May 10, 2017', 'America/Chicago');
$stamp = strtotime('April 1, 2017');
$time2 = $time->setTimestamp($stamp);
$time1 = Time::parse('May 10, 2017', 'America/Chicago');

$stamp = strtotime('2017-04-01'); // We use UTC as the default timezone.
$time2 = $time1->setTimestamp($stamp);

$this->assertInstanceOf(Time::class, $time2);
$this->assertNotSame($time, $time2);
$this->assertSame('2017-04-01 00:00:00', $time2->toDateTimeString());
$this->assertSame('2017-05-10 00:00:00 -05:00', $time1->format('Y-m-d H:i:s P'));
$this->assertSame('2017-03-31 19:00:00 -05:00', $time2->format('Y-m-d H:i:s P'));
}

public function testSetTimestampDateTimeImmutable(): void
{
$time1 = new DateTimeImmutable(
'May 10, 2017',
new DateTimeZone('America/Chicago')
);

$stamp = strtotime('2017-04-01'); // We use UTC as the default timezone.
$time2 = $time1->setTimestamp($stamp);

$this->assertSame('2017-05-10 00:00:00 -05:00', $time1->format('Y-m-d H:i:s P'));
$this->assertSame('2017-03-31 19:00:00 -05:00', $time2->format('Y-m-d H:i:s P'));
}

public function testToDateString(): void
Expand Down
6 changes: 6 additions & 0 deletions user_guide_src/source/changelogs/v4.6.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@ Time with Microseconds
Fixed bugs that some methods in ``Time`` to lose microseconds have been fixed.
See :ref:`Upgrading Guide <upgrade-460-time-keeps-microseconds>` for details.

Time::setTimestamp()
--------------------

``Time::setTimestamp()`` behavior has been fixed.
See :ref:`Upgrading Guide <upgrade-460-time-set-timestamp>` for details.

.. _v460-interface-changes:

Interface Changes
Expand Down
19 changes: 19 additions & 0 deletions user_guide_src/source/installation/upgrade_460.rst
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,25 @@ Also, methods that returns an ``int`` still lose the microseconds.
.. literalinclude:: upgrade_460/005.php
:lines: 2-

.. _upgrade-460-time-set-timestamp:

Time::setTimestamp() Behavior Fix
=================================

In previous versions, if you call ``Time::setTimestamp()`` on a Time instance with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it better to use this?

Suggested change
In previous versions, if you call ``Time::setTimestamp()`` on a Time instance with
In previous versions, if you call :php:meth:`Time::setTimestamp()` on a Time instance with

Copy link
Member Author

@kenjis kenjis Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:php:meth: makes a link to the class reference.
But there is no class reference for Time::setTimestamp() method.

Class reference is something like the following:
https://codeigniter4.github.io/CodeIgniter4/outgoing/view_renderer.html#namespace-CodeIgniter\View

a timezone other than the default timezone might return a Time instance with the
wrong date/time.

This bug has been fixed, and it now behaves in the same way as ``DateTimeImmutable``:

.. literalinclude:: upgrade_460/008.php
:lines: 2-

Note that if you use the default timezone, the behavior is not changed:

.. literalinclude:: upgrade_460/009.php
:lines: 2-

.. _upgrade-460-registrars-with-dirty-hack:

Registrars with Dirty Hack
Expand Down
18 changes: 18 additions & 0 deletions user_guide_src/source/installation/upgrade_460/008.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?php

use CodeIgniter\I18n\Time;

// The Application Timezone is "UTC".

// Set $time1 timezone to "America/Chicago".
$time1 = Time::parse('2024-08-20', 'America/Chicago');

// The timestamp is "2024-08-20 00:00" in "UTC".
$stamp = strtotime('2024-08-20'); // 1724112000

// But $time2 timezone is "America/Chicago".
$time2 = $time1->setTimestamp($stamp);

echo $time2->format('Y-m-d H:i:s P');
// Before: 2024-08-20 00:00:00 -05:00
// After: 2024-08-19 19:00:00 -05:00
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// After: 2024-08-19 19:00:00 -05:00
// After: 2024-08-19 19:00:00 -05:00

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The : symbols have been aligned for easier reading.

18 changes: 18 additions & 0 deletions user_guide_src/source/installation/upgrade_460/009.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?php

use CodeIgniter\I18n\Time;

// The Application Timezone is "America/Chicago".

// $time1 timezone is "America/Chicago".
$time1 = Time::parse('2024-08-20');

// The timestamp is "2024-08-20 00:00" in "America/Chicago".
$stamp = strtotime('2024-08-20'); // 1724130000

// $time2 timezone is also "America/Chicago".
$time2 = $time1->setTimestamp($stamp);

echo $time2->format('Y-m-d H:i:s P');
// Before: 2024-08-20 00:00:00 -05:00
// After: 2024-08-20 00:00:00 -05:00
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// After: 2024-08-20 00:00:00 -05:00
// After: 2024-08-20 00:00:00 -05:00

3 changes: 3 additions & 0 deletions user_guide_src/source/libraries/time.rst
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,9 @@ Returns a new instance with the date set to the new timestamp:

.. literalinclude:: time/030.php

.. note:: Prior to v4.6.0, due to a bug, this method might return incorrect
date/time. See :ref:`Upgrading Guide <upgrade-460-time-set-timestamp>` for details.

Modifying the Value
===================

Expand Down
4 changes: 3 additions & 1 deletion user_guide_src/source/libraries/time/030.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

use CodeIgniter\I18n\Time;

$time = Time::parse('May 10, 2017', 'America/Chicago');
// The Application Timezone is "America/Chicago".

$time = Time::parse('May 10, 2017');
$time2 = $time->setTimestamp(strtotime('April 1, 2017'));

echo $time->toDateTimeString(); // 2017-05-10 00:00:00
Expand Down
Loading