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

DateTime field value may become incorrect when an object persisted to MySQL is later retrieved #962

Open
rimas-kudelis opened this issue Jul 2, 2015 · 14 comments
Labels

Comments

@rimas-kudelis
Copy link

When MySQL connection timezone differs from PHP's "current timezone", a DateTime value persisted to MySQL storage will differ from the DateTime later being retrieved. For example, my my.cnf contains the following line:
default-time-zone='+0:00'
however, in the PHP config, I have the following line:
date.timezone = "Europe/Vilnius"
With this config, when I store a DateTime representing the following date to MySQL:
2015-01-01 01:00:00 UTC
and later retrieve it, I get back a DateTime representing the following date instead:
2015-01-01 01:00:00 Europe/Vilnius
which differs from what I expected by three whole hours (Vilnius is +0300)!

This effect is even more interesting if I attempt to store the DateTime in a completely different timezone. For example, I just stored the following date to the Database:
2015-10-01T21:15:00-0900
But what I see in the databaseis the following:
2015-10-01 21:15:00 // connection timezone is +0000 and the field is TIMESTAMP, thus the value differs by nine hours from the original by now.
And when I retrieve my object back, here's what I get instead of the expected timezone:
2015-10-01T18:15:00Z // differs by 12 hours from the original (the timezone is Z again, because toArray() converts datetime to UTC).

I see a few ways how this problem could be fixed. Since MySQL does not store timezone info in any of temporal fields, that info should be considered lost right at the beginning. Instead, I would concentrate on storing actually correct timestamps. So, here are the options I see:

  1. convert all timestamps to the timezone used by MySQL connection before storing them, and convert them from that same timezone when retrieving them back. Caveat: connection timezone might be "SYSTEM", and that value is ambiguous.
  2. explicitly set connection timezone to the PHP internal timezone when connecting to the DB. Caveats: PHP timezone might be changed from within a script while it runs. Which means it would then be safer to set it for each query separately, but this might be slower (although, I have no idea how slower) and would potentially litter MySQL logs with countless SET time_zone queries.
  3. use FROM_UNIXTIME() when storing datetime and UNIX_TIMESTAMP() when retrieving. This is my favourite option, although it has caveats as well: 1. converting time to timestamp and vice-versa might slow the whole process down (again, I have no clue how much noticeable the effect would be), and 2. doing that for DATETIME (not TIMESTAMP) fields would make the stored datetimes ambigous, since they would then rely on MySQL connection timezone remaining unchanged between storing and retrieving. Although I'd like to think that if Propel-generated models doesn't use DATETIME fields by default, that is an acceptable trade-off to make in the name of information integrity.

Not sure which one of these is best preferred. But in any way, I'm sure that this is a bug and it should be fixed.

@rimas-kudelis
Copy link
Author

A test case has been attached to #963.

@marcj
Copy link
Member

marcj commented Aug 7, 2015

As this is a result of a wrong configuration of PHP and MySQL which leads almost always also to other issues like displaying the wrong date in a view I'd recommend to trigger a warning if Propel detects in MySQL a different timezone than PHP is using. What do you think about that?

@marcj
Copy link
Member

marcj commented Aug 7, 2015

Well my idea would also provide a lot of queries at the beginning. So I believe your option 2) is the best if that fixes all of the timezone issues.

@rimas-kudelis
Copy link
Author

Hi Marc,
in fact, I don't think option 2) alone solves all problems. It is completely possible that for some reason, the DateTime being stored would use a non-default timezone. In fact, I think that's how I discovered this bug: I attempted storing DateTimes in different timezones in the application I work on, when I started getting unexpected DateTime conflicts.
I think it should be either 1) and 2) combined (we could SET time_zone when initializing a connection, store that timezone somewhere in Propel as a protected property, and convert all DateTime values to that timezone before persisting them and from that timezone after retrieving them), or just 3). What do you think?

@mpscholten
Copy link
Member

Maybe just enforce that the database uses UTC+0? :) It's best practice anyways to use UTC+0 for sql databases. Then propel just needs to convert the \DateTime object to UTC+0 before persisting.

@rimas-kudelis
Copy link
Author

Yeah, that's what I'm thinking about as well. There's a small downside to this: when getting DateTimes back, they will always be returned in UTC+0 as well, which might be suboptimal for a system which uses just one timezone, which is not UTC+0.

@mpscholten
Copy link
Member

Can't we just set the timezone of the \DateTime instance to the php default timezone after fetching?

@rimas-kudelis
Copy link
Author

Yes we can! :)

@gharlan
Copy link
Contributor

gharlan commented Sep 2, 2016

Maybe just enforce that the database uses UTC+0?

👍

Can't we just set the timezone of the \DateTime instance to the php default timezone after fetching?

👍

@SourceCode
Copy link

SourceCode commented Sep 2, 2016

This seems like an unneeded fix. This is a matter of configuration not a matter of convention, bug, or issue. Warning people or even setting anything on their behalf does not seem like the right answer. The right answer to me is to configure your system correctly. You should not compensate for this in software - you should not enforce or do anything and instead operate on the underlying configurations - even if they are wrong in the eyes of the software author or its users.

Yes if your system timezones are not configured correctly you will have odd results. This is not just PHP, MySQL or propel but all systems in the world. Incorrect configurations lead to less than desirable results. Saying this - I do not require my software to monitor for this or alert me of it. This is my responsibility to have correct before the software even ran.

@rimas-kudelis
Copy link
Author

rimas-kudelis commented Sep 2, 2016

@SourceCode: I beg to differ. Regardles of how you configure your system, if you store a DateTime with a non-default timezone, you get back a DateTime representing a completely different moment (timestamp). While this is a deficiency in MySQL, I'd like to expect the ORM to prevent data damage as much as possible. In other words, even if Propel cannot later reconstruct stored data with full precision, that doesn't mean it shouldn't do its best by at least making sure that the timestamp stored matches the timestamp retrieved.

@SourceCode
Copy link

The matter of prevention starts at configuration not compensation for the poor configuration within software. That is called treating the symptom and could lead to even worse issues since Propel is correcting your mistake but anything not using propel may not receive the same correction leading to inconsistency. If your ONLY source of access is Propel and ONLY your application is accessing the data MAYBE that would make sense but in large enterprise systems you correct your configurations.

@rimas-kudelis
Copy link
Author

rimas-kudelis commented Sep 3, 2016

In case I haven't made myself clear: this also happens when you store datetimes with non-default timezones. Non-default meaning arbitrary. You can fix and match your configs as much as you like, but this doesn't mean you can't use other time zones with your DateTime instances.

Saying that Propel should be buggier than necessary just because other accessors might expect these bugs or have them as well doesn't sound appealing at all.

Either way, if I remember correctly, I worked around this issue more than a year ago by converting all DateTime instances to UTC in the code before storing them. I just wish I didn't have to, because at least to me, correct timestamp in a DateTime matters more than [correct string representation without timezone], since we cannot infer the timezone from the DB anyway.

You know, scratch these last two paragraphs. The problem is pretty straightforward: Propel stores incorrect timestamps in TIMESTAMP fields, and retrieves incorrect timestamps from TIMESTAMP fields, that's it.

@jhaisley
Copy link
Contributor

jhaisley commented Sep 5, 2016

In case I haven't made myself clear: this also happens when you store datetimes with non-default timezones. Non-default meaning arbitrary. You can fix and match your configs as much as you like, but this doesn't mean you can't use other time zones with your DateTime instances.

Could this be a behavior, something like store date-time in UTC?

Incognito pushed a commit to spryker/Propel2 that referenced this issue Jul 10, 2020
…incorrect when an object persisted to MySQL is later retrieved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants