Description
David Cook
2016-04-28 06:31:45 UTC
You can easily verify this with the following Perl code: use DateTime; my $dt = DateTime->new( month => 9, year => 2015, day => 6, hour => 12, minute => 00, second => 00, time_zone => 'America/Santiago', ); print "DateTime:\n"; print $dt."\n"; print "Timezone:\n"; print $dt->time_zone()->name."\n"; print "Let's truncate...\n"; $dt->truncate( to => 'day'); TZ=America/Santiago perl issues.pl DateTime: 2015-09-06T12:00:00 Timezone: America/Santiago Let's truncate... Invalid local time for date in time zone: America/Santiago This problem was brought to me by Larry Letelier who found that he couldn't use overdue_notices.pl because of this error. You can replicate it by creating a Koha::Calendar, and then running the following: use DateTime; use Koha::DateUtils; use Koha::Calendar; my $dt = DateTime->new( month => 9, year => 2015, day => 6, hour => 12, minute => 00, second => 00, time_zone => 'America/Santiago', ); my $now = DateTime->now(); my $calendar = Koha::Calendar->new(branchcode => "MAIN"); $calendar->days_between($dt, $now); Run that using the America/Santiago TZ environmental variable: TZ=America/Santiago perl issues.pl -- You should get the "Invalid local time for date in time zone: America/Santiago" message and the script should die. Ahh, I bet it's truncating it to compare it to "single_holidays" and "exception_holidays"... right... that makes sense. In that case, before truncating it, I think you have to turn it into a "floating" DateTime. Except that we're using the Timezone in single_holidays() and exception_holidays()... that's great... I think we might need to re-think Koha::Calendar in light of https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12669 *** Bug 13079 has been marked as a duplicate of this bug. *** (In reply to David Cook from comment #3) > Ahh, I bet it's truncating it to compare it to "single_holidays" and > "exception_holidays"... right... that makes sense. > > In that case, before truncating it, I think you have to turn it into a > "floating" DateTime. > > Except that we're using the Timezone in single_holidays() and > exception_holidays()... that's great... > > I think we might need to re-think Koha::Calendar in light of > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12669 Hey, where's the patch? (In reply to Tomás Cohen Arazi from comment #5) > Hey, where's the patch? I've been too busy working on other projects :/. While I might have time to write a patch, I don't have anywhere near the time to test it or write a comprehensive test plan for others to use... :/. (In reply to David Cook from comment #6) > (In reply to Tomás Cohen Arazi from comment #5) > > Hey, where's the patch? > > I've been too busy working on other projects :/. > > While I might have time to write a patch, I don't have anywhere near the > time to test it or write a comprehensive test plan for others to use... :/. If you write a patch, I write regression tests for it :-) (In reply to Tomás Cohen Arazi from comment #7) > If you write a patch, I write regression tests for it :-) Thanks, Tomas :). Used 'grep -l -R "Koha::Calendar" * --exclude-dir="blib" | xargs grep "exception_holidays"' to try to find everything using Koha::Calendar and exception_holidays... looks like it's only Koha::Calendar. Looks like it's only Koha::Calendar::exeption_holidays() and Koha::Calendar::is_holiday() as well. I'm sure this issue pervades Koha, but perhaps we can resolve the problem in this particular piece of it... Created attachment 51702 [details] [review] Bug 16376 - Koha::Calendar->is_holiday date truncation creates fatal errors for TZ America/Santiago Using a DateTime object with a timezone of America/Santiago was causing fatal errors for Koha::Calendar->is_holiday and Koha::Calendar->exception_holidays, when the objects were truncated to an invalid local time. Using a floating zone allows us to use the same day, month, year for comparison purposes without running into the possibility of creating an invalid local time and thus a fatal software error. (In reply to Tomás Cohen Arazi from comment #7) > If you write a patch, I write regression tests for it :-) There's a patch. Where are the regression tests, Tomás? ;) (In reply to David Cook from comment #9) > Using a floating zone allows us to use the same day, month, year > for comparison purposes without running into the possibility of > creating an invalid local time and thus a fatal software error. Ok you will remove the software error, but I am not sure it's the way to go. Currently the dates are stored in local time (so with the tz), if we want to switch to floating time zones (what we should do at some point!) I think we should recalculate all dates present in DB. An easy workaround would be to use Koha::DateUtils, which deals (at least try to) with invalid dates due du DST. Moreover `grep time_zone Koha/Calendar.pm` will return another occurrence where the fix should be applied too. (In reply to Jonathan Druart from comment #11) > Ok you will remove the software error, but I am not sure it's the way to go. > I would certainly prefer a more thorough overhaul of Koha's DateTime handling, but I think this is good enough for now. Chile can't use the overdue functionality in Koha at all at this stage without this fix. > Currently the dates are stored in local time (so with the tz), if we want to > switch to floating time zones (what we should do at some point!) I think we > should recalculate all dates present in DB. > An easy workaround would be to use Koha::DateUtils, which deals (at least > try to) with invalid dates due du DST. Well, we calculate and interpret the dates using local time, but they're stored without timezone in the database. So I'm not sure what you mean by "recalculate all dates present in DB". The only difference between "local" and "floating" is the timezone. They're the same numbers, so there would be no recalculation to do. Or do you mean we should recalculate into UTC? Even if it were stored in UTC, it would need to be converted to local, and then to floating for the actual date handling, as we'd need the local numbers before doing any datetime math/truncation for the holiday checks. I have my reservations about Koha::DateUtils... I don't think enough thought has been put into its implementation. Besides, it wouldn't work in this case. Jonathan, the main problem at the moment is that Koha::DateUtils produces a valid local time, so it uses the local timezone. During the $calendar->days_between(), a day is added to this local datetime to produce another valid local time. However, the act of truncating that valid local time creates an invalid local time. So Koha::DateUtils isn't going to know there's a problem until it's too late. You could implement your own wrapper around DateTime, but I think that's getting excessive. We just need to be thoughtful and careful in our handling of dates and time. (In reply to Jonathan Druart from comment #12) > Moreover `grep time_zone Koha/Calendar.pm` will return another occurrence > where the fix should be applied too. Interesting. I think I avoided that the first time because $calendar->is_holiday() uses the following: my $ymd = $localdt->ymd('') ; if ($self->single_holidays( $ymd ) == 1 ) { return 1; } However, you are correct. sub single_holidays is weird... but definitely prone to this problem too. Created attachment 51906 [details] [review] Bug 16376 - Koha::Calendar->is_holiday date truncation creates fatal errors for TZ America/Santiago Using a DateTime object with a timezone of America/Santiago was causing fatal errors for Koha::Calendar->is_holiday and Koha::Calendar->exception_holidays, when the objects were truncated to an invalid local time. Using a floating zone allows us to use the same day, month, year for comparison purposes without running into the possibility of creating an invalid local time and thus a fatal software error. Created attachment 51968 [details] [review] Bug 16376 - Koha::Calendar->is_holiday date truncation creates fatal errors for TZ America/Santiago Using a DateTime object with a timezone of America/Santiago was causing fatal errors for Koha::Calendar->is_holiday and Koha::Calendar->exception_holidays, when the objects were truncated to an invalid local time. Using a floating zone allows us to use the same day, month, year for comparison purposes without running into the possibility of creating an invalid local time and thus a fatal software error. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> (In reply to David Cook from comment #13) > (In reply to Jonathan Druart from comment #11) > > Ok you will remove the software error, but I am not sure it's the way to go. > > > > I would certainly prefer a more thorough overhaul of Koha's DateTime > handling, but I think this is good enough for now. Chile can't use the > overdue functionality in Koha at all at this stage without this fix. > > > Currently the dates are stored in local time (so with the tz), if we want to > > switch to floating time zones (what we should do at some point!) I think we > > should recalculate all dates present in DB. > > An easy workaround would be to use Koha::DateUtils, which deals (at least > > try to) with invalid dates due du DST. > > Well, we calculate and interpret the dates using local time, but they're > stored without timezone in the database. > > So I'm not sure what you mean by "recalculate all dates present in DB". The > only difference between "local" and "floating" is the timezone. They're the > same numbers, so there would be no recalculation to do. Or do you mean we > should recalculate into UTC? If we plan to support several timezone, I think we will need to recalculate the existing dates into UTC, yes. > Even if it were stored in UTC, it would need to be converted to local, and > then to floating for the actual date handling, as we'd need the local > numbers before doing any datetime math/truncation for the holiday checks. Yes, floating times for calculation and tz to display/store the values. But let's have this discussion somewhere else :) > I have my reservations about Koha::DateUtils... I don't think enough thought > has been put into its implementation. Besides, it wouldn't work in this case. Well, look at the code before and after, we you will understand how it has been useful :) Looking at your patch again, it looks good. Please provide tests to highlight the issue you are trying to fix and to make sure this patch won't introduce regressions. This patch solve the repodted issue with overdue_process.pl using TZ=America/Santiago In fact, overdue_process.pl calculated until 1985 without errors. (Do not ask me why they have issues with that date :P ) Many Thanks David, Chris, John, and all involved !! (In reply to OpenGeek from comment #18) > This patch solve the repodted issue with overdue_process.pl using > TZ=America/Santiago > > In fact, overdue_process.pl calculated until 1985 without errors. > (Do not ask me why they have issues with that date :P ) > > > Many Thanks David, Chris, John, and all involved !! Hey, this one is failed-qa, I promised a regression test which I haven't provided and Jonathan failed it! (In reply to Tomás Cohen Arazi from comment #19) > (In reply to OpenGeek from comment #18) > > This patch solve the repodted issue with overdue_process.pl using > > TZ=America/Santiago > > > > In fact, overdue_process.pl calculated until 1985 without errors. > > (Do not ask me why they have issues with that date :P ) > > > > > > Many Thanks David, Chris, John, and all involved !! > > Hey, this one is failed-qa, I promised a regression test which I haven't > provided and Jonathan failed it! Ping! Any chance we can get those unit tests ; ) I spent a couple hours on this. And now have to leave for Manuel's birthday :-D So, for the record: - I narrowed the problem down to the sub days_between. Which is called in the failing scripts. The only way to reproduce this (in order to create the unit tests) was: $ENV{TZ} = $timezone; use POSIX qw(tzset); tzset; my $tz = C4::Context->tz; my $chile_dt = DateTime->new( day => 6, month => 9, year => 2015, time_zone => $tz ); my $now_dt = DateTime->now; my $branch = $builder->build({ source => 'Branch' })->{branchcode}; my $calendar = Koha::Calendar->new( branchcode => $branch ); my $diff = $calendar->days_between($now_dt,$chile_dt); Using the other suspected functions didn't trigger the problem (exception_holidays, etc)[1]. So the problem was correctly highlighted by David, but I'm not sure what's the best place to fix it. I vote for days_between. I'll try to resume tomorrow, and will be glad to hear people's comments, specially David, that created the patch. [1] Using truncate didn't raise the problem to me, either. Created attachment 58001 [details] [review] Bug 16376: (regression tests) This patch introduces a regression test for exception_holidays. This routine returns a list of datetimes to be used in date comparison and some datetimes don't exist in some timezones, so floating timezones should be used instead. To test: - Apply the patch on master - Run: $ prove t/db_dependent/Holidays.t => FAIL: The new test fails Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 58002 [details] [review] Bug 16376 - Koha::Calendar->is_holiday date truncation creates fatal errors for TZ America/Santiago Using a DateTime object with a timezone of America/Santiago was causing fatal errors for Koha::Calendar->is_holiday and Koha::Calendar->exception_holidays, when the objects were truncated to an invalid local time. Using a floating zone allows us to use the same day, month, year for comparison purposes without running into the possibility of creating an invalid local time and thus a fatal software error. Edit: While the changes to is_holiday and single_holiday make sense (Jonathan agrees too) I didn't manage to have them fail, because truncate is not failing in my trials, but days_between. So to me, it narrows down to have exception_holiday return floating tz datetime objects so it doesn't break days_between. Anyway, it is ok to push this patch, and the regression test I provide covers this scenario I'm describing. To test: - Apply the regression tests patch - Run: $ prove t/db_dependent/Holidays.t => FAIL: Unexpected error due to bad timezone/date combination - Apply this patch - Run: $ prove t/db_dependent/Holidays.t => SUCCESS: Tests pass Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to master for 17.05, thanks David, Tomas! These patches have been pushed to 16.11.x, will be in 16.11.01. Pushed to 3.22.x for 3.22.14 Hurray! Take that invalid local time for date in time zone... Pushed to 16.05.x, for 16.05.06 release |