Bug 14936 - Add validation for date strings to Koha::DateUtils
Summary: Add validation for date strings to Koha::DateUtils
Status: CLOSED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marc Véron
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-01 16:06 UTC by Marc Véron
Modified: 2016-06-21 21:37 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14936 - Add validation for date strings to Koha::DateUtils (5.05 KB, patch)
2015-10-01 16:13 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14936 - Add validation for date strings to Koha::DateUtils (5.05 KB, patch)
2015-10-01 16:16 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14936 - Add validation for date strings to Koha::DateUtils (6.29 KB, patch)
2015-10-01 19:37 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 14936 - Add validation for date strings to Koha::DateUtils (6.49 KB, patch)
2015-10-01 20:57 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2015-10-01 16:06:08 UTC
Patch to add a generic function in Koha::DateUtils to test a date string against the 'dateformat' syspref.
Comment 1 Marc Véron 2015-10-01 16:13:22 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2015-10-01 16:16:58 UTC Comment hidden (obsolete)
Comment 3 Marc Véron 2015-10-01 19:37:28 UTC Comment hidden (obsolete)
Comment 4 Héctor Eduardo Castro Avalos 2015-10-01 20:57:59 UTC
Created attachment 43033 [details] [review]
[SIGNED-OFF]Bug 14936 - Add validation for date strings to Koha::DateUtils

To test:
- Apply patch
- prove t/DateUtils.t

Bonus test: Apply patch(es) from Bug(s) that are blocked by this Bug
and test as appropriate.

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Tested in conjunction with patches 14923 and 14924 with different date formats
Tested with wrong formats 32/01/1970, 00-00-0000 and 0000-00-00
Test units pass OK
Comment 5 Jonathan Druart 2015-10-02 07:17:55 UTC
Marc,
This is not the way to go, for several reasons.
The main reason is that you c/p dt_from_string to create the new subroutine, which is bad.
Then, if you have a look at dt_from_string, you will note 1 eval around the first DateTime->new (l. 156), but not around the second one (l. 164).
So I think there are at least 2 better ways to know if a date is not correctly formatted:
1/ add an eval around the second DateTime->new call, the subroutine will return undef and the caller will have to check if something has been returned ;
2/ or catch the croak raised by the DateTime in the callers

I don't know what I would prefer (a bit early and not drank enough tea yet).
Comment 6 Marc Véron 2015-10-02 08:59:29 UTC
Hi Jonathan,

Propositon 1/ changes the behaviour of dt_from_string. This function is spread all over the system, and we do not know what side effects it will have if we change the behavior regarding the return value. That's why I decided not to touch it at the moment.

Proposition 2/ Catch a croak: Is that done by surrounding dt_from_string with an eval? (I simply do not know)

My opinion is that it is much better to have a dedicated function to validate a date string. It works fine afak and it has tests. I know that it duplicates code, but that can easily be streamlined later, after we finally have added the long outstandig DD.MM.YYYY date format (Bug 12072).


(In reply to Jonathan Druart from comment #5)
> Marc,
> This is not the way to go, for several reasons.
> The main reason is that you c/p dt_from_string to create the new subroutine,
> which is bad.
> Then, if you have a look at dt_from_string, you will note 1 eval around the
> first DateTime->new (l. 156), but not around the second one (l. 164).
> So I think there are at least 2 better ways to know if a date is not
> correctly formatted:
> 1/ add an eval around the second DateTime->new call, the subroutine will
> return undef and the caller will have to check if something has been
> returned ;
> 2/ or catch the croak raised by the DateTime in the callers
> 
> I don't know what I would prefer (a bit early and not drank enough tea yet).
Comment 7 Marc Véron 2015-10-14 18:17:36 UTC
Date strings can be validated by using dt_from_string inside an eval{ }.