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

IBX-300: Added timezone offset to timestamp #181

Merged
merged 9 commits into from
May 12, 2021

Conversation

mateuszdebinski
Copy link
Contributor

@mateuszdebinski mateuszdebinski commented May 6, 2021

Question Answer
JIRA issue IBX-300
Type bug
Target eZ Platform version v3.2
BC breaks no
Doc needed no

Related PR: ezsystems/ezplatform-admin-ui#1752

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (master for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ezsystems/php-dev-team).

@mateuszdebinski mateuszdebinski requested a review from a team May 6, 2021 19:49
@Steveb-p
Copy link
Contributor

Steveb-p commented May 6, 2021

I've noticed that Value::fromTimestamp() method constructs DateTime objects by passing timestamps directly into constructor method. Shouldn't this be changed into:

    public static function fromTimestamp($timestamp)
    {
        try {
-            return new static(new DateTime("@{$timestamp}"));
+            $datetime = new DateTime();
+            $datetime->setTimestamp($timestamp);
+
+            return new static($datetime);
        } catch (Exception $e) {
            throw new InvalidArgumentValue('$timestamp', $timestamp, __CLASS__, $e);
        }
    }

Reason for this is that DateTime objects created using timestamps passed to constructor are actually initialized with offsets +0000, instead of offsets coming from date_default_timezone_get (which in most cases equals server timezone setting). This means that DateTime objects created this way will have potentially different offsets, therefore generating different values for, for example, DateTime::format method.

This might be the reason why those datetimes differ in the first place. Adding default offset would cause that an actually invalid timestamp would be stored (!).

See: https://www.php.net/manual/en/datetime.construct.php#refsect1-datetime.construct-parameters

Note:

The $timezone parameter and the current timezone are ignored when the $datetime parameter either is a UNIX timestamp (e.g. @946684800) or specifies a timezone (e.g. 2010-01-28T15:00:00+02:00).

EDIT: I'm rather sure that this is the root of the issue you're fixing. As I said, you're storing offset info into timestamp (timestamps should not change depending on timezone). When creating DateTime from timestamp we should set the default timezone instead (which happens when calling DateTime's constructor without argument).

Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

Please address the issue raised in the comment.

@mateuszdebinski
Copy link
Contributor Author

Please address the issue raised in the comment.

The proposed change actually works as you describe, but then there is a problem on the front side, so I need to verify the JS files that are responsible for it

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Looks like we're missing test coverage

Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

LGTM. @alongosz do you think we need some more tests for this?

@Steveb-p
Copy link
Contributor

One thing to note: do we need to migrate anyone? Can any customer have timestamps stored with an offset?

@mateuszdebinski
Copy link
Contributor Author

One thing to note: do we need to migrate anyone? Can any customer have timestamps stored with an offset?

I think no, there shouldn't be any incorrect data saved anywhere, because when creating this item we changed the timestamp (add offset timezone) via JS now we do it in PHP

@sonarcloud
Copy link

sonarcloud bot commented May 10, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Steveb-p
Copy link
Contributor

Similar issues exist in eZ\Publish\Core\FieldType\DateAndTime\Value::fromTimestamp and eZ\Publish\Core\FieldType\Time\Value::fromTimestamp.

@bogusez bogusez self-assigned this May 11, 2021
@lserwatka lserwatka merged commit da07b78 into 1.2 May 12, 2021
@mateuszdebinski mateuszdebinski deleted the IBX-300-current-date-with-timezone-problem branch May 13, 2021 08:37
@mateuszdebinski
Copy link
Contributor Author

merge up - done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working QA approved
6 participants