Bug 8429

Summary: Unnecessary use of Exporter in SIP/ILS objects
Product: Koha Reporter: Colin Campbell <colin.campbell>
Component: Architecture, internals, and plumbingAssignee: Galen Charlton <gmcharlt>
Status: CLOSED FIXED QA Contact:
Severity: minor    
Priority: P5 - low CC: chris, colin.campbell, jcamins, kyle, mtj, paul.poulain
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Proposed Patch
Bug 8429: Remove unnecessary use of Exporter from SIP/ILS
Patch rebased against current head
[SIGNED-OFF] Bug 8429: Remove unnecessary use of Exporter from SIP/ILS

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.