Bug 10643 - Inappropriate uses of $sth->finish() in C4::ClassSource.pm
Summary: Inappropriate uses of $sth->finish() in C4::ClassSource.pm
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: kenza
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-25 11:54 UTC by kenza
Modified: 2014-12-07 20:02 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10643: Inappropriate uses of $sth->finish() in C4::ClassSource.pm (3.48 KB, patch)
2013-07-25 12:06 UTC, kenza
Details | Diff | Splinter Review
Bug 10643: Inappropriate uses of $sth->finish() in C4::ClassSource.pm (5.93 KB, patch)
2013-08-05 07:59 UTC, kenza
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10643: Inappropriate uses of $sth->finish() in C4::ClassSource.pm (5.98 KB, patch)
2013-08-06 01:12 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 10643: Inappropriate uses of $sth->finish() in C4::ClassSource.pm (6.03 KB, patch)
2013-08-09 15:17 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description kenza 2013-07-25 11:54:58 UTC

    
Comment 1 kenza 2013-07-25 12:06:55 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2013-07-30 04:56:56 UTC
I propose something even better then - get rid of prepare() all together. For example:

* GetClassSources():
return $dbh->selectall_hashref("SELECT...", undef, 'cn_source');

* AddClassSource()
return $dbh->do("INSERT...", undef, @params)

etc. How often are those used to get advantage of prepare_cached()?
Good databases (tm) cache queries anyway, however not sure that MySQL does.
Comment 3 Galen Charlton 2013-08-02 14:30:51 UTC
(In reply to Srdjan Jankovic from comment #2)
> I propose something even better then - get rid of prepare() all together.
> For example:
> 
> * GetClassSources():
> return $dbh->selectall_hashref("SELECT...", undef, 'cn_source');
> 
> * AddClassSource()
> return $dbh->do("INSERT...", undef, @params)
> 
> etc. How often are those used to get advantage of prepare_cached()?
> Good databases (tm) cache queries anyway, however not sure that MySQL does.

Getting rid of $sth->finish() and replacing prepare_cached() with prepare() suffices as a minimal change, but doesn't preclude going further and using selectall_hashref.

As far as the current patch is concerned, I do think it would be useful to at least get rid of prepare_cached().
Comment 4 kenza 2013-08-05 07:59:08 UTC Comment hidden (obsolete)
Comment 5 Owen Leonard 2013-08-05 14:06:25 UTC
None of these "$sth->finish()" patches have a test plan. I assume there's no way to detect an improvement, but how can we confirm that this change hasn't broken anything?
Comment 6 kenza 2013-08-05 14:34:15 UTC
(In reply to Owen Leonard from comment #5)
> None of these "$sth->finish()" patches have a test plan. I assume there's no
> way to detect an improvement, but how can we confirm that this change hasn't
> broken anything?

For this patch , the t/db_dependent/ClassSource.t exists. So we can be sure that nothing is broken by doing 'prove t/db_dependent/ClassSource.t' after applying this patch:

t/db_dependent/ClassSource.t .. 1/24 attempting to use non-existent class sorting routine routine1
t/db_dependent/ClassSource.t .. ok     
All tests successful.
Files=1, Tests=24,  0 wallclock secs ( 0.02 usr  0.00 sys +  0.06 cusr  0.01 csys =  0.09 CPU)
Result: PASS
Comment 7 Srdjan Jankovic 2013-08-06 01:12:20 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2013-08-09 15:17:38 UTC
Created attachment 20236 [details] [review]
Bug 10643: Inappropriate uses of $sth->finish() in C4::ClassSource.pm

This patch gets rid of finish() and replace prepare_cached by prepare.

From the man page

finish()
Indicate that no more data will be fetched from this statement handle
before it is either executed again or destroyed.
You almost certainly do not need to call this method.

Adding calls to "finish" after loop that fetches all rows is a common
mistake, don't do it, it can mask genuine problems like uncaught fetch errors.

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Galen Charlton 2013-08-09 15:34:50 UTC
Pushed to master.  Thanks, Kenza!

I notice, however, that the author line is wrong:

root <root@kenza-VirtualBox>

Please use git config to set its user.name and user.email attributes to your correct name and email address.
Comment 10 Tomás Cohen Arazi 2013-08-16 16:00:40 UTC
This patch has been pushed to 3.12.x, will be in 3.12.4.

Thanks Kenza!
Comment 11 Bernardo Gonzalez Kriegel 2013-09-13 22:53:53 UTC
Pushed to 3.10.x, will be in 3.10.11
Comment 12 Chris Hall 2013-09-17 07:56:35 UTC
Pushed to 3.8.x, will be in 3.8.18