[Bug 10643] New: Inappropriate uses of $sth->finish() in C4::ClassSource.pm
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10643 Bug ID: 10643 Summary: Inappropriate uses of $sth->finish() in C4::ClassSource.pm Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: kenza.zaki@biblibre.com QA Contact: testopia@bugs.koha-community.org -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10643 kenza <kenza.zaki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |kenza.zaki@biblibre.com Assignee|gmcharlt@gmail.com |kenza.zaki@biblibre.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10643 kenza <kenza.zaki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10643 --- Comment #1 from kenza <kenza.zaki@biblibre.com> --- Created attachment 19948 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19948&action=edit Bug 10643: Inappropriate uses of $sth->finish() in C4::ClassSource.pm
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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10643 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |srdjan@catalyst.net.nz --- Comment #2 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10643 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |gmcharlt@gmail.com --- Comment #3 from Galen Charlton <gmcharlt@gmail.com> --- (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(). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10643 kenza <kenza.zaki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19948|0 |1 is obsolete| | --- Comment #4 from kenza <kenza.zaki@biblibre.com> --- Created attachment 20105 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20105&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10643 --- Comment #5 from Owen Leonard <oleonard@myacpl.org> --- 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10643 --- Comment #6 from kenza <kenza.zaki@biblibre.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10643 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10643 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20105|0 |1 is obsolete| | --- Comment #7 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 20120 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20120&action=edit [SIGNED-OFF] 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10643 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10643 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20120|0 |1 is obsolete| | --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 20236 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20236&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10643 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #9 from Galen Charlton <gmcharlt@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10643 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |tomascohen@gmail.com Version|unspecified |master --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- This patch has been pushed to 3.12.x, will be in 3.12.4. Thanks Kenza! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10643 Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bgkriegel@gmail.com --- Comment #11 from Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> --- Pushed to 3.10.x, will be in 3.10.11 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10643 Chris Hall <chrish@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |chrish@catalyst.net.nz --- Comment #12 from Chris Hall <chrish@catalyst.net.nz> --- Pushed to 3.8.x, will be in 3.8.18 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon@bugs.koha-community.org