Bug 12669

Summary: "Template process failed: undef error - Invalid local time for date in time zone"
Product: Koha Reporter: Rolando Isidoro <rolando.isidoro>
Component: SerialsAssignee: Rolando Isidoro <rolando.isidoro>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: chris, colin.campbell, dcook, jonathan.druart, m.de.rooy, mtj, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16376
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 14494    
Attachments: Error backtrace
Use floating timezone when handling dates without hours, minutes and seconds.
Bug 12669: Use floating timezone when handling dates without hours, minutes and seconds.
Patch signed off
Bug 12669: Centralize the timezone handle into Koha::DateUtils
Bug 12669: Use floating timezone when handling dates without hours, minutes and seconds.
Bug 12669: Centralize the timezone handle into Koha::DateUtils
Bug 12669: Use floating timezone when handling dates without hours, minutes and seconds.
Bug 12669: Centralize the timezone handle into Koha::DateUtils
Bug 12669: Use floating timezone when handling dates without hours, minutes and seconds.
Bug 12669: Centralize the timezone handle into Koha::DateUtils
[PASSED QA] Bug 12669: Use floating timezone when handling dates without hours, minutes and seconds.
[PASSED QA] Bug 12669: Centralize the timezone handle into Koha::DateUtils

Description Rolando Isidoro 2014-07-29 10:27:10 UTC
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.
Comment 1 Colin Campbell 2014-07-29 15:56:49 UTC
Possibly if passed a date only string the timezone should default to 'floating'
Comment 2 Rolando Isidoro 2014-07-31 15:10:20 UTC
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?
Comment 3 Rolando Isidoro 2014-07-31 15:37:25 UTC
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.
Comment 4 Chris Cormack 2014-08-01 08:54:16 UTC
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
Comment 5 Rolando Isidoro 2014-08-01 09:09:38 UTC Comment hidden (obsolete)
Comment 6 Rolando Isidoro 2014-08-01 12:49:05 UTC
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.
Comment 7 Chris Cormack 2014-08-01 22:43:19 UTC Comment hidden (obsolete)
Comment 8 Colin Campbell 2014-08-19 13:22:48 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2014-08-22 14:20:00 UTC
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);
  }
?
Comment 10 Rolando Isidoro 2014-09-02 15:28:09 UTC
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
Comment 11 Jonathan Druart 2014-09-16 12:02:56 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2014-09-16 12:04:54 UTC
(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?
Comment 13 Rolando Isidoro 2014-09-26 13:19:12 UTC
Hey Johnathan, your patch complexity and range greatly exceeds my knowledge of Koha's source code. Colin, what's your take on this?

Rolando
Comment 14 Colin Campbell 2014-10-01 12:25:44 UTC
(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
Comment 15 Jonathan Druart 2014-11-25 15:44:57 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2014-11-25 15:45:01 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2014-11-25 15:47:21 UTC
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.
Comment 18 Tomás Cohen Arazi 2014-11-25 22:11:15 UTC
(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.
Comment 19 Marcel de Rooy 2015-01-19 13:28:48 UTC
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.
Comment 20 Jonathan Druart 2015-01-19 14:40:03 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2015-01-19 14:40:12 UTC Comment hidden (obsolete)
Comment 22 Chris Cormack 2015-02-19 08:49:57 UTC Comment hidden (obsolete)
Comment 23 Chris Cormack 2015-02-19 08:50:17 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2015-02-20 11:57:25 UTC
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>
Comment 25 Kyle M Hall 2015-02-20 11:57:33 UTC
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>
Comment 26 Tomás Cohen Arazi 2015-02-20 14:13:35 UTC
Patches pushed to master.

Thanks Rolando and Jonathan!
Comment 27 Chris Cormack 2015-02-22 23:07:02 UTC
Pushed to 3.18.x will be in 3.18.4
Comment 28 Mason James 2015-08-28 05:11:52 UTC
Pushed to 3.16.x, will be in 3.16.14
Comment 29 Jonathan Druart 2016-03-02 07:31:44 UTC
*** Bug 12795 has been marked as a duplicate of this bug. ***
Comment 30 David Cook 2016-05-04 02:55:15 UTC
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.
Comment 31 David Cook 2016-05-04 02:58:33 UTC
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.
Comment 32 Marcel de Rooy 2016-05-04 05:52:53 UTC
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.
Comment 33 Colin Campbell 2016-05-04 13:45:44 UTC
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
Comment 34 David Cook 2016-05-05 02:39:24 UTC
(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.
Comment 35 David Cook 2016-05-05 02:44:11 UTC
(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.