Summary: | NewBasket does not use placeholders in sql | ||
---|---|---|---|
Product: | Koha | Reporter: | Colin Campbell <colin.campbell> |
Component: | Acquisitions | Assignee: | Colin Campbell <colin.campbell> |
Status: | CLOSED FIXED | QA Contact: | Katrin Fischer <katrin.fischer> |
Severity: | major | ||
Priority: | P5 - low | CC: | chris, jonathan.druart |
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
Bug 9454: Use placeholders when adding basket Bug 9454: Use placeholders when adding basket |
Description
Colin Campbell
2013-01-23 11:17:51 UTC
Created attachment 14776 [details] [review] Proposed patch Patch should not change functional behaviour. To test check baskets still correctly created Created attachment 14924 [details] [review] Bug 9454: Use placeholders when adding basket Should always use placeholders when passing variables to DBI; avoids unforeseen bugs and security issues. Also: - reformated the long lists of parameters to add CR - moved the setting of defaults out of the call to ModBasket to clarify code - Setting parameters to undef if they were not defined was unnecessary bloat and obscuration Testing: Patch should not change functional behaviour. To test check that order baskets can still be correctly created. Signed-off-by: Galen Charlton <gmc@esilibrary.com> (In reply to comment #2) > - Setting parameters to undef if they were not defined > was unnecessary bloat and obscuration I disagree, it is not that the original code does. $t ||= undef and $t //= undef is not the same. The second one is unnecessary, not the first one. The original code says: if a variable ($basketcontractnumber, $deliveryplace or $billingplace) is equal to 0 or an empty string, we call ModBasketHeader with an undefined value. If someone calls the routine with "", we want to insert undef, not ""
> The original code says: if a variable ($basketcontractnumber, $deliveryplace
> or $billingplace) is equal to 0 or an empty string, we call ModBasketHeader
> with an undefined value.
>
> If someone calls the routine with "", we want to insert undef, not ""
The only call of the routine in basketheader.pl already forces basketcontractnumber to undef in these conditions and this would seem the appropriate place to validate inputs.
(In reply to comment #4) > > The original code says: if a variable ($basketcontractnumber, $deliveryplace > > or $billingplace) is equal to 0 or an empty string, we call ModBasketHeader > > with an undefined value. > > > > If someone calls the routine with "", we want to insert undef, not "" > > The only call of the routine in basketheader.pl already forces > basketcontractnumber to undef in these conditions and this would seem the > appropriate place to validate inputs. There are 2 places: - C4::Acq::NewBasket, where the validation is done. - In the basketheader.pl script, the validation is only done for the contractnumber value. I don't know if it is a big deal, but in the doubt, without modification of this patch I would prefer that someone else of the QA team gives his opinion on it. Looking at this now. Created attachment 15282 [details] [review] Bug 9454: Use placeholders when adding basket Should always use placeholders when passing variables to DBI; avoids unforeseen bugs and security issues. Also: - reformated the long lists of parameters to add CR - moved the setting of defaults out of the call to ModBasket to clarify code - Setting parameters to undef if they were not defined was unnecessary bloat and obscuration Testing: Patch should not change functional behaviour. To test check that order baskets can still be correctly created. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> All tests and QA script pass. Created a new basket, added order lines and closed basket. Checked everything worked in the staff interface, also checked the entry for the new basket in aqbasket in the database. Logs are clean too. This patch has been pushed to master. No string changes so pushed to 3.10.x will be in 3.10.3 |