Bug 13075 - Use of uninitialized value while proving db_dependent/Holds.t
Summary: Use of uninitialized value while proving db_dependent/Holds.t
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low minor (vote)
Assignee: Mark Tompsett
QA Contact: Testopia
URL:
Keywords:
: 12928 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-10-12 00:27 UTC by Mark Tompsett
Modified: 2015-06-04 23:32 UTC (History)
2 users (show)

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


Attachments
Bug 13075 - Silence warnings and improve Charset testing. (2.48 KB, patch)
2014-10-12 00:50 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 13075 - Silence warnings and improve Charset testing. (2.52 KB, patch)
2014-10-14 18:25 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 13075 - Silence warnings and improve Charset testing. (2.54 KB, patch)
2014-10-23 17:20 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13075: Silence warnings and improve Charset testing. (2.61 KB, patch)
2014-11-12 23:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13075: (followup) remove remaining warnings (2.20 KB, patch)
2014-11-12 23:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13075: (followup) remove remaining warnings (2.36 KB, patch)
2014-11-13 00:40 UTC, Mark Tompsett
Details | Diff | Splinter Review
[PASSED QA] Bug 13075: Silence warnings and improve Charset testing. (2.66 KB, patch)
2014-11-13 20:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13075: (followup) remove remaining warnings (2.41 KB, patch)
2014-11-13 20:24 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 Mark Tompsett 2014-10-12 00:27:36 UTC
Calls to C4/Charset.pm's NormalizeString function with an undefined string was triggering errors when running: prove -v t/db_dependent/Holds.t

Sadly, t/Charset.t is lacking any function calls to NormalizeString.

Silence the errors, and add minimal tests to t/Charset.t for testing.
Comment 1 Mark Tompsett 2014-10-12 00:50:46 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2014-10-14 15:24:32 UTC
Comment on attachment 32282 [details] [review]
Bug 13075 - Silence warnings and improve Charset testing.

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

::: C4/Charset.pm
@@ +178,4 @@
>  
>  sub NormalizeString{
>  	my ($string,$nfd,$transform)=@_;
> +    return $string if !defined($string);

could we write this better as return unless $string;

Or should we be actually croaking if we call this without actually passing it anything to normalize, instead of silently hiding that we are calling it wrong?
Comment 3 Mark Tompsett 2014-10-14 18:24:14 UTC
(In reply to Chris Cormack from comment #2)
> > +    return $string if !defined($string);
> 
> could we write this better as return unless $string;

return unless defined($string), perhaps. String could be 0 or q{}.


> Or should we be actually croaking if we call this without actually passing
> it anything to normalize, instead of silently hiding that we are calling it
> wrong?

I did not determine why undef was passed, but perhaps further investigation as to why undef is passed would clarify if croak is necessary. Seeing as no other warnings were generated with this solution, I don't think it is.

It should be noted that NFC(undef) and NFD(undef) both return the empty string, so perhaps another solution (possibly better?) is: $string //= q{};

Though, I think it is rather ugly to mangle an undef into an empty string to avoid warnings. That is why I choose this current solution.

I'll rework this to the 'unless' idea that you suggested, Chris. -- time passes -- While doing this, I discovered that a NormalizeString call is pushed into an array, and so the context is a list, so return unless defined($string); triggers warnings while trying to run prove -v t/db_dependent/Holds.t for testing. Inserting an explicit $string (return $string unless defined($string)) forces a scalar return, and prevents warnings.
Comment 4 Mark Tompsett 2014-10-14 18:25:59 UTC Comment hidden (obsolete)
Comment 5 Owen Leonard 2014-10-23 16:05:17 UTC
Auto-merging t/Charset.t
CONFLICT (content): Merge conflict in t/Charset.t
Auto-merging C4/Charset.pm
Failed to merge in the changes.
Comment 6 Mark Tompsett 2014-10-23 17:20:34 UTC Comment hidden (obsolete)
Comment 7 Mark Tompsett 2014-10-23 17:21:09 UTC
Rebased. Another bug fix had added an additional test.
Comment 8 Mark Tompsett 2014-10-23 17:24:05 UTC
Oops. Still working on it.
Comment 9 Mark Tompsett 2014-10-23 17:31:49 UTC
Nope. All good. Less speed, more haste. :) This needs testing as per the test plan in comment #6.
Comment 10 Tomás Cohen Arazi 2014-11-12 23:43:13 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2014-11-12 23:43:19 UTC Comment hidden (obsolete)
Comment 12 Mark Tompsett 2014-11-13 00:40:05 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2014-11-13 20:23:33 UTC
Created attachment 33532 [details] [review]
[PASSED QA] Bug 13075: Silence warnings and improve Charset testing.

Calls to C4/Charset.pm's NormalizeString function with an
undefined string were triggering warnings when running:
  prove -v t/db_dependent/Holds.t

Sadly, t/Charset.t was also lacking calls to NormalizeString.

TEST PLAN
---------
1) prove -v t/db_dependent/Holds.t
   -- This should generate the uninitialized string warnings.
      Make sure CPL and MPL are in your branches to save
      yourself from headaches due to expected data.
2) cat t/Charset.t
   -- note there are no function calls to NormalizeString.
      You can see other shortfalls in the tests beyond
      NormalizeString with: grep ^sub C4/Charset.pm
3) prove -v t/Charset.t
4) Apply patch
5) prove -v t/Charset.t
   -- Run as before with more tests.
6) cat t/Charset.t
   -- note there are now function calls to NormalizeString.
7) prove -v t/db_dependent/Holds.t
   -- Nice and clean run! :)
8) koha-qa.pl -v 2 -c 1
   -- all should be Ok.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2014-11-13 20:24:09 UTC
Created attachment 33533 [details] [review]
[PASSED QA] Bug 13075: (followup) remove remaining warnings

There's no point creating a MARC record with undef subfields
for testing holds. This patch avoids that so no warnings are shown.

To test:
- Run
  $ prove t/db_dependent/Holds.t
=> FAIL: verify several warnings show
- Apply the patch
- Re-run
=> SUCCESS: no warnings showed.
- Sign off :-D

Regards

NOTE: Not noticable under Ubuntu 12.04 LTS, but verifiable under
      Debian Wheezy.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Tomás Cohen Arazi 2014-11-14 12:40:37 UTC
Patches pushed to master.

Thanks Mark!
Comment 16 Mark Tompsett 2014-11-19 18:07:57 UTC
*** Bug 12928 has been marked as a duplicate of this bug. ***