Description
Marc Véron
2014-09-14 07:04:45 UTC
Created attachment 31792 [details] [review] Bug 12916 - Use of uninitialized values $max and $min in C4/Tags.pm Test plan: Make sure that you have no tags In Opac, go to 'Tag cloud' Check opac-error.log for entries regarding uninitialized values $max and $min in C4/Tags.pm, line 600 Apply patch Reload page 'Tag cloud' Result: No more warnings in opac-error.log Still applies on current master: 3.17.00.029 Created attachment 32590 [details] [review] [SIGNED-OFF] Bug 12916 - Use of uninitialized values $max and $min in C4/Tags.pm Test plan: Make sure that you have no tags In Opac, go to 'Tag cloud' Check opac-error.log for entries regarding uninitialized values $max and $min in C4/Tags.pm, line 600 Apply patch Reload page 'Tag cloud' Result: No more warnings in opac-error.log Signed-off-by: Owen Leonard <oleonard@myacpl.org> 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. Created attachment 32604 [details] [review] 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. After further code reading and test, the proposed solution is adequate. Here are the missing tests to make testing trivial. :) Created attachment 32605 [details] [review] 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. Created attachment 32611 [details] [review] 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. Sorry, prove -v t/db_dependent/Tags.t fails with signed off patch. Created attachment 32612 [details] [review] 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. Created attachment 32700 [details] [review] 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> Created attachment 32701 [details] [review] 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> 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> 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. Patches pushed to master. Thanks Marc and Mark! Could this be included in 3.16.x? It applies cleanly. |