[Koha-devel] Restructuring C4 (work continued)
Paul Poulain
paul.poulain at biblibre.com
Wed Sep 14 23:27:16 CEST 2011
Hi,
I've continued my work in the train, here is a result of my work.
I've checked a lot of the packages in C4 and checked for dependancies
(sub packages used)
With the check_subs.pl script, it was quite easy to find which subs were
called and see if it's needed.
What I propose next is to file a bug for each package with my findings
and my suggestions. Then, write a patch.
I also plan to investigate some of the most used pages (like
circ/circulation.pl, circ/return.pl, detail.pl, and most opac pages.
Note that the check_subs can be also use to check package dependancies
for perl scripts as well: on line 39, set "dirToCheck" to the directory
you want to check. I've checked circ and opac, and have already
discovered some interesting informations.
If you have some comment, want to make another suggestion or give a
hand, you're welcomed ;-)
mainpage.pl loads the following .pm
==== C4::Auth.pm ====
== use Members.pm ==
only to call GetMemberDetails and fill USER_INFO parameters
USER_INFOS are used in a large variety of place.
* In a strange way, like in : acqui/acqui-home.pl:
$template->{VARS}->{'USER_INFO'}[0]->{'borrowernumber'} );
Q: don't we have another way to retrieve loggedin librarian number ?
* in opac-reserve, to display who we place a request for. Again,
this could/should be moved to opac-reserve, and we use GetMemberDetails
in this sub
CONCLUSION = we should be able to get rid of use Members.pm in Auth.pm
== use C4::Context.pm ==
package that can't be avoided and loads nothing specific
== use C4::Output.pm ==
= use C4::Budgets =
just to call Budget::GetCurrency in the Output::FormatNumber sub.
The Output::FormatNumber sub is used only in a few places, related to
C4::Budgets
CONCLUTION => move FormatNumber outside from C4::Output into
C4::Budgets and we will get rid of this dependancy
= use C4::Languages =
to call a lot of sub here, no possibility to do something
= use C4::Dates =
to call format_date, in Date::FormatDate in C4::Output::FormatData
sub that is called only in aqbudgetperiod.pl and should easily be
removed. it contains only
$$data_hashref{$_} = format_date( $$data_hashref{$_} ) for
grep{/date/} keys (%$data_hashref);
that can be inlined in aqbudgetperiod (and it would be better to use
the OOo date tools)
= use C4::Templates =
needed, it's the package that smoothen the transition between
H::T::P and T::T
== use C4::Branch ==
= use C4::Koha = just to call get_infos_of once, we could get rid
with this sub by inlining and remove the C4::Koha dependancy
== use C4::VirtualShelves ==
* this package is needed in case of an OPAC query (on every opac
page we display the public & private lists). I suspect it's not needed
in staff interface. Anyone to confirm/argue ?
* We use only GetRecentShelves sub,
* we put everything in the session (set_shelves_userenv) & context
($session->param('barshelves', $barshelves);) I think there is no use
for this. Anyone to confirm/argue ?
* also note that the following line
my ($total, $pubshelves) =
C4::Context->get_shelves_userenv(); # an anonymous user has no
'barshelves'...
(C4::Auth, line 301 and the following) seems strange.
= use C4::Circulation =
and that's the beginning of circular-painfull-loading-everything.
In C4::VirtualShelves, there is no use of anything from
C4::Circulation. I've commented the line, made a lot of tests, and saw
no problem at all.
==== C4::Koha ====
This loading is useless. I've commented the line and saw no problem.
==== C4::AuthoritiesMarc; ====
This part is useless here, we do nothing with the authorities retrieved.
==== C4::NewsChannels ====
== use C4::Context ==
nothing to say, needed to access the database
== use C4::Dates ==
nothing to say, needed to format the date the news has been published.
<<<< END OF mainpage specifics >>>>
Continued to check some of the major .pm
==== Member.pm ====
Member.pm, (that is loaded by Auth.pm at the moment) load the following
packages:
== use Reserves.pm ==
Reserves.pm is used only to call
my @itemswaiting =
C4::Reserves::GetReservesFromBorrowernumber(
$patroninformation->{'borrowernumber'},'W' );
in C4::Members::patronflags sub
I'm not sure we do anything with the result of
GetReservesFromBorrowernumber:
C4::Members, line 518 contains :
my @itemswaiting =
C4::Reserves::GetReservesFromBorrowernumber(
$patroninformation->{'borrowernumber'},'W' );
my $nowaiting = scalar @itemswaiting;
if ( $nowaiting > 0 ) {
my %flaginfo;
$flaginfo{'message'} = "Reserved items available";
$flaginfo{'itemlist'} = \@itemswaiting;
$flags{'WAITING'} = \%flaginfo;
}
return ( \%flags );
I've some doubts about the need for this code, i've commented
it, placed a hold, make it waiting for pickup, I see everything as
usual, I saw no difference. It will be worth a bug entry and a patch to
test soon !
I've moved the use C4::Members to a require C4::Member just
before the GetReservesFromBorrowernumber call
NYTProf before commenting :
Profile of mainpage.pl for 1.24s (of 1.54s), executing
102597 statements and 25016 subroutine calls in 212 source files and 55
string evals.
NYTProf after commenting :
Profile of mainpage.pl for 1.25s (of 1.55s), executing
102489 statements and 24984 subroutine calls in 212 source files and 55
string evals.
(the duration is strange. But I was with my laptop on the
battery, so that may explain. The number of statements&subroutines are
lowered though)
== use C4::Overdues.pm ==
this is loaded only to call checkoverdues in
C4::Members::GetMemberDetails
The result is stored in the big member detail hash returned :
my ( $odues, $itemsoverdue ) =
checkoverdues($patroninformation->{'borrowernumber'});
if ( $odues && $odues > 0 ) {
my %flaginfo;
$flaginfo{'message'} = "Yes";
$flaginfo{'itemlist'} = $itemsoverdue;
foreach ( sort { $a->{'date_due'} cmp $b->{'date_due'} }
@$itemsoverdue )
{
$flaginfo{'itemlisttext'} .=
"$_->{'date_due'} $_->{'barcode'} $_->{'title'}
\n"; # newline is display layer
}
$flags{'ODUES'} = \%flaginfo;
It's used only in circulation.pl, return.pl and SIP.pm
My opinion is that we could have a dedicated sub for that in
Circulation.pm (that is always loaded in circulation.pl & return.pl),
retrieve the result from here when needed and reduce the size of
GetMemberDetails (that would not return everything)
== use C4::Biblio.pm ==
this is loaded just to call C4::Biblio::GetBiblioFromItemNumber in
C4::Members::GetMemberAccountRecords to retrieve title and author
This can be easily circumvented with a correct LEFT join a few
lines before.
Instead of
SELECT *
FROM accountlines
WHERE borrowernumber
write :
SELECT accountlines.*,biblio.title,biblio.author FROM accountlines
LEFT JOIN items USING(itemnumber) LEFT JOIN biblio USING(biblionumber)
WHERE borrowernumber
The use C4::Biblio can be removed !
== use C4::Account.pm ==
this is loaded to call manualinvoice twice.
the manualinvoice is used only when adding or renewing a patron.
So it should be efficient to move from a use C4::Account to a require
C4::Account where applicable
==== Accounts.pm ====
== use C4::Stats ==
this package contains only one sub (updatestats) and no dependancies
== use C4::Members ==
loaded just to call C4::Members::GetMember once in the
C4::Accounts::returnlost sub.
This sub seems totally unused in Koha (dead code)
grep -R "returnlost" * shows only the sub header itself. Could be
removed.
== use C4::Items ==
loaded to call ModItem twice.
The 1st call is in C4::Accounts::returnlost, that can be removed
(see below)
The 2nd call is in C4::Accounts::chargelostitem, that is rarely
used. Switching to require C4::Items should be possible
== use C4::Circulation ==
This loading is similar to C4::Items with
C4::Circulation::MarkIssueReturned sub :
The 1st call is in C4::Accounts::returnlost, that can be removed
(see below)
The 2nd call is in C4::Accounts::chargelostitem, that is rarely
used. Switching to require C4::Circulation should be possible
==== Acquisition.pm ====
There is a use Debug.pm that is useless in Acquisition.pm
There is a use Biblio.pm that is useless in Acquisition.pm
use C4::Debug is written twice !
== use C4::Suggestions ==
the ModSuggestion and GetSuggestionFromBiblionumber are called
when an order is recieved. Could switch to require, both statements are
close
==== Biblio.pm ====
== use C4::Serials ==
Serials.pm is loaded to delete subscriptions in
C4::Biblio::DelBiblio. 2 subs are used :
C4::Serials::GetFullSubscriptionsFromBiblionumber and
C4::Serials::DelSubscription
both can be moved to require at the right place
== use C4::Items ==
* the check_subs script wrongly report a use of ModItem and
DelItem, it's a comment in the code :
# duplication or incorrect data - use AddItem() or ModItem()
* C4::Biblio uses C4::Items::GetMarcItem twice
* once in PrepareItemrecordDisplay => this sub sounds at a wrong
place. It could/should be moved to C4::Items itself.
PrepareItemrecordDisplay is used by:
* acqui/addorderiso2709.pl = already loads C4::Items
* acqui/neworderempty.pl = don't load C4::Items for instance
* acqui/orderreceive.pl = already loads C4::Items
* C4/Serials.pm = need more investigation
* serials/serials-edit.pl = already loads C4::Items
* once in C4::Biblio::EmbedItemsInMarcBiblio
Could we remove this sub ? question still pending. it's used
only in:
* C4::Biblio::GetMarcBiblio
* misc/migration_tools/rebuild_zebra.pl
* tools/export.pl
==== Items.pm ====
== use C4::Biblio ==
Too many things are required, we can't change. The best solution
would be to remove C4::Items reference in C4::Biblio
GetOrderFromItemnumber
== use C4::Reserves ==
This package is loaded just for C4::Reserves::CheckReserves called
in C4::Items::GetItemsInfo
The GetItemsInfo stores the result of CheckReserves in a hash entry,
count_reserve, that is used only in opac_detail to display the status of
a hold. We could remove the reserve_count hash entry and inline
C4::Reserves::CheckReserves directly from opac-detail.pl page
in opac-detail.pl, instead of
if( $itm->{'count_reserves'} eq "Waiting"){ $itm->{'waiting'} = 1; }
write :
if ( C4::Reserves::CheckReserves(<<parameters>>) eq "Waiting"){
$itm->{'waiting'} = 1; }
== use C4::Acquisition ==
This package is loaded to call
* C4::Acquisition::ModOrder = recent addition, when you move an item
to another biblio, you move the order if it's relevant. This sub is
rarely called and could be switched to require
* C4::Acquisition::GetOrderFromItemnumber = same comment, used a few
lines before ModOrder
==== Reserves.pm ====
Reserves.pm is really crapy and should probably be rewritten entirely.
A grep -R "use C4::Reserves;" *|wc -l says that 33 scripts are using
this package.
There is a use Search.pm that is useless in Reserves.pm
== use C4::Biblio ==
loaded to call GetBiblioItemData and GetBiblioData:
* GetBiblioData is called in AddReserve, to prepare and send a
mail depending on the syspref emailLibrarianWhenHoldIsPlaced
* GetBiblioItemData is used twice in the same sub
C4::Reserves::GetReservesFromBiblionumber. Those informations are
retrieved just to get title,author,... and could be added into the
previous SQL query :
instead of:
SELECT biblioitemnumber
FROM reserveconstraints
WHERE biblionumber = ?
AND borrowernumber = ?
AND reservedate = ?
write:
SELECT reserveconstraints.biblionumber,biblio.*,biblioitems.* FROM
reserveconstraints
LEFT JOIN biblio USING(biblionumber) LEFT JOIN biblioitems
USING(biblionumber)
WHERE biblionumber = ? AND borrowernumber = ? AND
reservedate = ?
That should do the job, and the dependancy could be removed.
Note: C4::Reserves::GetReservesFromBiblionumber is used in various
places, for example opac-reserve.pl or in catalogue/detail.pl, but it
seems we don't use all of the result computed in the sub. So maybe we
can just clean GetReservesFromBiblionumber. It must be checked better
== use C4::Members ==
loaded to call:
* C4::Members::GetMember is called in
* C4::Reserves::AddReserve, to prepare and send a mail
depending on the syspref emailLibrarianWhenHoldIsPlaced
* C4::Reserves::CanItemBeReserved. This call is just to
retrieve patron category and branch. The CanItemBeReserved is called
only in request & opac-reserve & ILSDI pages. an option would be to send
categorycode and branchcode from here (and change the API of the sub)
* C4::Reserves::_koha_notify_reserve, to retrieve the Email
(GetMember is always called, but the email is then retrieved from it or
from GetFirstValidEmailAddress (see below, it's a dirty code...)
* C4::Members::GetFirstValidEmailAddress
GetFirstValidEmailAddress is called to send emails to patron that places
a reserve, I don't see how to remove this dependancy...
* C4::Members::GetMemberDetails is called in
C4::Reserves::CheckReserves This sub is probably the crappiest in Koha.
It mixes many things, it should be entirely rewritten
Conclusion for C4::Reserves: I think we should try to isolate as much as
possible this package.
--
Paul POULAIN
http://www.biblibre.com
Expert en Logiciels Libres pour l'info-doc
Tel : (33) 4 91 81 35 08
More information about the Koha-devel
mailing list