Need fix review: DST with events (Moodle 1.9 - 2.0)

Need fix review: DST with events (Moodle 1.9 - 2.0)

by Jérôme Mouneyrac -
Number of replies: 4

Hi,

there are some timestamp issues with event DST when the Moodle timezone is set to 'server default' (MDL-17672):

1- if a first event is set at a DST then all recurrent events are saved in the database with a GMT - 1 hour (timestart timestamp). This behaviour doesn't match the behaviour of 'UTC' timezone setting where all recurrent events are saved with GMT regardless the DST.

2- when a recurrent event is in a DST, Moodle displays this event with the DST offset. It should be displayed without the offset because the timestart was saved at GMT.

I created a 1.9 patch for it: https://github.com/mouneyrac/moodle/commit/e1bcacf37dee127157e1fe2e11e95b5c247eecd1

It is a sensible issue and it's the first time I deal with a DST issue, so I would like some patch review (MDL-17672).

Thanks

Average of ratings: -
In reply to Jérôme Mouneyrac

Re: Need fix review: DST with events (Moodle 1.9 - 2.0)

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers

I just added a detailed comment to the bug, but I want to add a comment here too, because more people will probably see this, and the suggested approach here is dangerous and should not be copied.

The basic advice for developers is never, ever try to manipulate dates yourself. Calendars are insanely complicated, and you will get it wrong. There are not 24*60*60 seconds in a day; there are not 7*24*60*60 seconds in a week; and there are not even 60 seconds in every minute.

To get it right, you have to find a reliable date library that was written by someone who took time to understands all the insane rules, and implement them correctly.

On top of that, you still need to get give the right inputs and interpret the outputs correctly. That basically means knowing which time-zone each timestamp is, and treating it correctly. Fortunately, in Moodle, most of the time you can just use the userdate() function and not worry.

As, an example, to reliably get a date exactly one week ahead of the timestamp $t1, I would use $t2 = strtotime("+1 week", $ti); or should that be gmstrtotime? I would need to carefully read the docs and think to make sure I got that right. (Actually, I would look at some code that sam marshall wrote that I know is right, and copy that, but I can't access that here.)

Average of ratings: -
In reply to Tim Hunt

Re: Need fix review: DST with events (Moodle 1.9 - 2.0)

by Jérôme Mouneyrac -

Hi Tim,

I found out that if I do these tests, UTC and server default doesn't seem to be the same time:

My PHP date.timezone = 'Europe/London'.

My OS location is set to London too.

$time = 1302782400; //DST time '13 April'
var_dump(usergetdate($time));

with 'server default':

Array(    [seconds] => 0    [minutes] => 0    [hours] => 13    [mday] => 14    [wday] => 4    [mon] => 4    [year] => 2011    [yday] => 103    [weekday] => Thursday    [month] => April    [0] => 1302782400)

with 'UTC':

Array(    [seconds] => 00    [minutes] => 00    [hours] => 12    [mday] => 14    [wday] => 4    [mon] => 04    [year] => 2011    [yday] => 104    [weekday] => Thursday    [month] => April)

Calendar use this function to display the event date (which is a simple getdate() for 'server default' settings) causing the related issue.

Moreover event creation also use mktime for 'server default'. I think it's two bugs. Timestamps should be the same for a same event if server default and UTC-X have the same timezone. What do you think?

Average of ratings: -
In reply to Jérôme Mouneyrac

Re: Need fix review: DST with events (Moodle 1.9 - 2.0)

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers

Well, I suppose the key question is:

1. When we have a unix timestamp stored in the database, what does that mean?

And once you know that, you can work out the right answer to the questions:

2. Suppose the user wants to input the time 12:34 on 5th June 1978 in London, what timestamp should we store in the database.

3. Suppose we have a timestamp stored in the database, and we want to display in to a user in Perth Australia, what should we display.

In Moodle, the answer to 3. is "Just feed it to userdate()".

And for 2. if you just want the current time, you use the PHP function time().

Hopefully those two things are always right and consistent with each other.

Oh, and I just found the make_timestamp function in moodellib.php. Clearly we should use that for making dates that are not 'now'.

And, we should write enough unit tests to confirm to ourselves that make_timestamp and userdate always do the right thing in every possible circumstance.

Average of ratings: -
In reply to Tim Hunt

Re: Need fix review: DST with events (Moodle 1.9 - 2.0)

by Jérôme Mouneyrac -

Hi Tim, I just wanted to let you know that we are going to remove 'Server Default' option => MDL-26391 smile So events will always behave like they currently do with UTC. Feel free to comment smile

I confirm that timestamp is a time at one moment. (1.)

Average of ratings: -