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-29545 Values of DateTime Field Type should be handled in UTC only #1405

Open
wants to merge 1 commit into
base: 2017.12
Choose a base branch
from

Conversation

mateuszbieniek
Copy link
Contributor

@mateuszbieniek mateuszbieniek commented Nov 30, 2018

JIRA issue: EZP-29545

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

This PR depends on #1401 and should be merged only after it.

@gggeek
Copy link
Contributor

gggeek commented Nov 30, 2018

Do we miss a db update script ?

@mateuszbieniek
Copy link
Contributor Author

@gggeek currently yes, but it is almost completed.

@gggeek
Copy link
Contributor

gggeek commented Nov 30, 2018

I also do not see any phpdoc paragraph mentioning what the timestamp is supposed to be

@mateuszbieniek
Copy link
Contributor Author

@gggeek can you please point me to the part of the code you are talking about?

@gggeek
Copy link
Contributor

gggeek commented Nov 30, 2018

No specific part. I just think that the fact that we are now storing the timestamp of midnight-in-utc-timezone should be documented clearly. Ideally both in the online docs and in the source code.

To give some context: of all the ez5 api, one of the parts that I found most lacking in documentation was what are the accepted input/output values for Fields.

@mateuszbieniek
Copy link
Contributor Author

@gggeek the "midnight" part was a bad case of copy&paste from Date FieldType PR's description - sorry for that.

In legacy dates/datetimes were always stored as timestamps - this PR changes that those timestamps are stored with UTC to be more reliable and compatible with Platform (which does that from the start) - for example when using new stack with Legacy Bridge.

I don't think that such low-level things (as what database value lays behind ezdatetime) have to be better documented and actually are out of this PR scope.

@gggeek
Copy link
Contributor

gggeek commented Nov 30, 2018

Let me disagree with you.

  1. in general, documenting what values are stored in the database is valuable to developers who use the platform. Some of them might want to create import/export/create scripts that bypass the API because they deal with massive amounts of data, ore because transactions, or whatever. Some of them might want to create a separate API layer. Some of them might want to create data-consistency checkers (ever heard of ezdbintegrity?). Some of them etc.. etc...

  2. in this specific case, the fact that problems with timezone settings surfaced well 10 years after the cms was first created is a clear sign that for this specific field, documentation is even more important than for other fields:

  • for a php web app such as ezplatform, we have a db tz, server tz, php tz and possibly cms tz and end-user tz to deal with. making sure we are all on teh same page is important
  • dealing with timezones is a huge pain in general
  • timestamps are ALWAYS in UTC, so saying that they are "now in utc" is actually misleading. What has changed is that we now store 'midnight in the UTC tz' instead of 'midnight in the server tz'

@mateuszbieniek
Copy link
Contributor Author

@gggeek I get your point, but still - it is out of the scope of this PR. Nevertheless, I'm taking it into the account and do my best to improve the documentation.

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.

I agree somewhat about doc. The "root PR" #1401 should have a BC entry and method docblocks could also be good. Could avoid trouble for people doing hard core development/migration.

@mateuszbieniek
Copy link
Contributor Author

mateuszbieniek commented Nov 30, 2018

@glye about docs: yes I agree. I got confused because those are from the "root PR" and I couldn't pinpoint where I could put some doc blocks here 😉

@mateuszbieniek
Copy link
Contributor Author

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

@andrerom
Copy link
Contributor

MERGE NOTE: Needs to be merged after #1401

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.

5 participants