http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4450 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |normal Priority|P5 |PATCH-Sent -- Configure bugmail: http://bugs.koha.org/cgi-bin/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.

http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4450 Chris Nighswonger <cnighswonger@foundations.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|cnighswonger@foundations.edu|colin.campbell@ptfs- | |europe.com -- Configure bugmail: http://bugs.koha.org/cgi-bin/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.

http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4450 --- Comment #3 from Chris Nighswonger <cnighswonger@foundations.edu> 2010-05-06 17:16:02 --- This looks good. I've sent it along to patches@ -- Configure bugmail: http://bugs.koha.org/cgi-bin/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.

http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4450 --- Comment #5 from Chris Nighswonger <cnighswonger@foundations.edu> 2010-05-06 18:51:49 --- Further thought (ie. I was too quick on the trigger.) has revealed that this change will cause the error trapping to be rendered useless in the higher level label/card scripts. That code depends upon the return value being -1 if there is an error. Within the context of labels/cards, -1 should never be valid data since all valid data either consists of an index field or a data structure (ie. hash or array). If returning null on db errors is the preferred technique in these cases, then this patch should also visit all of the error trapping code at higher levels to port the change there as well. Otherwise we should only address the line of code which caused the error to be thrown in this case. -- Configure bugmail: http://bugs.koha.org/cgi-bin/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.

http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4450 --- Comment #7 from Chris Nighswonger <cnighswonger@foundations.edu> 2010-05-07 13:32:37 ---

The initial perceived issue is that where return is a hash or array we're dereferencing -1 and crashing and burning

Right. I'm not arguing the validity of the bug. However, in fixing this bug, we should not destroy the error trapping capabilities of the code. Error trapping is something the entire code base is badly in need of. Hence my recommendation that we either: a) do something like: if ((ref($data) eq 'SCALAR') && ($data == -1)) { return undef; } elsif ((ref($data) eq 'ARRAY') && (scalar(@$data) == 0)) { return undef; } which would fix the problem and preserve error trapping in other portions of code. b) port changes to the return values of the methods to the error trapping code higher up. In any case, the fix should preserve error trapping higher up. -- Configure bugmail: http://bugs.koha.org/cgi-bin/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.

http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4450 --- Comment #9 from Galen Charlton <gmcharlt@gmail.com> 2010-05-07 14:32:50 --- (In reply to comment #7)

However, in fixing this bug, we should not destroy the error trapping capabilities of the code. Error trapping is something the entire code base is badly in need of.

Stepping back a bit: how do the callers of those methods *currently* handle errors? I.e., is the error trapping you're proposing actually in use in some form? -- Configure bugmail: http://bugs.koha.org/cgi-bin/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.

http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4450 --- Comment #11 from Galen Charlton <gmcharlt@gmail.com> 2010-05-07 16:47:08 --- (In reply to comment #10)

...

Stepping back a bit: how do the callers of those methods *currently* handle errors? I.e., is the error trapping you're proposing actually in use in some form?

Examples:

labels/label-edit-layout.pl - line 144 labels/label-manage.pl - line 79 (the entire if block) patroncards/edit-batch.pl - lines 68-73, 76-80, 87-92, patroncards/edit-layout.pl - line 224 patroncards/edit-template.pl - line 88 (which needs to be changed from 'die' to 'warn') patroncards/manage.pl - line 78 (the entire if block)

etc.

But not all of those involve methods that Colin's patch had touched. For example, C4::Creators::Batch::remove_items wasn't one of the ones changed - I think he was just changing methods that, on success, return an object or a data structure, not those that perform an access and just return an integer with a success flag. -- Configure bugmail: http://bugs.koha.org/cgi-bin/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.

http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4450 --- Comment #13 from Galen Charlton <gmcharlt@gmail.com> 2010-05-07 17:14:04 --- (In reply to comment #12)

After a further look, I'm ok with the patch as is. Thanks Colin.

I've reinstated it. Please test and close. -- Configure bugmail: http://bugs.koha.org/cgi-bin/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.

http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4450 Chris Nighswonger <cnighswonger@foundations.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |REOPENED Resolution|FIXED | --- Comment #14 from Chris Nighswonger <cnighswonger@foundations.edu> 2010-05-13 22:59:35 --- Unfortunately I ran into more regressions caused by this patch today. While doing some work on a client installation (code from current HEAD), I found that attempting to create a new layout results in the following error: Can't bless non-reference value at /home/cnighswonger/Repositories/koha.3.2.labels/C4/Creators/Layout.pm line 107. Additionally, attempting to add an item to a new label batch results in the following error: Can't call method "add_item" on an undefined value at /home/cnighswonger/Repositories/koha.3.2.labels/labels/label-edit-batch.pl line 83. Both regressions are resolved by reverting commit 1f56a04cadc60f26b36f5599f24d0f380bc23828 While I agree in principle with the need to improve the returns on errors in the C4::Creators module, I think that the solution needs to touch upon every line of code which will be affected by the improvement. It seems to me that this is better left to 3.4. In any case, any work which changes C4::Creators should also port those changes to all of the Label and Card creator code as well. So for the time being, I am withdrawing my sign-off on this patch and reopening the bug. But only until such a time as a complete solution is provided. -- Configure bugmail: http://bugs.koha.org/cgi-bin/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.

http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4450 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|rel_3_2 |rel_3_4 -- Configure bugmail: http://bugs.koha.org/cgi-bin/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.