Bug 12916

Summary: Use of uninitialized values $max and $min in C4/Tags.pm
Product: Koha Reporter: Marc Véron <veron>
Component: OPACAssignee: Marc Véron <veron>
Status: CLOSED FIXED QA Contact:
Severity: trivial    
Priority: P5 - low CC: mtompset, tomascohen
Version: 3.16   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: Trivial patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 12916 - Use of uninitialized values $max and $min in C4/Tags.pm
[SIGNED-OFF] Bug 12916 - Use of uninitialized values $max and $min in C4/Tags.pm
Bug 12916 - Missing Test to demonstrate warnings.
Bug 12916 - Missing Test to demonstrate warnings.
Bug 12916 - Missing Test to demonstrate warnings.
Bug 12916 - Use of uninitialized values $max and $min in C4/Tags.pm
Bug 12916 - Missing Test to demonstrate warnings.
Bug 12916 - Use of uninitialized values $max and $min in C4/Tags.pm
[PASSED QA] Bug 12916 - Missing Test to demonstrate warnings.
[PASSED QA] Bug 12916 - Use of uninitialized values $max and $min in C4/Tags.pm

Description Marc Véron 2014-09-14 07:04:45 UTC
If tag list is empty, sub stratify_tags issues a warning in opac-error.log because of uninitialized values $max and $min in C4/Tags.pm, line 600
Comment 1 Marc Véron 2014-09-20 18:29:46 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2014-10-20 13:17:19 UTC
Still applies on current master: 3.17.00.029
Comment 3 Owen Leonard 2014-10-22 16:47:57 UTC Comment hidden (obsolete)
Comment 4 Mark Tompsett 2014-10-22 19:06:24 UTC
Comment on attachment 32590 [details] [review]
[SIGNED-OFF] Bug 12916 - Use of uninitialized values $max and $min in C4/Tags.pm

Review of attachment 32590 [details] [review]:
-----------------------------------------------------------------

::: C4/Tags.pm
@@ +589,4 @@
>  sub stratify_tags {
>      my ( $strata, $tags ) = @_;
>  
> +    my ( $min, $max ) = ( 0, 0 );

I'm not sure this is the correct solution.
Look at 595. There is a !defined($min) check.
The problem is there is nothing in the @$tags.
By initializing to 0, if there are tags, and the minimum is 5, the minimum is never set correctly.
Similarly, if there are tags, and the maximum is negative, the maximum is never set correctly.
Comment 5 Mark Tompsett 2014-10-22 19:52:35 UTC Comment hidden (obsolete)
Comment 6 Mark Tompsett 2014-10-22 19:53:35 UTC
After further code reading and test, the proposed solution is adequate. Here are the missing tests to make testing trivial. :)
Comment 7 Mark Tompsett 2014-10-22 19:58:08 UTC Comment hidden (obsolete)
Comment 8 Mark Tompsett 2014-10-22 22:50:44 UTC Comment hidden (obsolete)
Comment 9 Mark Tompsett 2014-10-22 22:58:41 UTC
Sorry, prove -v t/db_dependent/Tags.t fails with signed off patch.
Comment 10 Mark Tompsett 2014-10-23 01:01:53 UTC Comment hidden (obsolete)
Comment 11 Chris Cormack 2014-10-25 03:22:22 UTC Comment hidden (obsolete)
Comment 12 Chris Cormack 2014-10-25 03:23:35 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2014-10-25 13:16:07 UTC
Created attachment 32732 [details] [review]
[PASSED QA] Bug 12916 - Missing Test to demonstrate warnings.

Modifying C4/Tags.pm should have tests.

TEST PLAN
---------
1) Apply this test patch only.
2) prove -v t/db_dependent/Tags.t
   -- should see two warnings.
3) Apply the first patch.
4) prove -v t/db_dependent/Tags.t
   -- no warnings.

Signed-off-by: Chris <chris@bigballofwax.co.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2014-10-25 13:16:20 UTC
Created attachment 32733 [details] [review]
[PASSED QA] Bug 12916 - Use of uninitialized values $max and $min in C4/Tags.pm

When the stratify_tags function is called without any tags, the
$min and $max variables are left undefined, which triggers a
warning message about uninitialized values in the subtraction.

TEST PLAN
---------
1) prove -v t/db_dependent/Tags.t
   -- should succeed, but there is no test for empty tags.
2) Make sure that you have no tags
3) In Opac, go to 'Tag cloud'
4) Check the opac error log file
   (e.g. ~/koha-dev/var/log/koha-opac-error_log) for entries
   regarding uninitialized values $max and $min in C4/Tags.pm
5) apply this patch
6) prove -v t/db_dependent/Tags.t
   -- should still succeed, though no tests are added.
7) Reload page 'Tag cloud'
   -- Result: No additional warnings in opac error log file.

Signed-off-by: Chris <chris@bigballofwax.co.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
New and old tests pass. Works as described.
Comment 15 Tomás Cohen Arazi (tcohen) 2014-10-30 03:34:59 UTC
Patches pushed to master.

Thanks Marc and Mark!
Comment 16 Mark Tompsett 2015-02-09 05:48:59 UTC
Could this be included in 3.16.x? It applies cleanly.