Bug 18022 - Empty barcode causes internal server error
Summary: Empty barcode causes internal server error
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 16.11
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-31 19:03 UTC by ClintD
Modified: 2017-05-03 03:52 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18022: Fix possible crash if CircAutoPrintQuickSlip=clear screen (1.82 KB, patch)
2017-02-24 11:02 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description ClintD 2017-01-31 19:03:15 UTC
Internal server error
The error is
Can't use string ("1") as a HASH ref while "strict refs" in use at /usr/share/koha/intranet/cgi-bin/circ/circulation.pl line 472

to reproduce:
1. Set the  CircAutoPrintQuickSlip system preference to "clear the screen."
2. Load a patron's account in circulation.
3. Click the "Checkout" button with an empty barcode field or hit enter.

Also reported to exist in 3.22.08 by Owen Leonard
Comment 2 Jonathan Druart 2017-02-01 08:14:33 UTC
Note, from the mailing list: I am not able to recreate this issue.
Comment 3 Jonathan Druart 2017-02-09 07:45:37 UTC
Clint, could to try and recreate on a sandbox?

What is the line 472 of circ/circulation.pl?
Comment 4 Koha Team Lyon 3 2017-02-23 09:56:06 UTC
Hello,
We experiment the same problem on our preprod install (3.22).
I'm not sure of this but probably It does not happen when the patron has no permission set (standard case). It could explain why Jonathan was not able to reproduce the error.
But from the moment you set whatever permission to the patron, you will get the error systematically. What is very strange is that the error persists even if you remove again the permission. I also try to empty the browser cache and cookies, restart starman service, close the browser and run again : seems that, when the patron has ever had any permissions in the past, he's condamned to cause the error (!!?).

Olivier Crouzet
Comment 5 Koha Team Lyon 3 2017-02-23 12:00:21 UTC
as a temporary workaround, I put this in the intranetuserjs syspref :

$('#mainform').submit(function(event){
    if ( ! $('#barcode').val().length ) {
        event.preventDefault();
        window.location.replace("/cgi-bin/koha/circ/circulation.pl");
    }
});

Olivier Crouzet
Comment 6 Jonathan Druart 2017-02-23 15:59:25 UTC
(In reply to Koha Team Lyon 3 from comment #4)
> Hello,
> We experiment the same problem on our preprod install (3.22).

Are you able to test against master?
Can you recreate in a sandbox?

> I'm not sure of this but probably It does not happen when the patron has no
> permission set (standard case). It could explain why Jonathan was not able
> to reproduce the error.

When the blank page is displayed there is no borrowernumber parameter passed to circulation.pl, so it should not depend on the patron's info.
Comment 7 Koha Team Lyon 3 2017-02-24 09:12:29 UTC
I tested against master (on a vbox) and at first sight, it seems OK. At least, no error, even if we set a permission to the patron.

But the issue is real on 3.22 (our preprod version is 3.22.12). A parameter is really passed to circulation.pl (probably in post method) and I can now assert that this parameter is the flags value (?!!). Don't ask me how it is but it is.
In the first comment from ClintD, the "1" string corresponds with no doubt to superlibrarian flags value.

To reproduce, you can try that. 
1) take a patron without any permissions and check that the error issue does not already occurs.
2) set a permission to him. Tick for instance the "reservesforothers" one (flags value = 64)
3) go back to checkout tab, put your cursor in the barcode input and press Enter :
you will get this :
Can't use string ("64") as a HASH ref while "strict refs"....
4) remove the permission
5) redo step 3. you will now get this :
Can't use string ("0") as a HASH ref while "strict refs"....
Comment 8 Jonathan Druart 2017-02-24 10:44:42 UTC
Confirmed and recreated on 16.11.x
Comment 9 Jonathan Druart 2017-02-24 11:02:06 UTC
Created attachment 60647 [details] [review]
Bug 18022: Fix possible crash if CircAutoPrintQuickSlip=clear screen

If CircAutoPrintQuickSlip is set to clear screen, the $borrowernumber variable
is reset but $borrower.
In 16.11.x and before that causes the app to crash, because
$borrower->{flags} is set to a hashref by GetMemberDetails, if
$borrowernumber is set.

This case is better handled in master (17.05) but side-effects could
happen: if $borrowernumber is reset, $borrower should be reset too.

The way to drive this behaviour is terrible and lot of things should be
done to clean this area. This patch is a quick and easy fix to make it
backportable easily.

Test plan:
On master, no change expected
On 16.11 and before:
0. Set CircAutoPrintQuickSlip=clear screen
1. Create a new patron
2. Set permission for this patron to 'reservesforothers' (or something
else).
3. Go on the circulation tab
4. Do not fill the barcode input and submit
=> Without this patch you will get an error
Can't use string ("64") as a HASH ref while "strict refs"....
=> With this patch apply you will get a blank screen (expected
behaviour).

I would recommend to test this patch with the other value of
CircAutoPrintQuickSlip as well
Comment 10 Koha Team Lyon 3 2017-02-27 13:24:55 UTC
Tested on 16.11.x
Works as expected :
with CircAutoPrintQuickSlip system preference to "clear the screen." :
the screen is cleared
with CircAutoPrintQuickSlip system preference to "open a print quick slip/slip window." :
a print window is opened.

Olivier Crouzet
Comment 11 Marcel de Rooy 2017-03-10 09:16:25 UTC
Can't say that terrible hacks look good, but at least we could move this patch to the queue of Katrin..
Comment 12 Katrin Fischer 2017-04-02 16:46:42 UTC
Please ping me on IRC about patches for 16.11.x - almost missed this! :)

This patch has been pushed to 16.11.x and will be in 16.11.07.
Comment 13 Julian Maurice 2017-04-21 09:32:57 UTC
Pushed to 3.22.x for 3.22.20
Comment 14 Mason James 2017-05-03 03:52:37 UTC
Pushed to 16.05.x, for 16.05.12 release