Bug 8429 - Unnecessary use of Exporter in SIP/ILS objects
Summary: Unnecessary use of Exporter in SIP/ILS objects
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Galen Charlton
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-12 14:08 UTC by Colin Campbell
Modified: 2013-05-23 06:23 UTC (History)
6 users (show)

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


Attachments
Proposed Patch (8.28 KB, patch)
2012-07-12 14:13 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 8429: Remove unnecessary use of Exporter from SIP/ILS (8.34 KB, patch)
2012-08-19 08:04 UTC, Chris Cormack
Details | Diff | Splinter Review
Patch rebased against current head (8.26 KB, patch)
2012-09-19 09:46 UTC, Colin Campbell
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8429: Remove unnecessary use of Exporter from SIP/ILS (8.36 KB, patch)
2012-11-09 16:32 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 Colin Campbell 2012-07-12 14:08:18 UTC
Confusedly an unnecessary dependence on Exporter has been added to some modules in SIP/ILS hierarchy. It should be excised and the code be made more consistent to enhance maintainability
Comment 1 Colin Campbell 2012-07-12 14:13:38 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2012-08-19 08:04:48 UTC Comment hidden (obsolete)
Comment 3 Mason James 2012-09-16 13:55:10 UTC
> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

nice patch!, passing QA…

$ koha-qa.pl  

testing 1 commit(s) (applied to commit 2a60d49)
 * 3e771fc Bug 8429: Remove unnecessary use of Exporter from SIP/ILS
      C4/SIP/ILS/Item.pm
      C4/SIP/ILS/Patron.pm
      C4/SIP/ILS/Transaction/Checkin.pm
      C4/SIP/ILS/Transaction/Checkout.pm
      C4/SIP/ILS/Transaction/FeePayment.pm
      C4/SIP/ILS/Transaction/Hold.pm
      C4/SIP/ILS/Transaction/Renew.pm
      C4/SIP/ILS/Transaction/RenewAll.pm

* C4/SIP/ILS/Item.pm                                                       OK
* C4/SIP/ILS/Patron.pm                                                     OK
* C4/SIP/ILS/Transaction/Checkin.pm                                        OK
* C4/SIP/ILS/Transaction/Checkout.pm                                       OK
* C4/SIP/ILS/Transaction/FeePayment.pm                                     OK
* C4/SIP/ILS/Transaction/Hold.pm                                           OK
* C4/SIP/ILS/Transaction/Renew.pm                                          OK
* C4/SIP/ILS/Transaction/RenewAll.pm                                       OK
Comment 4 Paul Poulain 2012-09-18 13:19:17 UTC
Sorry colin, but I just pushed bug 8336 (from you), and this one does not apply anymore. Should not be hard to fix:
CONFLICT (content): Merge conflict in C4/SIP/ILS/Transaction/RenewAll.pm
Auto-merging C4/SIP/ILS/Transaction/Renew.pm
CONFLICT (content): Merge conflict in C4/SIP/ILS/Transaction/Renew.pm
Failed to merge in the changes.
Comment 5 Colin Campbell 2012-09-19 09:46:17 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2012-11-09 16:32:52 UTC
Created attachment 13360 [details] [review]
[SIGNED-OFF] Bug 8429: Remove unnecessary use of Exporter from SIP/ILS

All the modules in the SIP/ILS tree are objects
The addition of calls to Exporter or hand manipulation of
@ISA added unnecessary bloat
Removed the "self = shift or return" idiom  as it is nonsensical
if the method can only be called via an object.
standardized inheritance via use parent
added a $self = shift in a couple of places where it
was not strictly necessary as its absence seemed to have
misled readers in the past

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall 2012-11-09 16:36:58 UTC
Signed off with a caveat. The 'parent' pragma was not part of the perl core until version 5.10.1. Do we require this version of perl? What about installations still stuck on Lenny? I don't know about other distributions perl versions.
Comment 8 Colin Campbell 2012-11-09 18:14:19 UTC
(In reply to comment #7)
> Signed off with a caveat. The 'parent' pragma was not part of the perl core
> until version 5.10.1. Do we require this version of perl? What about
> installations still stuck on Lenny? I don't know about other distributions
> perl versions.

We upped the required version to 5.10 a few releases ago in a couple of places we have a use 5.10 to explicitly use 5.10 features (even 5.10 is no longer really a supported perl release!)
Comment 9 Paul Poulain 2012-11-29 17:24:23 UTC
QA comment: patch passed QA already (see comment 3), also had a signoff from chris (from before the rebase)

setting passed QA in the name of Mason

PS: is it really an ENH ? I think it's more a minor bugfix, so changing severity accordingly
Comment 10 Jared Camins-Esakov 2012-11-30 22:52:08 UTC
This patch has been pushed to master. I do not have a self-check machine so I was unable to test this thoroughly, however, the changes look good and SIP *seems* to still work. Further tests prior to the 3.12 release would be greatly appreciated.