ezpublish-legacy icon indicating copy to clipboard operation
ezpublish-legacy copied to clipboard

EZP-29545 Values of DateTime Field Type should be handled in UTC only

Open mateuszbieniek opened this issue 6 years ago • 11 comments

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 https://github.com/ezsystems/ezpublish-legacy/pull/1401 and should be merged only after it.

mateuszbieniek avatar Nov 30 '18 10:11 mateuszbieniek

Do we miss a db update script ?

gggeek avatar Nov 30 '18 10:11 gggeek

@gggeek currently yes, but it is almost completed.

mateuszbieniek avatar Nov 30 '18 10:11 mateuszbieniek

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

gggeek avatar Nov 30 '18 10:11 gggeek

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

mateuszbieniek avatar Nov 30 '18 10:11 mateuszbieniek

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.

gggeek avatar Nov 30 '18 11:11 gggeek

@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.

mateuszbieniek avatar Nov 30 '18 11:11 mateuszbieniek

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'

gggeek avatar Nov 30 '18 12:11 gggeek

@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.

mateuszbieniek avatar Nov 30 '18 12:11 mateuszbieniek

@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 :wink:

mateuszbieniek avatar Nov 30 '18 14:11 mateuszbieniek

Script for updating DB: https://github.com/ezsystems/ezpublish-kernel/pull/2499

mateuszbieniek avatar Dec 11 '18 10:12 mateuszbieniek

MERGE NOTE: Needs to be merged after #1401

andrerom avatar Dec 21 '18 11:12 andrerom