core icon indicating copy to clipboard operation
core copied to clipboard

tstamp smaller 3600 are saved as 01:00

Open magicsepp opened this issue 9 years ago • 13 comments

make a new event with 00:30 for the time and 01.01.1970 as date. After saving the time the time value is changed to 01:00.

magicsepp avatar Oct 10 '14 13:10 magicsepp

Might be a collision with your selected timezone? We have GMT+1. Substracting this from 1800 brings us into negative which might get corrected to 0 then which in turn get's GMT+1 and therefore 3600. Just a guess though.

discordier avatar Oct 15 '14 22:10 discordier

How do I reproduce this in the online demo?

leofeyer avatar Oct 16 '14 18:10 leofeyer

Leo, make a new event with starttime 00:30 and the date 01.01.1970 the endtime could be 0:45. After saving the event you see the start time changed to 01:00 (same for the end time)

magicsepp avatar Oct 17 '14 10:10 magicsepp

It turns out to be exactly what @discordier said above. 00:30 is converted to -1800 internally, because the time zone is GMT+1. However, since the database field is unsigned, it cannot store negative values and thus stores 0.

Generally, I consider this an edge case. Or do you really have events starting on January 1st, 1970, at 00:30 am?

leofeyer avatar Oct 21 '14 18:10 leofeyer

the problem occur by using only time based values f.e opening times. In such a case there are only values from 00:00 up to 23:59 so only integers from 0 up to 86.400 are used.There is no need for negativ values. Hope this clarified the situation alittle bit ;)

magicsepp avatar Oct 22 '14 13:10 magicsepp

@leofeyer He is right on that one. When only storing the time, we will end up in negative values when having GMT+X (depending on time zone).

This however is not as easy to solve (aside from finally allowing negative timestamps) as GMT-11 is the one with the smallest value and therefore we need to have at least of 12 hours "buffer" to ensure we do not drop below 0.

We might try to solve this by forcing the date part of the timestamp to be 02 January 1970 but this still might drop down to 01. January 1970 when displaying the time for a different time zone setting.

In the end the main part is, if the time only should indeed take timezones and DST into account or not. If it should not, a simple handling by dividing by 3600 etc. should be sufficient but must be implemented at the various places.

To make long story short, this needs more discussion and therefore I vote for reopening the issue.

discordier avatar Oct 23 '14 01:10 discordier

Isn't the problem just the unsigned flag in the database? If I remove it, I can store -1800 without a problem.

leofeyer avatar Oct 23 '14 12:10 leofeyer

As I wrote:

This however is not as easy to solve (aside from finally allowing negative timestamps)

Problem arising is the much smaller maximum value, see [1]:

Type S/US Bytes Minimum Value Maximum Value Min date('Y-m-d H:i:s') Max date('Y-m-d H:i:s')
TINYINT S 1 -128 127 1969-12-31 23:57:52 1970-01-01 00:02:07
U 0 255 1970-01-01 00:00:00 1970-01-01 00:04:15
SMALLINT S 2 -32768 32767 1969-12-31 14:53:52 1970-01-01 09:06:07
U 0 65535 1970-01-01 00:00:00 1970-01-01 18:12:15
MEDIUMINT S 3 -8388608 8388607 1969-09-25 21:49:52 1970-04-08 02:10:07
U 0 16777215 1970-01-01 00:00:00 1970-07-14 04:20:15
INT S 4 -2147483648 2147483647 1901-12-13 21:45:52 2038-01-19 04:14:07
U 0 4294967295 1970-01-01 00:00:00 2106-02-07 07:28:15
BIGINT S 8 -9223372036854775808 9223372036854775807 -292277022657-01-27 09:29:52 -292277022657-01-27 09:29:50
U 0 18446744073709551615 1970-01-01 00:00:00 1969-12-31 22:09:20

As you can see, we will drop the valid maximum date from 2106-02-07 07:02:15 down to 2038-01-19 04:01:07 (which is the default maximum unix timestamp for 32bit anyway) but we WERE able to store up to 2106-02-07 07:02:15 which renders such a change a bc break.

Another option is to, as I included in the table above, use BIGINT which is 64bit and available on any 64bit system nowadays (who is still running a 32bit server?). Downside is, PHP still can not cope with too large values.choking on ver big or small values in 64bit space (see table above).

Therefore changing the column from unsigned to signed is IMO only an option when changing to BIGINT along the way. Another solution would be to switch to DATETIME types as they were made for this.

[1] http://dev.mysql.com/doc/refman/5.5/en/integer-types.html

discordier avatar Oct 24 '14 02:10 discordier

Thanks for the detailed explanation. However, that was not my point actually :)

the problem occur by using only time based values f.e opening times. In such a case there are only values from 00:00 up to 23:59 so only integers from 0 up to 86.400 are used.

In the core, we are not using time fields without a date field anywhere, therefore we don't need to change anything at all. Only if someone needs to use time fields only, they have to make sure not to use the unsigned flag. And since time only fields have a maximum range of -86.400 to 86.400, a MEDIUMINT column should suffice.

leofeyer avatar Oct 24 '14 07:10 leofeyer

@leofeyer but how do you suggest to fix the bug then? tl_calendar_event.startTime does hold date and time information, and according to @discordier a signed DB column would be a BC break.

aschempp avatar Oct 24 '14 07:10 aschempp

We don't need to fix anything. The bug only affects events on January 1st, 1970, between 0:00 and 12:00 (depending on your timezone) and that really is an edge case.

leofeyer avatar Oct 24 '14 07:10 leofeyer

I am fine with defining this as a known issue but we might want to reevaluate the whole timestamp thing for C4 again.

discordier avatar Oct 24 '14 12:10 discordier

but we might want to reevaluate the whole timestamp thing for C4 again

Since we won't be having time to reevaluate the issue for C4, I'm moving the ticket to the C5 milestone.

leofeyer avatar May 04 '15 08:05 leofeyer