Bug 4450

---- Reported by colin.campbell@ptfs-europe.com 2010-05-05 12:25:23 ----

Site reported this error:

"Can't use string ("-1") as an ARRAY ref while "strict refs" in use at /home/koha/kohaclone/C4/Creators/Lib.pm line 538."

Cause was a db error which had been logged but the return which should have been an array ref was a scalar (-1) Routines in C4/Creators are defaulting to -1 returns in error conditions. It is safer and more robust to pass undef when the calling code is expecting a reference or blessed reference to be returned, or an empty array when an array is expected. 

Patch to follow



---- Additional Comments From colin.campbell@ptfs-europe.com 2010-05-05 12:43:56 ----

Created an attachment
Proposed Patch

More robust error returns 



---- Additional Comments From gmcharlt@gmail.com 2010-05-05 12:49:21 ----

I agree with the patch in principle, but request testing and sign-off by Chris given the number of methods changed.



---- Additional Comments From cnighswonger@foundations.edu 2010-05-06 17:16:02 ----

This looks good. I've sent it along to patches@



---- Additional Comments From gmcharlt@gmail.com 2010-05-06 18:26:27 ----

Patch pushed.  Please test and close.



---- Additional Comments From 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.



---- Additional Comments From colin.campbell@ptfs-europe.com 2010-05-07 10:25:27 ----

>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)

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



---- Additional Comments From 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.



---- Additional Comments From colin.campbell@ptfs-europe.com 2010-05-07 14:06:10 ----

I was trying to avoid code like this:
return undef is bad because it is ambiguous in a list context use return; For example in the elsif case below if this is called in
a list context you are now returning not an error but a a one element array
(@{$data) has been expanded from [] to [ undef, ]
-

if ((ref($data) eq 'SCALAR') && ($data == -1)) {
    return undef;
}
elsif ((ref($data) eq 'ARRAY') && (scalar(@$data) == 0)) {
    return undef;
}

In the patch I was trying to ensure that error conditions were being caught by making the returns consistent with how the calling routines were actually testing the return and going for a perlish default where the returns were not currently being tested. (in the hopes that this might be how people would expect to test them)




---- Additional Comments From 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?



---- Additional Comments From cnighswonger@foundations.edu 2010-05-07 14:53:58 ----

>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.



---- Additional Comments From 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.




---- Additional Comments From cnighswonger@foundations.edu 2010-05-07 17:00:47 ----

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



---- Additional Comments From 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.



---- Additional Comments From 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.



---- Additional Comments From gmcharlt@gmail.com 2010-05-14 00:21:30 ----

I confirm the test results and have reverted the patch.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:27 UTC  ---

This bug was previously known as _bug_ 4450 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4450
Imported an attachment (id=2119)

Actual time not defined. Setting to 0.0
Setting qa contact to the default for this product.
   This bug either had no qa contact or an invalid one.
The original submitter of attachment 2119 [details] [review] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.