Summary: | Invalid SQL being passed in circulation checkout | ||
---|---|---|---|
Product: | Koha | Reporter: | Colin Campbell <colin.campbell> |
Component: | Circulation | Assignee: | Colin Campbell <colin.campbell> |
Status: | CLOSED FIXED | QA Contact: | Bugs List <koha-bugs> |
Severity: | normal | ||
Priority: | PATCH-Sent (DO NOT USE) | CC: | chris, dev_patches, gmcharlt, koha.sekjal, paul.poulain |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Attachments: |
Proposed Patch
Signed-off patch proposed patch (passed QA) |
Description
Colin Campbell
2011-04-28 13:00:26 UTC
Created attachment 4014 [details] [review] Proposed Patch 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 Yes its executing bad syntax like WHERE ORDER BY and the second (failing) query is unnecessary Created attachment 4438 [details] [review] Signed-off patch Although I'm not qualified to evaluate how it does it, applying this patch quiets the errors I was seeing in the log from circulation.pl so I'm signing off. *** Bug 6568 has been marked as a duplicate of this bug. *** 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; to + if ($_->{date_due} and $_->{date_due} lt $today) { + $_->{overdue} = 1; + } (we must be Perlish, but sometimes that result in really cryptic things...) passed QA 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 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. Pushed, please test *** Bug 6805 has been marked as a duplicate of this bug. *** |