Bug 13601 - Get rid of DateTime::Format::DateParse
Summary: Get rid of DateTime::Format::DateParse
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 14016 14194
  Show dependency treegraph
 
Reported: 2015-01-20 14:22 UTC by Jonathan Druart
Modified: 2016-06-21 21:39 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13601: Add tests to highlight the problems with DateTime::Format::DateParse (3.30 KB, patch)
2015-01-20 15:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13601: Make dt_from_string not using DateTime::Format::DateParse (5.57 KB, patch)
2015-01-20 15:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13601: get rid of DateTime::Format::DateParse (2.34 KB, patch)
2015-01-20 15:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13601: Add a fallback check for compability with existing code (2.18 KB, patch)
2015-01-20 15:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13601: Fix special case in basket.pl (3.21 KB, patch)
2015-01-21 11:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13601: Add tests to highlight the problems with DateTime::Format::DateParse (3.27 KB, patch)
2015-03-09 08:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13601: Make dt_from_string not using DateTime::Format::DateParse (6.03 KB, patch)
2015-03-09 08:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13601: get rid of DateTime::Format::DateParse (2.34 KB, patch)
2015-03-09 08:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13601: Add a fallback check for compability with existing code (2.01 KB, patch)
2015-03-09 08:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13601: Fix special case in basket.pl (3.21 KB, patch)
2015-03-09 08:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
[Signed-off] Bug 13601: Add tests to highlight the problems with DateTime::Format::DateParse (3.51 KB, patch)
2015-03-09 17:53 UTC, Marc Véron
Details | Diff | Splinter Review
[Signed-off] Bug 13601: Make dt_from_string not using DateTime::Format::DateParse (6.23 KB, patch)
2015-03-09 18:09 UTC, Marc Véron
Details | Diff | Splinter Review
[Signed-off] Bug 13601: get rid of DateTime::Format::DateParse (2.56 KB, patch)
2015-03-09 18:13 UTC, Marc Véron
Details | Diff | Splinter Review
[Signed-off] Bug 13601: Add a fallback check for compability with existing code (2.16 KB, patch)
2015-03-09 18:23 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 13601: Fix special case in basket.pl (1.56 KB, patch)
2015-03-10 08:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13601: Fix special case in basket.pl (3.40 KB, patch)
2015-03-10 08:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
[Signed-off] Bug 13601: Fix special case in basket.pl (3.56 KB, patch)
2015-03-10 09:14 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 13601: t/Letters.t needs to mock the dateformat pref (671 bytes, patch)
2015-03-16 13:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
[Signed-off] Bug 13601: t/Letters.t needs to mock the dateformat pref (841 bytes, patch)
2015-03-16 14:38 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 13601: The fallback regex should contain the time part (1.82 KB, patch)
2015-03-17 08:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
[Signed-off] Bug 13601: The fallback regex should contain the time part (2.05 KB, patch)
2015-03-20 08:59 UTC, Marc Véron
Details | Diff | Splinter Review
[PASSED QA] Bug 13601: Add tests to highlight the problems with DateTime::Format::DateParse (3.56 KB, patch)
2015-03-22 19:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13601: Make dt_from_string not using DateTime::Format::DateParse (6.28 KB, patch)
2015-03-22 19:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13601: get rid of DateTime::Format::DateParse (2.62 KB, patch)
2015-03-22 19:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13601: Add a fallback check for compability with existing code (2.21 KB, patch)
2015-03-22 19:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13601: Fix special case in basket.pl (3.61 KB, patch)
2015-03-22 19:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13601: t/Letters.t needs to mock the dateformat pref (892 bytes, patch)
2015-03-22 19:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13601: The fallback regex should contain the time part (2.10 KB, patch)
2015-03-22 19:57 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2015-01-20 14:22:04 UTC
For a couple of reasons, dt_from_string should not use DateTime::Format::DateParse:
1/ It does not manage date < 1900 (certainly caused by l.47 of this module:
     $p{ year }      = $year ? $year + 1900 : DateTime->now->year;
2/ It considers 31/01/2015 as a valid us date, which is not.
Comment 1 Jonathan Druart 2015-01-20 15:12:20 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-01-20 15:12:24 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-01-20 15:12:32 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2015-01-20 15:12:45 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-01-20 15:14:20 UTC
Colin,
As you are the main author of Koha::DateUtils, I really would like to get your opinion on this patch set :)
Especially on the QA comment (second patch).
Comment 6 Colin Campbell 2015-01-20 16:21:40 UTC
(In reply to Jonathan Druart from comment #0)
> For a couple of reasons, dt_from_string should not use
> DateTime::Format::DateParse:
> 1/ It does not manage date < 1900 (certainly caused by l.47 of this module:
>      $p{ year }      = $year ? $year + 1900 : DateTime->now->year;
> 2/ It considers 31/01/2015 as a valid us date, which is not.

The reason dates before 1900 are not handled is in the doc for Date::Parse 

    Date::Parse uses Time::Local internally, so is limited to only parsing
    dates which result in valid values for Time::Local::timelocal. This
    generally means dates between 1901-12-17 00:00:00 GMT and 2038-01-16
    23:59:59 GMT

That wasnt a problem with DateUtils as it was just ensuring the old C4::Dates routines could be handled which were also unable to handle pre 1900 dates
Comment 7 Colin Campbell 2015-01-20 16:24:58 UTC
(In reply to Jonathan Druart from comment #2)
> Created attachment 35406 [details] [review] [review]
> Bug 13601: Make dt_from_string not using DateTime::Format::DateParse
> 
> For a couple of reasons, dt_from_string should not use
> DateTime::Format::DateParse:
> 1/ It does not manage date < 1900, certainly caused by l.47 of this
> module:
>  $p{ year }      = $year ? $year + 1900 : DateTime->now->year;
> 
> 2/ It considers 31/01/2015 as a valid us date, which is not.
> 
> Test plan:
> 1/ Verify that
>   prove t/DateUtils.t
> returns green
> 2/ Play with dates in Koha (yes I know, it's vague...)
> 3/ Try to find a regression with dates
> 4/ Create a date with year <= 1900 and confirm it works
> 
> QA comment:
> Why the sql format switch was:
> -                $date_string =~
> -s/(\d{4})(\d{2})(\d{2})\s+(\d{2})(\d{2})(\d{2})/$1-$2-$3T$4:$5:$6/;
> 
> From where a date like "yyyymmdd hhmmss" can come?

C4::Dates supported the format so DateUtils had to accept it.
Comment 8 Jonathan Druart 2015-01-21 11:36:03 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-01-21 11:37:12 UTC
> display date on 4 digits and month/day on 2 digits.

It was

display *year* on 4 digits and month/day on 2 digits.
Comment 10 M. Tompsett 2015-03-07 15:49:20 UTC
There are conflicts in:
- t/DateUtils.t
- Koha/DateUtils.pm
- and something sha1 blob fatal in basket.pl
Comment 11 Jonathan Druart 2015-03-09 08:39:59 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-03-09 08:40:02 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2015-03-09 08:40:06 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2015-03-09 08:40:09 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2015-03-09 08:40:13 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2015-03-09 08:42:11 UTC
Rebase not trivial, please check there is no regression with bug 12669.
Comment 17 Marc Véron 2015-03-09 17:12:43 UTC
Patch did not apply:

Applying: Bug 13601: Add tests to highlight the problems with DateTime::Format::DateParse
Applying: Bug 13601: Make dt_from_string not using DateTime::Format::DateParse
Applying: Bug 13601: get rid of DateTime::Format::DateParse
Applying: Bug 13601: Add a fallback check for compability with existing code
Applying: Bug 13601: Fix special case in basket.pl
fatal: corrupt patch at line 55
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 13601: Fix special case in basket.pl
Comment 18 Marc Véron 2015-03-09 17:53:28 UTC Comment hidden (obsolete)
Comment 19 Marc Véron 2015-03-09 18:09:03 UTC Comment hidden (obsolete)
Comment 20 Marc Véron 2015-03-09 18:13:37 UTC Comment hidden (obsolete)
Comment 21 Marc Véron 2015-03-09 18:23:27 UTC Comment hidden (obsolete)
Comment 22 Marc Véron 2015-03-09 18:27:40 UTC
After signing off the first 4 patches I tried again with "Fix secial case in basket.pl", this one still does not apply with:
fatal: corrupt patch at line 55
Comment 23 Jonathan Druart 2015-03-10 08:17:38 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2015-03-10 08:20:44 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2015-03-10 08:21:49 UTC
(In reply to Marc Véron from comment #22)
> After signing off the first 4 patches I tried again with "Fix secial case in
> basket.pl", this one still does not apply with:
> fatal: corrupt patch at line 55

I c/p a diff in the commit message, git does not like that :)
Should be fixed now (I added a tab at the beginning of the lines).
Comment 26 Marc Véron 2015-03-10 09:14:34 UTC Comment hidden (obsolete)
Comment 27 Marc Véron 2015-03-10 09:16:05 UTC
Hi Jonathan,

Is it possible to extract the regexp part in patch "Make dt_from_string not using DateTime::Format::DateParse" to a separate sub (as replacement for the regexp function in C4::Dates)?

Or should it be done in Bug 13813?

Marc
Comment 28 Jonathan Druart 2015-03-10 09:19:46 UTC
(In reply to Marc Véron from comment #27)
> Hi Jonathan,
> 
> Is it possible to extract the regexp part in patch "Make dt_from_string not
> using DateTime::Format::DateParse" to a separate sub (as replacement for the
> regexp function in C4::Dates)?
> 
> Or should it be done in Bug 13813?
> 
> Marc

Hello Marc,
I would prefer not to add any other changes to this patch set.
Comment 29 Marc Véron 2015-03-10 09:39:04 UTC
Ok, I set to Signed-off and hope that it will be soon QAed and pushed to master :-)
Comment 30 Katrin Fischer 2015-03-15 22:18:06 UTC
This breaks t/Letters.t - tests pass OK on master:

perl t/Letters.t 
1..5
ok 1 - use C4::Letters;
ok 2 - letter name for "ISBN" letter is book
ok 3 - GetLetters returns the 2 inserted letters
Use of uninitialized value $date_format in string eq at /home/katrin/kohaclone/Koha/DateUtils.pm line 79.
Use of uninitialized value $date_format in string eq at /home/katrin/kohaclone/Koha/DateUtils.pm line 89.
Use of uninitialized value $date_format in string eq at /home/katrin/kohaclone/Koha/DateUtils.pm line 99.
Use of uninitialized value $date_format in string eq at /home/katrin/kohaclone/Koha/DateUtils.pm line 99.
Use of uninitialized value $date_format in concatenation (.) or string at /home/katrin/kohaclone/Koha/DateUtils.pm line 104.
Invalid dateformat parameter () at /home/katrin/kohaclone/Koha/DateUtils.pm line 104.
# Looks like you planned 5 tests but ran 3.
# Looks like your test exited with 255 just after 3.
Comment 31 Jonathan Druart 2015-03-16 13:55:28 UTC Comment hidden (obsolete)
Comment 32 Marc Véron 2015-03-16 14:38:14 UTC Comment hidden (obsolete)
Comment 33 Katrin Fischer 2015-03-16 22:14:46 UTC
I am having problems checking out items with a certain due time with this date.

For a test I am checking out an item, with the due date specified to:
19/03/2015 07:45
But the item checks out with:
19/03/2015 00:00

But... in the database it shows the correct due date, it's just not being displayed?
Comment 34 Jonathan Druart 2015-03-17 08:47:01 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2015-03-17 08:48:15 UTC
(In reply to Katrin Fischer from comment #33)
> I am having problems checking out items with a certain due time with this
> date.
> 
> For a test I am checking out an item, with the due date specified to:
> 19/03/2015 07:45
> But the item checks out with:
> 19/03/2015 00:00
> 
> But... in the database it shows the correct due date, it's just not being
> displayed?

Good catch Katrin!
All sql dates were impacted.
Comment 36 Marc Véron 2015-03-20 08:59:46 UTC Comment hidden (obsolete)
Comment 37 Katrin Fischer 2015-03-22 19:56:59 UTC
Created attachment 37087 [details] [review]
[PASSED QA] Bug 13601: Add tests to highlight the problems with DateTime::Format::DateParse

Test behaves as expected (Fail as expected  with first patch only, OK after 3rd patch)
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 38 Katrin Fischer 2015-03-22 19:57:02 UTC
Created attachment 37088 [details] [review]
[PASSED QA] Bug 13601: Make dt_from_string not using DateTime::Format::DateParse

For a couple of reasons, dt_from_string should not use
DateTime::Format::DateParse:
1/ It does not manage date < 1900, certainly caused by l.47 of this
module:
 $p{ year }      = $year ? $year + 1900 : DateTime->now->year;

2/ It considers 31/01/2015 as a valid us date, which is not.

Test plan:
1/ Verify that
  prove t/DateUtils.t
returns green
2/ Play with dates in Koha (yes I know, it's vague...)
3/ Try to find a regression with dates
4/ Create a date with year <= 1900 and confirm it works

QA comment:
Why the sql format switch was:
-                $date_string =~
-s/(\d{4})(\d{2})(\d{2})\s+(\d{2})(\d{2})(\d{2})/$1-$2-$3T$4:$5:$6/;

From where a date like "yyyymmdd hhmmss" can come?

Tested patches 1 - 3 together. Worked as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 39 Katrin Fischer 2015-03-22 19:57:06 UTC
Created attachment 37089 [details] [review]
[PASSED QA] Bug 13601: get rid of DateTime::Format::DateParse

This module is used in C4::Members::GetPendingIssues too, but we can use
dt_from_string.

Test plan:
1/ Verify that
    prove t/db_dependent/Members/GetPendingIssues.t
returns green
2/ On the patron pending issue list, verify that the issue and the due
dates are correctly displayed.

Tested together with other patches (except "Fix special cases). Worked as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 40 Katrin Fischer 2015-03-22 19:57:09 UTC
Created attachment 37090 [details] [review]
[PASSED QA] Bug 13601: Add a fallback check for compability with existing code

There are a lot of places where the date comes from the DB but the
dateformat parameter is not set to 'sql'.
dt_from_string needs to fallback with this format if the pref format
does not match.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 41 Katrin Fischer 2015-03-22 19:57:13 UTC
Created attachment 37091 [details] [review]
[PASSED QA] Bug 13601: Fix special case in basket.pl

There is a badly managed date in acqui/basket.pl:
if the date is 15/01/2015 (metric format), it will become
2015-1-15 after the following line:
  $estimateddeliverydate = "$year-$month-$day";
Add_Delta_Days is used at several place, and the ouput is forced to
display date on 4 digits and month/day on 2 digits.
This patch does the same thing for $estimateddeliverydate.

Note that I previously developed a patch to take this format into account (with missing 0)
in Koha::DateUtils::dt_from_string, but I don't think it's a good idea
to manage bad formated dates.
We will certainly find some issues after previous patches, but it will permit to catch
them!
IMO it's preferable than to keep them hidden.

The patch was:
    diff --git a/Koha/DateUtils.pm b/Koha/DateUtils.pm
    index 5fe2653..4434a67 100644
    --- a/Koha/DateUtils.pm
    +++ b/Koha/DateUtils.pm
    @@ -72,17 +72,17 @@ sub dt_from_string {
             my $fallback_re = qr|
                 (?<year>\d{4})
                 -
    -            (?<month>\d{2})
    +            (?<month>\d{1,2})
                 -
    -            (?<day>\d{2})
    +            (?<day>\d{1,2})
             |xms;

             if ( $date_format eq 'metric' ) {
                 # metric format is "dd/mm/yyyy[ hh:mm:ss]"
                 $regex = qr|
    -                (?<day>\d{2})
    +                (?<day>\d{1,2})
                     /
    -                (?<month>\d{2})
    +                (?<month>\d{1,2})
                     /
                     (?<year>\d{4})
                 |xms;
    @@ -90,9 +90,9 @@ sub dt_from_string {
             elsif ( $date_format eq 'us' ) {
                 # us format is "mm/dd/yyyy[ hh:mm:ss]"
                 $regex = qr|
    -                (?<month>\d{2})
    +                (?<month>\d{1,2})
                     /
    -                (?<day>\d{2})
    +                (?<day>\d{1,2})
                     /
                     (?<year>\d{4})
                 |xms;
    diff --git a/t/DateUtils.t b/t/DateUtils.t
    index 886e1d6..0877240 100755
    --- a/t/DateUtils.t
    +++ b/t/DateUtils.t
    @@ -189,3 +189,8 @@ is( output_pref( { dt => $dt } ), '31/01/2015 12:34', 'dt_from_string should mat
     # date before 1900
     $dt = dt_from_string('01/01/1900');
     is( output_pref( { dt => $dt, dateonly => 1 } ), '01/01/1900', 'dt_from_string should manage date < 1900' );
    +
    +# missing 0
    +$dt = dt_from_string('1/1/2015');
    +is( output_pref( { dt => $dt, dateonly => 1 } ), '01/01/2015', 'dt_from_string should generate a DT object even if 0 are missing' );

Works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 42 Katrin Fischer 2015-03-22 19:57:16 UTC
Created attachment 37092 [details] [review]
[PASSED QA] Bug 13601: t/Letters.t needs to mock the dateformat pref

Test passes OK.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 43 Katrin Fischer 2015-03-22 19:57:21 UTC
Created attachment 37093 [details] [review]
[PASSED QA] Bug 13601: The fallback regex should contain the time part

On displaying a sql date, if the dateformat is not set to sql, the
output should contain the date.
In the previous version, the fallback regex (used for sql format) did not
included the time part (hh:mm:ss).

Checked out an item with due date an ddue time. Time displays as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 44 Tomás Cohen Arazi 2015-03-30 16:50:20 UTC
Patches pushed to master.

Good job Jonathan!