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

EZP-29279: Values of Date Field Type should be handled in UTC only #1401

Merged
merged 3 commits into from
Mar 11, 2019

Conversation

mateuszbieniek
Copy link
Contributor

@mateuszbieniek mateuszbieniek commented Nov 29, 2018

JIRA issue: EZP-29279
Replaces #1384

Currently, in Legacy, the timestamp stored in the database is always midnight in the server's timezone, while in Platform it is always in UTC.
This PR modifies eZDateType class so timestamps are converted to UTC before being stored and are converted back to Local Timezone timestamp on retrieval.

Copy link
Member

@glye glye left a comment

Choose a reason for hiding this comment

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

Looks good! I presume Time and DateTime are already ok.

@mateuszbieniek
Copy link
Contributor Author

Thanks @glye! DataTime has different JIRA issue and I will be creating similar PR for it after this one is done ¯_(ツ)_/¯

Copy link
Member

@kmadejski kmadejski left a comment

Choose a reason for hiding this comment

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

The following is rather a suggestion than change request. Besides that 👍

$localDate = new \DateTime( $utcDate->format( 'Y-m-d H:i:s' ), $localTimezone );
$localTimestamp = $localDate->getTimestamp();

return $localTimestamp;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you should do it like:

$localDate = new \DateTime( $utcDate->format( 'Y-m-d H:i:s' ), $localTimezone );

return $localDate->getTimestamp();

or even:

return new \DateTime( $utcDate->format( 'Y-m-d H:i:s' ), $localTimezone )->getTimestamp();

but I prefer the first way, it's more readable.

The same applies for https://github.com/ezsystems/ezpublish-legacy/pull/1401/files/6b98fbe6e7b1c9e93bbf6b42a1d00205f42705bd#diff-9ae7112c632bad6c06997d57aefd6f82R24.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right! I prefer the first approach as well. Fixed.

@mateuszbieniek
Copy link
Contributor Author

Script for updating DB:
ezsystems/ezpublish-kernel#2499

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants