Created attachment 30268 [details] Error backtrace When accessing "Serial collection" for a given subscription the "Invalid local time for date in time zone" error message will show if any of the dates (publisheddate or planneddate) in the subscription is in fact a day where a timezone offset occurred at 00:00:00. Ex:. Date 1979-04-01 and timezone Europe/Lisbon. The day existed, but the 00:00:00 hour didn't, since after 1979-03-31 23:59:59 came 1979-04-01 01:00:00 source: http://www.timeanddate.com/time/change/portugal/lisbon?year=1979) I've debugged the issue and found out that it occurs when applying the KohaDates filter (lib/Koha/Template/Plugin/KohaDates.pm, specifically in the dt_from_string call @ line 32) in the serials collection template (intranet/htdocs/intranet-tmpl/prog/en/modules/serials/serials-collection.tt). The expected result would be for the dates to show up correctly since we're dealing with days and not timestamps, so 1979-04-01 should be valid even though 1979-04-01 00:00:00 isn't for the Europe/Lisbon timezone.
Possibly if passed a date only string the timezone should default to 'floating'
Hey Colin, following on your comment I tried to fix the problem on my own and I think I narrowed it to 1 code entry: $dt->set_time_zone( C4::Context->tz ); @ https://github.com/digibib/koha-work/blob/master/Koha/DateUtils.pm#L126 Since my knowledge regarding Koha's codebase is limited I now have doubts on how to proceed, but I think it would be as simple as replacing that line for: my $tz = $dateonly ? DateTime::TimeZone->new(name => 'floating') : C4::Context->tz; $dt->set_time_zone( $tz ); What's your take on this solution?
It looks like there is another code entry that needs correction: my $dt = dt_from_string( $text, 'iso' ); @ https://github.com/digibib/koha-work/blob/master/Koha/Template/Plugin/KohaDates.pm#L32 becomes: my $tz = DateTime::TimeZone->new(name => 'floating') unless $config->{with_hours}; my $dt = dt_from_string( $text, 'iso', $tz ); With these 2 fixes in place the "Serial collection" page is rendered correctly.
Hi Rolando We don't actually use github, its just a mirror/backup Can you please attach a patch (using git format-patch) against git.koha-community.org/koha.git master branch. Then set the status to needs sign off. It will then get code reviewed, and will shift to signed off if it passes. Then its on to QA, then the release manager finally. This is the workflow we use http://wiki.koha-community.org/wiki/Bug-enhancement-patch_Workflow If you cant create the patch let me know, and i'll do it from your github commit
Created attachment 30446 [details] [review] Use floating timezone when handling dates without hours, minutes and seconds.
Test plan: 1. Create a new serial record or pick an existing one; 2. Add a "New subscription" and set the "First issue publication date" and "Frequency" values so that the "Planned date" for a given issue hits a day where a transition to Daylight Saving Time (DST) occurs ex:. 1979-04-01 in Europe/Lisbon (http://www.timeanddate.com/time/change/portugal/lisbon?year=1979) ... use this website page as a source http://www.timeanddate.com/time/dst/2014.html to find a suitable transition to DST in your timezone in the current year. 3. From the "Subscription detail" page in Koha for the given record click the "Serial collection" link on the left side context menu. Expected result: Prior to applying the patch the error described in this bug report should occur. After the patch is applied the "Serial collection" page should load correctly.
Created attachment 30474 [details] [review] Bug 12669: Use floating timezone when handling dates without hours, minutes and seconds. Test plan: 1. Create a new serial record or pick an existing one; 2. Add a "New subscription" and set the "First issue publication date" and "Frequency" values so that the "Planned date" for a given issue hits a day where a transition to Daylight Saving Time (DST) occurs ex:. 1979-04-01 in Europe/Lisbon (http://www.timeanddate.com/time/change/portugal/lisbon?year=1979) ... use this website page as a source http://www.timeanddate.com/time/dst/2014.html to find a suitable transition to DST in your timezone in the current year. 3. From the "Subscription detail" page in Koha for the given record click the "Serial collection" link on the left side context menu. Expected result: Prior to applying the patch the error described in this bug report should occur. After the patch is applied the "Serial collection" page should load correctly.
Created attachment 30985 [details] [review] Patch signed off Does whats required. floating should be default for day granularity. No doubt there are a few other corners that should specify floating but this seems to deal with the problems experienced by some users
I am not sure this is the right way to fix it. For instance, in Paris the DST is at 2: perl -MDateTime -E'say DateTime->new(year => 2014, month => 9, day => 7, hour => 0, time_zone => "America/Santiago");'; Invalid local time for date in time zone: America/Santiago perl -MDateTime -E'say DateTime->new(year => 2014, month => 3, day => 30, hour => 2, time_zone => "Europe/Paris");'; Invalid local time for date in time zone: Europe/Paris perl -MDateTime -E'say DateTime->new(year => 2014, month => 3, day => 30, time_zone => "Europe/Paris");'; 2014-03-30T00:00:00 The DateTime doc mentions "This may change in future version of this module." http://search.cpan.org/~drolsky/DateTime-1.10/lib/DateTime.pm#Invalid_Local_Times What about something like: my $dt = eval { DateTime->new($params) }; if ( $@ ) { $params->{hours}++; $dt = Datetime->new($params); } ?
Hi Jonathan, at first I also thought of the solution you proposed, but it seemed like a small workaround that wouldn't address the real issue. In the cases I pointed in this bug the dates didn't include time information (hours, minutes and seconds), so why should koha validate them using timezones? As Colin puts it: "floating should be default for day granularity". The solution I presented fixes the issue and introduces this minor improvement. Best regards, Rolando Isidoro
Created attachment 31623 [details] [review] Bug 12669: Centralize the timezone handle into Koha::DateUtils This patch adds unit tests for the previous changes and centralize the timezone handle into the Koha::DateUtils module. Like that the behavior will affect all date manipulations using this module (should be all dates in Koha).
(In reply to Jonathan Druart from comment #11) > Created attachment 31623 [details] [review] [review] Rolando and Colin, could it be possible to get your point of view on this patch please?
Hey Johnathan, your patch complexity and range greatly exceeds my knowledge of Koha's source code. Colin, what's your take on this? Rolando
(In reply to Jonathan Druart from comment #12) > (In reply to Jonathan Druart from comment #11) > > Created attachment 31623 [details] [review] [review] [review] > > Rolando and Colin, could it be possible to get your point of view on this > patch please? (In reply to Rolando Isidoro from comment #13) > Hey Johnathan, your patch complexity and range greatly exceeds my knowledge > of Koha's source code. Colin, what's your take on this? > > Rolando This changes behaviour in an undocumented way if I pass a tz other than C4::Context's it seems in some strings (need more test cases) to be changing to the system tz but not correctly converting the date ... It also adds processing to a frequently used routine which is not efficient e.g. the evals are unnecessary if anything failes the date parse returns undef flagging the error. I'll try testing some more inputs to this
Created attachment 33914 [details] [review] Bug 12669: Use floating timezone when handling dates without hours, minutes and seconds. Test plan: 1. Create a new serial record or pick an existing one; 2. Add a "New subscription" and set the "First issue publication date" and "Frequency" values so that the "Planned date" for a given issue hits a day where a transition to Daylight Saving Time (DST) occurs ex:. 1979-04-01 in Europe/Lisbon (http://www.timeanddate.com/time/change/portugal/lisbon?year=1979) ... use this website page as a source http://www.timeanddate.com/time/dst/2014.html to find a suitable transition to DST in your timezone in the current year. 3. From the "Subscription detail" page in Koha for the given record click the "Serial collection" link on the left side context menu. Expected result: Prior to applying the patch the error described in this bug report should occur. After the patch is applied the "Serial collection" page should load correctly. Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Created attachment 33915 [details] [review] Bug 12669: Centralize the timezone handle into Koha::DateUtils This patch adds unit tests for the previous changes and centralize the timezone handle into the Koha::DateUtils module. Like that the behavior will affect all date manipulations using this module (should be all dates in Koha).
Patches rebased (conflict with bug 13242). I removed the useless processing (I don't remember why I parse the string...) and an eval. Please obsolete my patch if you think the first patch is better alone.
(In reply to Jonathan Druart from comment #17) > Patches rebased (conflict with bug 13242). > > I removed the useless processing (I don't remember why I parse the > string...) and an eval. > > Please obsolete my patch if you think the first patch is better alone. I like your approach. You eval the datetime, and if it fails you just assume is a date without time, thus a floating tz is used instead. It might fail in some situations (date is invalid no matter the tz). Will test it and find a border situation to write a regression test for.
Applying: Bug 12669: Use floating timezone when handling dates without hours, minutes and seconds. Applying: Bug 12669: Centralize the timezone handle into Koha::DateUtils fatal: sha1 information is lacking or useless (Koha/DateUtils.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge.
Created attachment 35389 [details] [review] Bug 12669: Use floating timezone when handling dates without hours, minutes and seconds. Test plan: 1. Create a new serial record or pick an existing one; 2. Add a "New subscription" and set the "First issue publication date" and "Frequency" values so that the "Planned date" for a given issue hits a day where a transition to Daylight Saving Time (DST) occurs ex:. 1979-04-01 in Europe/Lisbon (http://www.timeanddate.com/time/change/portugal/lisbon?year=1979) ... use this website page as a source http://www.timeanddate.com/time/dst/2014.html to find a suitable transition to DST in your timezone in the current year. 3. From the "Subscription detail" page in Koha for the given record click the "Serial collection" link on the left side context menu. Expected result: Prior to applying the patch the error described in this bug report should occur. After the patch is applied the "Serial collection" page should load correctly. Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Created attachment 35390 [details] [review] Bug 12669: Centralize the timezone handle into Koha::DateUtils This patch adds unit tests for the previous changes and centralize the timezone handle into the Koha::DateUtils module. Like that the behavior will affect all date manipulations using this module (should be all dates in Koha).
Created attachment 36011 [details] [review] Bug 12669: Use floating timezone when handling dates without hours, minutes and seconds. Test plan: 1. Create a new serial record or pick an existing one; 2. Add a "New subscription" and set the "First issue publication date" and "Frequency" values so that the "Planned date" for a given issue hits a day where a transition to Daylight Saving Time (DST) occurs ex:. 1979-04-01 in Europe/Lisbon (http://www.timeanddate.com/time/change/portugal/lisbon?year=1979) ... use this website page as a source http://www.timeanddate.com/time/dst/2014.html to find a suitable transition to DST in your timezone in the current year. 3. From the "Subscription detail" page in Koha for the given record click the "Serial collection" link on the left side context menu. Expected result: Prior to applying the patch the error described in this bug report should occur. After the patch is applied the "Serial collection" page should load correctly. Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 36012 [details] [review] Bug 12669: Centralize the timezone handle into Koha::DateUtils This patch adds unit tests for the previous changes and centralize the timezone handle into the Koha::DateUtils module. Like that the behavior will affect all date manipulations using this module (should be all dates in Koha). Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 36068 [details] [review] [PASSED QA] Bug 12669: Use floating timezone when handling dates without hours, minutes and seconds. Test plan: 1. Create a new serial record or pick an existing one; 2. Add a "New subscription" and set the "First issue publication date" and "Frequency" values so that the "Planned date" for a given issue hits a day where a transition to Daylight Saving Time (DST) occurs ex:. 1979-04-01 in Europe/Lisbon (http://www.timeanddate.com/time/change/portugal/lisbon?year=1979) ... use this website page as a source http://www.timeanddate.com/time/dst/2014.html to find a suitable transition to DST in your timezone in the current year. 3. From the "Subscription detail" page in Koha for the given record click the "Serial collection" link on the left side context menu. Expected result: Prior to applying the patch the error described in this bug report should occur. After the patch is applied the "Serial collection" page should load correctly. Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 36069 [details] [review] [PASSED QA] Bug 12669: Centralize the timezone handle into Koha::DateUtils This patch adds unit tests for the previous changes and centralize the timezone handle into the Koha::DateUtils module. Like that the behavior will affect all date manipulations using this module (should be all dates in Koha). Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Patches pushed to master. Thanks Rolando and Jonathan!
Pushed to 3.18.x will be in 3.18.4
Pushed to 3.16.x, will be in 3.16.14
*** Bug 12795 has been marked as a duplicate of this bug. ***
I don't think this patch is actually a good idea... you're only sometimes - and very unexpectedly - making a DateTime object with a floating timezone. Consider the following from DateTime's perldoc (http://search.cpan.org/~drolsky/DateTime-1.26/lib/DateTime.pm#): "Floating DateTimes The default time zone for new DateTime objects, except where stated otherwise, is the "floating" time zone. This concept comes from the iCal standard. A floating datetime is one which is not anchored to any particular time zone. In addition, floating datetimes do not include leap seconds, since we cannot apply them without knowing the datetime's time zone. The results of date math and comparison between a floating datetime and one with a real time zone are not really valid, because one includes leap seconds and the other does not. Similarly, the results of datetime math between two floating datetimes and two datetimes with time zones are not really comparable. If you are planning to use any objects with a real time zone, it is strongly recommended that you do not mix these with floating datetimes." -- I think it would be smarter to actually review where we're using timezones and re-read http://search.cpan.org/~drolsky/DateTime-1.26/lib/DateTime.pm#How_DateTime_Math_Works. -- Another idea would be to use DateTime->ymd() instead of DateTime->truncate() (http://search.cpan.org/~drolsky/DateTime-1.26/lib/DateTime.pm#$dt->ymd(_$optional_separator_),_$dt->mdy(...),_$dt->dmy(...)), so that you're actually just comparing the date portion of a datestamp rather than possibly creating an invalid local time.
Except ymd() probably wouldn't work for sub single_holidays or sub exception_holidays because they use DateTime::Set I think... so that option is probably no good.
David: I recall falling back to floating timezone for making calculations with far future dates like an expiry in 9999 or so. If you would use a normal timezone, you will have major performance issues.
I wonder if we could use utc internally and just use the local timezone to convert user input and to present the date externally. We'd need to be fairly vigorous in cleaning up the various vaguely different ways we currently treat dates, cetainly when we're doing calculations we probably want to do them in a common base as otherwise there's too many opportunities to fall into a given timezone's black hole
(In reply to Marcel de Rooy from comment #32) > David: I recall falling back to floating timezone for making calculations > with far future dates like an expiry in 9999 or so. > If you would use a normal timezone, you will have major performance issues. Yeah, that's an unrelated issue I believe.
(In reply to Colin Campbell from comment #33) > I wonder if we could use utc internally and just use the local timezone to > convert user input and to present the date externally. We'd need to be > fairly vigorous in cleaning up the various vaguely different ways we > currently treat dates, cetainly when we're doing calculations we probably > want to do them in a common base as otherwise there's too many opportunities > to fall into a given timezone's black hole Agreed. I think we could use UTC internally and just use the local timezone to convert user input and convert output for users. We would need to be vigorous in cleaning up how we've currently treating dates though, yeah :/. Generally speaking, we're not recording super precise datetime data, so I don't see why we couldn't use floating timezones for calculations. I suppose it depends on the calculations being done, but that's really the only way of doing it with our current holiday checking code without causing invalid local times. We might be able to use UTC for some calculations, but... I don't think so in the holiday case because we'd be interested in local days and obviously UTC doesn't always correspond with the same day. You might have 2 UTC timestamps for the same day in UTC, but they'd actually correspond to different days in local time. If you truncated that for the sake of date checking, you'd be skipping one of the local days, which would be really bad.