http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13075 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|koha-bugs@lists.koha-commun |mtompset@hotmail.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13075 --- Comment #1 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 32282 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32282&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13075 --- Comment #3 from M. Tompsett <mtompset@hotmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13075 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply --- Comment #5 from Owen Leonard <oleonard@myacpl.org> --- Auto-merging t/Charset.t CONFLICT (content): Merge conflict in t/Charset.t Auto-merging C4/Charset.pm Failed to merge in the changes. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13075 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #7 from M. Tompsett <mtompset@hotmail.com> --- Rebased. Another bug fix had added an additional test. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13075 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #9 from M. Tompsett <mtompset@hotmail.com> --- Nope. All good. Less speed, more haste. :) This needs testing as per the test plan in comment #6. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13075 --- Comment #11 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 33504 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33504&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13075 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33504|0 |1 is obsolete| | --- Comment #12 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 33505 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33505&action=edit [SIGNED OFF] 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13075 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33503|0 |1 is obsolete| | Attachment #33505|0 |1 is obsolete| | --- Comment #13 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 33532 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33532&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13075 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |tomascohen@gmail.com --- Comment #15 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patches pushed to master. Thanks Mark! -- You are receiving this mail because: You are watching all bug changes.