Bug 6280 - Invalid SQL being passed in circulation checkout
Summary: Invalid SQL being passed in circulation checkout
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: PATCH-Sent (DO NOT USE) normal (vote)
Assignee: Colin Campbell
QA Contact: Bugs List
: 6568 6805 (view as bug list)
Depends on:
Reported: 2011-04-28 13:00 UTC by Colin Campbell
Modified: 2012-10-25 23:09 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:

Proposed Patch (3.91 KB, patch)
2011-04-28 13:48 UTC, Colin Campbell
Details | Diff | Splinter Review
Signed-off patch (3.97 KB, patch)
2011-06-09 14:32 UTC, Owen Leonard
Details | Diff | Splinter Review
proposed patch (passed QA) (3.97 KB, patch)
2011-08-10 13:00 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2011-04-28 13:00:26 UTC
Bug 5952 changed the interface to C4::Members::GetPendingIssues however in circulation.pl it is now being called with an empty array in many cases so that a simple checkout is resulting in two calls the first (for the borrower succeeds) the second for the relatives fails to getting an error return from the database

This is not good
Comment 1 Colin Campbell 2011-04-28 13:48:46 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2011-06-09 13:42:04 UTC
Is an error this patch addresses?

circulation.pl: DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'ORDER BY issues.issuedate' at line 25 at /kohaclone/C4/Members.pm line 1070
Comment 3 Colin Campbell 2011-06-09 14:22:39 UTC
Yes its executing bad syntax like WHERE ORDER BY and the second (failing) query is unnecessary
Comment 4 Owen Leonard 2011-06-09 14:32:11 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2011-07-08 10:30:47 UTC
*** Bug 6568 has been marked as a duplicate of this bug. ***
Comment 6 Paul Poulain 2011-08-10 13:00:53 UTC
Created attachment 4935 [details] [review]
proposed patch (passed QA)

QA comment

* works as announced, usefull patch, logs will thank you forever ;-)
* Just a minor comment/suggestion. You write:
+if ( @borrowernumbers ) {
+    $relissue    = GetPendingIssues(@borrowernumbers);
It's correct although it's more perlish to have :
$relissue    = GetPendingIssues(@borrowernumbers) if @borrowernumbers;

I agree with the change 
-        ($_->{date_due} lt $today) and $_->{overdue} = 1;
+        if ($_->{date_due}  and $_->{date_due} lt $today) {
+            $_->{overdue} = 1;
+        }
(we must be Perlish, but sometimes that result in really cryptic things...)

passed QA
Comment 7 Colin Campbell 2011-08-10 14:44:04 UTC
Paul it is considered bad practice in many perl circles these days to use postfix if except for flow of control statements e.g. " LAST if $x;". They cause problems because people often miss them when rereading code. The default policies in perlcritic will warn against them see Perl Best Practices for a rationale
Comment 8 Chris Cormack 2011-08-10 18:59:29 UTC
I agree with Colin, we should follow Perl best practices where ever we can.

And if you look at bug 6679, you will see we are extending the Perl::Critic tests slowly to cover everything.
Comment 9 Chris Cormack 2011-08-11 00:56:46 UTC
Pushed, please test
Comment 10 Ian Walls 2011-09-30 18:42:40 UTC
*** Bug 6805 has been marked as a duplicate of this bug. ***