Bug 16848 - Wrong warning "Invalid date ... passed to output_pref" can be carped
Summary: Wrong warning "Invalid date ... passed to output_pref" can be carped
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:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-04 14:31 UTC by Jonathan Druart
Modified: 2017-12-07 22:18 UTC (History)
5 users (show)

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


Attachments
Bug 16848: Prevent invalid warning to be carped from output_pref (1.55 KB, patch)
2016-07-04 15:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16848: Prevent invalid warning to be carped from output_pref (1.62 KB, patch)
2016-07-06 02:51 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[PASSED QA] Bug 16848: Prevent invalid warning to be carped from output_pref (1.68 KB, patch)
2016-08-07 12:03 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 2016-07-04 14:31:21 UTC
From Koha::DateUtils::output_pref:

   $dt = eval { dt_from_string( $str ) } if $str;
   carp "Invalid date '$str' passed to output_pref\n" if $@;

This second line is wrong: if $str does not exist, the first line is not evaluated and $@ could be filled with previous error.

To reproduce:

diff --git a/t/DateUtils.t b/t/DateUtils.t
index eede4a0..2dd6a9d 100755
--- a/t/DateUtils.t
+++ b/t/DateUtils.t
@@ -229,3 +229,5 @@ is( $output_for_invalid_date, undef, 'output_pref should return undef if an inva
 warning_is { output_pref( { 'str' => $testdate_iso, dt => $dt, dateformat => 'iso', dateonly => 1 } ) }
            { carped => 'output_pref should not be called with both dt and str parameters' },
            'output_pref should carp if str and dt parameters are passed together';
+
+output_pref();


Then:
  prove t/DateUtils.t
will display:
t/DateUtils.t .. 20/60 Use of uninitialized value $str in concatenation (.) or string at Koha/DateUtils.pm line 217.
Invalid date '' passed to output_pref
 at t/DateUtils.t line 233.
t/DateUtils.t .. ok     
All tests successful.
Files=1, Tests=60,  2 wallclock secs ( 0.02 usr  0.00 sys +  1.40 cusr  0.00 csys =  1.42 CPU)
Result: PASS
Comment 1 Jonathan Druart 2016-07-04 15:54:13 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2016-07-06 02:51:37 UTC Comment hidden (obsolete)
Comment 3 Srdjan Jankovic 2016-07-06 02:52:30 UTC
I took liberty of adding local $@
Comment 4 Jonathan Druart 2016-07-06 13:33:47 UTC
(In reply to Srdjan Jankovic from comment #3)
> I took liberty of adding local $@

Are you sure it is useful?
Comment 5 Srdjan Jankovic 2016-07-07 01:19:57 UTC
Probably not that much, but I have a habit of doing that, I believe it is a good practice.
Comment 6 Katrin Fischer 2016-07-13 20:28:56 UTC
Jonathan, Srdjan, is this ready for QA?
Comment 7 Srdjan Jankovic 2016-07-13 23:57:09 UTC
I think so, unless Jonathan fiercely objects, in which case I'll remove the line that I've added.
Comment 8 Jonathan Druart 2016-07-15 12:56:14 UTC
(In reply to Srdjan Jankovic from comment #7)
> I think so, unless Jonathan fiercely objects, in which case I'll remove the
> line that I've added.

It should not hurt, so I am ok with it.
Comment 9 Katrin Fischer 2016-08-07 12:03:12 UTC
Created attachment 54121 [details] [review]
[PASSED QA] Bug 16848: Prevent invalid warning to be carped from output_pref

From Koha::DateUtils::output_pref:

   $dt = eval { dt_from_string( $str ) } if $str;
   carp "Invalid date '$str' passed to output_pref\n" if $@;

This second line is wrong: if $str does not exist, the first line is not evaluated and $@ could be filled with previous error.

To reproduce:

Then:
  prove t/DateUtils.t
will display:
t/DateUtils.t .. 20/60 Use of uninitialized value $str in concatenation (.) or string at Koha/DateUtils.pm line 217.
Invalid date '' passed to output_pref
 at t/DateUtils.t line 233.
t/DateUtils.t .. ok
All tests successful.
Files=1, Tests=60,  2 wallclock secs ( 0.02 usr  0.00 sys +  1.40 cusr  0.00 csys =  1.42 CPU)
Result: PASS

Test plan:
Without this patch, you should not see the carp

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Kyle M Hall 2016-08-10 13:43:32 UTC
Pushed to master for 16.11, thanks Jonathan!
Comment 11 Frédéric Demians 2016-08-23 09:43:07 UTC
Pushed in 16.05. Will be in 16.05.03.
Comment 12 Frédéric Demians 2016-08-23 09:43:45 UTC
Pushed in 16.05. Will be in 16.05.03.
Comment 13 Julian Maurice 2016-08-23 15:37:06 UTC
Pushed to 3.22.x, will be in 3.22.10