Created attachment 5442 [details] script to check which package used in a package is useless C4 packages are using use C4::another package a lot. It result in all packages being loaded (& compiled) on each page. This bugzilla entry will contain the work to remove as much as possible the dependancies That will be achieved through : * removing use C4::otherpackage and using require C4::otherpackage in the subs that need C4::otherpackage. Require is loaded at run time only. Most of the time, the C4::otherpackage is loaded everytime, just to be used once, in one sub (that is rarely used) * removing useless subs that use a C4::otherpackage::sub, because there are some Cleaning for each package will result in a patch, with a lot of comments/details/feedback. To discover which sub is used by which script, I'll use the attached script.
Created attachment 5443 [details] log of the 1st script run before doing anythin
Moving compilation of the module code from compile to runtime seems a bit counter intuitive. Certainly if we start running the code in any kind of persistent framework we want to do the opposite. One thing that aids maintenenance is to only pollute (I mean import into ) your namespace those subroutines that you need. i.e, use Module qw( function1 function2); It also flags up to those who come after you how much of that module is used.
Created attachment 5496 [details] [review] Bug 6875 de-nesting Auth.pm * removed use C4::Koha that is useless * moved "use C4::Members" to "require C4::Members" just before GetMemberDetails call. This will avoid loading C4::Member everytime a page is called by someone not logged * still to do = work on C4::VirtualShelves, that can be optimized, definetly !
Created attachment 5497 [details] [review] Bug 6875 de-nesting C4/Branch.pm use C4::Koha was loaded only to call C4::Koha::get_infos_of in get_branchinfos_of the sub get_branchinfos_of was used nowhere (checked with "grep -R get_branchinfos_of *") only one reference was made to get_branchinfos_of, in reserve/request.pl, but the sub was not used in fact (maybe removed previously & silently) This patch removes sub get_branchinfos_of and use C4::Koha in C4::Branch, as well as the reference in request.pl and the t test The patch don't remove use C4::Branch in reserve/request.pl, it must be checked that it's useless first
Created attachment 5498 [details] [review] Bug 6875 de-nesting C4::Koha.pm * removed use C4::Output, that was not used * moved use URI::split to getitemtypeimagelocation and switched to "require". Checked it still works through admin/itemtype.pl * moved use Business::ISBN to _isbn_cleanup.pl and switched to "require". Checked it still works with the following small script : #!/usr/bin/perl use C4::Koha; print GetNormalizedISBN("1-56592-257-3"); (GetNormalizedISBN uses _isbn_cleanup)
Created attachment 5499 [details] [review] Bug 6875 de-nesting C4/Serials * use C4::Branch is loaded only to call GetBranchNAme in PrepareSerialsData and set branchname in the hashresult. This sub PrepareSerialsData is used in the following scripts : opac/opac-serial-issues.pl: my $subscriptioninformation=PrepareSerialsData($subscriptions); => no use of branchname after serials/serials-collection.pl: $subscriptions=PrepareSerialsData(\@subscriptioninformation); => no use of branchname after => we can remove the ->{branchname} from the result, and remove the C4::Branch dependancy * moves use C4::Items to require C4::Items, to call AddItemFromMarc, when receiving a serial, with Recieving create an item set in the subscription. * removed use C4::Letters and C4::Search, that are useless
Created attachment 5500 [details] [review] Bug 6875 de-nesting C4::ClassSort & ClassSortRoutine just removing use C4::Koha, that are useless
Created attachment 5501 [details] [review] [NOT_APPLY][PATCH 01]Bug 6875 de nesting C4::Biblio C4::Biblio is used in many many places. The goal of this cleaning is to do from C4::Biblio a package with as many dependancies as possible. * C4::Heading is called only in 1 place, highly rarely used (only in 1 misc/link_bibs_to_authorities.pl), moving to require * PrepareItemrecordDisplay is a sub that is more related to Items, moving it here. It means some scripts that used this sub must be checked against use C4::Items * C4::Items is needed in EmbedItemsInMarcBiblio, moving it only in this sub, and switching to require * 2 subs are totally useless z3950_extended_services and set_service_options, removing them
Created attachment 5502 [details] [review] Bug 6875 de-nesting C4::Heading C4::Search is needed only in authorities sub, moving it here and switching to require
Created attachment 5503 [details] [review] [NOT_APPLY][PATCH 02]Bug 6875 de-nesting C4::Items C4::Branch is used only in CheckItemPresave, moving from a use to a require in the sub C4::Reserve: 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; } C4::Acquisition is used only in MoveItemFromBiblio, a sub that is rarely called. Moving from a use to a require in the sub C4::Charset is used only in _parse_unlinked_item_subfields_from_xml. Moving from a use to require in the sub
Created attachment 5504 [details] [review] Bug 6875 de-nesting C4::VirtualShelves * C4::Circulation is unused, removing it * C4::Members used only in ShelfPossibleAction, switching from use to require * C4::Auth used only in RefreshShelvesSummary, a sub called only in opac-addbybiblionumber.pl script, moving the require inside the sub
Created attachment 5505 [details] [review] [FAILED_QA][PATCH 03]bug 6875 de-nesting C4::Member * working on GetMemberDetails. This sub must be optimized, as it is called on each page by C4::Auth. The sub call everytime patronflags, that need to load Reserves and Overdues. The patronflag returns a hash with various flags (overdues, fines, ...). It's not related to permissionflags, that are loaded elsewhere. Most of the time, we do nothing with those flags. The strategy is to split the GetMemberDetails in 2 subs : * GetMemberDetails, same as previously, without the flags * GetMemberFlags, that will return just the flags when needed (and will load when needed Reserves package) The following scripts call GetMemberDetails : * admin/aqbudget.pl = no use is done with flags, the splitting changes nothing * C4/SIP/ILS/Patron.pm = the flags are used later, so we call GetMemberFlags as well * C4/SIP/interactive_members_dump.pl = nothing is done with the flags, we change nothing * C4/Print.pm = used by printslip, that don't do anything with flags, we change nothing * C4/VirtualShelves = used to check permission to access to a virtualshelf, nothing done with flags, we change nothing * C4/Reserves = used in CheckReserves. With the result, we call _GetCircControlBranch only, that does nothing with flags, we change nothing * C4/Auth = used in get_template_and_user. The return from GetMemberDetails is stored in the session, I don't see any change, flags are useless * C4/Circulation.pm = used many times * in CanBookBeIssued, just to retrieve the name/cardnumber/borrowernumber of a book already issued to someone else * in CanBookBeIssued, just to retrieve the name/cardnumber/borrowernumber of a book already reserved by someone else * in AddReturn, the result of GetMemberDetails is returned. It's then used in return.pl, to display patron flags. Retrieving flags with GetMemberFlags in return.pl * in AddRenewal, to retrieve some information, nothing done with flags, we change nothing * in GetRenewCount, to retrieve name & categorycode, nothing done with flags, we change nothing * in ILSDI/Services.pm, GetMemberDetails is called many times. Only once we check for charges, that is in the flags, so retrieving flags with GetMemberFlags * in catalogue/detail.pl, GetMemberDetails is called to retrieve the name of a patron that has placed a hold on a book, we change nothing * in circ/transferstoreceive, GetMemberDetails is called to retrieve the name of a patron, (and address), we change nothing * in circ/circulation.pl, we must retrieve the flags to display them (like for return.pl) * in circ/waitingreserves.pl, GetMemberDetails is called to retrieve the name of the patron, nothing to change * in members/moremember.pl, GetMemberDetails is called to retrieve name, surname, borrowernumber, we change nothing * in members/deletemem.pl, GetMemberDetails is followed by a $flag = $bor->{flags}, replacing by a call to GetMemberDetails * in members/messaging.pl, GetMemberDetails is called to retrieve name, category, nothing to change * members/member-flags.pl is here for permissions, nothing to deal with patron flags, nothing to change * opac/opac-passwd.pl, nothing related to flags, nothing to change * opac/opac-ics.pl, nothing related to flags, nothing to change * opac/opac-renew.pl, used to retrieve only branchcode, nothing to change * opac/opac-account.pl, nothing to change, the core part of this script is to display fines that are retrieved by GetMemberAccountRecords * opac/opac-reserve.pl, nothing related to flags, nothing to change * opac/opac-userdetail.pl, script that retrieve only a few dates and other infos from GetMemberDetails, nothing to change * opac/opac-showreviews.pl, retrieve only name & email from GetMemberDetails, nothing to change * opac/SCO/sco-main.pl, calls canbookbeissued, that need flags, so retrieving them * opac/opac-readingrecord.pl, use GetMemberDetails to retrieve patron name, address... nothing to change * opac/opac-messaging.pl, use GetMemberDetails to retrieve sms informations, nothing to change * opac/opac-userupdate.pl, use GetMemberDetails to display member information that can be updated by the patron, nothing related to flags, nothing to change * opac/opac-privacy.pl, loaded just to retrieve name & privacy flag info, nothing to change * opac/opac-user.pl, use GetMemberDetails to display patron information, but not flags one, nothing to change * opac/opac-suggestion.pl use GetMemberDetails to retrieve the branchcode, nothing to change * reserve/request.pl use GetMemberDetails to retrieve many informations displayed (name, branchcode,...), but none related to flags. Nothing to change GetMemberDetails also launches 2 SQL queries, the 2nd being superfluous : the enrolmentperiod is already available in the 1st query, just return it ! => C4::Reserves & C4::Overdues are now used only in GetMemberFlags sub, that is not always called, so switching from a use to a require inside the sub C4::Accounts is used only in AddMember and ExtendMemberSubscriptionTo, and only if there is a fee for subscription, switching from use to require sub GetMemberAccountRecords the SQL query has been completed. With a LEFT JOIN, we retrieve directly the title & biblionumber. Thus the call to GetBiblioByItemNumber is now useless and the use C4::Biblio can be removed
Created attachment 5506 [details] [review] Bug 6875 de-nesting C4::Circulation Just removing use C4::Koha, as other packages are widely used.
Created attachment 5507 [details] [review] Bug 6875 de-nesting C4/Print.pm removing use C4::Circulation, that is useless
Created attachment 5508 [details] [review] Bug 6875 de-nesting C4::Reserve, 1st step C4::Search is not used, so removing it from C4::Reserves
Created attachment 5509 [details] [review] Bug 6875 cleaning mainpage.pl AutoritiesMarc is loaded for nothing : we don't display authorities anywhere, so removing the dependancy as well as the useless code
Created attachment 5510 [details] [review] Bug 6875 cleaning opac-main.pl C4::VirtualShelves, C4::Branch and C4::Acquisition are useless, removing their loading
Created attachment 5511 [details] [review] [NOT_APPLY][PATCH 04]Bug 6875 cleaning opac-detail.pl C4::Tags and C4::Reviews used only if the feature is activated. Changing the use to a require inside the if (after testing it's activated) For libraries that have activated both, that means no change, but for others, it's an improvement
Created attachment 5512 [details] [review] Bug 6875 C4::Context cleaning Checking NYTProf in C4::Context, it appears that the /i flag is highly time-consumming As we don't support anything outside from mysql, returning directly mysql directly If in a future version PostgresSQL works, then we will have to update this sub. But if an ORM is introduced, this code will be removed completly anyway
(In reply to comment #2) > Moving compilation of the module code from compile to runtime seems a bit > counter intuitive. Certainly if we start running the code in any kind of > persistent framework we want to do the opposite. > One thing that aids maintenenance is to only pollute (I mean import into ) your > namespace those subroutines that you need. i.e, use Module qw( function1 > function2); I agree it seems counter intuitive (well, it's illogic in fact ;-) ) However, as long as we don't have persistency, it's good to have those tweakings, as my tests shows. I've attached the 17 patches i've done, and here are some results : * Biblio.pm is well isolated and load only what's needed * Members is correctly isolated and doesn't load only what's needed * Reserves and Circulation still require a lot of investigations. They load almost everything and it can't be simplified easily. Circulation & Reserve are loading recursively Test on mainpage.pl : Profile of mainpage.pl for 1.08s (of 1.43s), executing 118729 statements and 29826 subroutine calls in 132 source files and 41 string evals. To compare with the same page before the de-nesting: Profile of mainpage.pl for 1.73s (of 2.18s), executing 155108 statements and 41041 subroutine calls in 249 source files and 64 string evals. ==> -36379 statements (-24% !) Test on opac-main.pl Before de-nesting & cleaning = Profile of opac/opac-main.pl for 1.96s (of 2.61s), executing 218971 statements and 51466 subroutine calls in 248 source files and 61 string evals. After de-nesting & cleaning = Profile of opac/opac-main.pl for 1.29s (of 1.88s), executing 182574 statements and 40411 subroutine calls in 138 source files and 38 string evals. ==> -36397 statements (-16%) The timings are not fully relevants, they depend on too much things (like mysql having the data in cache or no). All tests have been done without any tweaking (ie : no memcache, basic mysql/apache config...) > It also flags up to those who come after you how much of that module is used. ??? I don't understand what you mean here ?
Adding link to 7025, which I had filed without reading the the C4/Items.pm patch attached here. I'm not marking 7025 as a duplicate, however, since it's a more direct fix to one small aspect of this overall bug report. We can safely integrate the fix in 7025 without making all the other C4/Items.pm changes in the patch attached here.
Comment on attachment 5497 [details] [review] Bug 6875 de-nesting C4/Branch.pm Marking C4/Branch.pm patch as obsolete, since I've moved it to 7034 (and signed off/Passed QA)
Comment on attachment 5500 [details] [review] Bug 6875 de-nesting C4::ClassSort & ClassSortRoutine Marking C4::ClassSource and C4::ClassSortRoutine patch as obsolete here, since they are moved to 7035.
Comment on attachment 5506 [details] [review] Bug 6875 de-nesting C4::Circulation Marking patch obsolete here, as it's been moved to 7036
Comment on attachment 5507 [details] [review] Bug 6875 de-nesting C4/Print.pm Obsoleting, as this is moved to 7036 (with signoff)
Comment on attachment 5508 [details] [review] Bug 6875 de-nesting C4::Reserve, 1st step Marking obsolete, since I'm moving this patch to it's specific bug report (7051)
Updating Version : This ENH will be for Koha 3.8
Bug versionned for master. entries will be made against rel_3_8 once the patch has been applied (see thread about that on koha-devel yesterday)
Comment on attachment 5512 [details] [review] Bug 6875 C4::Context cleaning Moving C4::Context cleaning patch to bug 7188
I tested the new benchmark_staff.pl script that does some load testing the results are great ( results compared with master, see http://wiki.koha-community.org/wiki/Benchmark_for_3.8) 1st column is with those patches, 2nd column is master 15311 vs 23709 -35% for the mainpage.pl !!! Some of the attached patches does not apply anymore, i've rebased on my laptop, but not uploaded them for instance.
1) Changes to Auth.pm do not apply: Bug 6875 de-nesting Auth.pm (1.29 KB, patch) Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y Applying: Bug 6875 de-nesting Auth.pm Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging C4/Auth.pm CONFLICT (content): Merge conflict in C4/Auth.pm Failed to merge in the changes. Patch failed at 0001 Bug 6875 de-nesting Auth.pm When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". 2) Changes to C4:Biblio do not apply: Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y Applying: Bug 6875 de nesting C4::Biblio /home/katrin/kohaclone/.git/rebase-apply/patch:292: trailing whitespace. /home/katrin/kohaclone/.git/rebase-apply/patch:425: trailing whitespace. fatal: sha1 information is lacking or useless (C4/Serials.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 6875 de nesting C4::Biblio When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". 3) Also failing: Items.pm Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y Applying: Bug 6875 de-nesting C4::Items fatal: sha1 information is lacking or useless (C4/Items.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 6875 de-nesting C4::Items When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". 4) C4:Member Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y Applying: bug 6875 de-nesting C4::Member /home/katrin/kohaclone/.git/rebase-apply/patch:81: trailing whitespace. FROM borrowers /home/katrin/kohaclone/.git/rebase-apply/patch:89: trailing whitespace. FROM borrowers /home/katrin/kohaclone/.git/rebase-apply/patch:90: trailing whitespace. LEFT JOIN categories ON borrowers.categorycode=categories.categorycode /home/katrin/kohaclone/.git/rebase-apply/patch:271: trailing whitespace. GetMemberFlags warning: 4 lines add whitespace errors. Using index info to reconstruct a base tree... <stdin>:81: trailing whitespace. FROM borrowers <stdin>:89: trailing whitespace. FROM borrowers <stdin>:90: trailing whitespace. LEFT JOIN categories ON borrowers.categorycode=categories.categorycode <stdin>:271: trailing whitespace. GetMemberFlags warning: 4 lines applied after fixing whitespace errors. Falling back to patching base and 3-way merge... Auto-merging C4/ILSDI/Services.pm Auto-merging C4/Members.pm CONFLICT (content): Merge conflict in C4/Members.pm Auto-merging circ/circulation.pl Auto-merging circ/returns.pl Auto-merging opac/sco/sco-main.pl Auto-merging t/db_dependent/lib/KohaTest/Members.pm Failed to merge in the changes. Patch failed at 0001 bug 6875 de-nesting C4::Member When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". 5) opac-detail.pl Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y Applying: Bug 6875 cleaning opac-detail.pl fatal: sha1 information is lacking or useless (opac/opac-detail.pl). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 6875 cleaning opac-detail.pl When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". Other patches still apply.
Created attachment 7359 [details] [review] Bug 6875 de-nesting Auth.pm * removed use C4::Koha that is useless * moved "use C4::Members" to "require C4::Members" just before GetMemberDetails call. This will avoid loading C4::Member everytime a page is called by someone not logged * still to do = work on C4::VirtualShelves, that can be optimized, definetly ! Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Rebased on latest master, 28 Jan 2012
Created attachment 7360 [details] [review] [PASSED_QA][PATCH 06]Bug 6875 de-nesting C4::Koha.pm * removed use C4::Output, that was not used * moved use URI::split to getitemtypeimagelocation and switched to "require". Checked it still works through admin/itemtype.pl * moved use Business::ISBN to _isbn_cleanup.pl and switched to "require". Checked it still works with the following small script : use C4::Koha; print GetNormalizedISBN("1-56592-257-3"); (GetNormalizedISBN uses _isbn_cleanup) Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 7361 [details] [review] [PASSED_QA][PATCH 07]Bug 6875 cleaning mainpage.pl AutoritiesMarc is loaded for nothing : we don't display authorities anywhere, so removing the dependancy as well as the useless code Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 7362 [details] [review] [PASSED_QA][PATCH 08]Bug 6875 cleaning opac-main.pl C4::VirtualShelves, C4::Branch and C4::Acquisition are useless, removing their loading Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 7363 [details] [review] [PASSED_QA][PATCH 09]Bug 6875 de-nesting C4/Serials * use C4::Branch is loaded only to call GetBranchNAme in PrepareSerialsData and set branchname in the hashresult. This sub PrepareSerialsData is used in the following scripts : opac/opac-serial-issues.pl: my $subscriptioninformation=PrepareSerialsData($subscriptions); => no use of branchname after serials/serials-collection.pl: $subscriptions=PrepareSerialsData(\@subscriptioninformation); => no use of branchname after => we can remove the ->{branchname} from the result, and remove the C4::Branch dependancy * moves use C4::Items to require C4::Items, to call AddItemFromMarc, when receiving a serial, with Recieving create an item set in the subscription. * removed use C4::Letters and C4::Search, that are useless Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 7364 [details] [review] [PASSED_QA][PATCH 10]Bug 6875 de-nesting C4::Heading C4::Search is needed only in authorities sub, moving it here and switching to require Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Corrected call to SimpleSearch in both subs where it is used. Additional performance improvements are included in bug 7284. 28 Jan 2012
Created attachment 7365 [details] [review] [PASSED_QA][PATCH 11]Bug 6875 de-nesting C4::VirtualShelves * C4::Circulation is unused, removing it * C4::Members used only in ShelfPossibleAction, switching from use to require * C4::Auth used only in RefreshShelvesSummary, a sub called only in opac-addbybiblionumber.pl script, moving the require inside the sub Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
I tested and signed off on the following patches: De-nesting Auth.pm De-nesting C4::Koha.pm Cleaning mainpage.pl Cleaning opac-main.pl De-nesting C4/Serials De-nesting C4::Heading De-nesting C4::VirtualShelves The code in those patches is pretty clean. Three patches would not apply: De-nesting C4::Biblio De-nesting C4::Items Cleaning opac-detail.pl One patch fails QA on account of breaking circulation: De-nesting C4::Member I would like to encourage the QA team to review the patches that I signed off on so that they can be pushed to Master, rather than waiting for revised patches for the four that I didn't sign off on. Removing unnecessary dependencies is a result greatly to be desired.
(In reply to comment #39) > I would like to encourage the QA team to review the patches that I signed off > on so that they can be pushed to Master, rather than waiting for revised > patches for the four that I didn't sign off on. Agreed, as all those patches are independant from each other, and change nothing to the Koha behaviour (at least if they're not bugguy ;-) ) I'll ask Marcel & Ian by pm : they are small patches, should be easy to QA
Testing the first one: Applying: Bug 6875 de nesting C4::Biblio /usr/share/koha/testclone/.git/rebase-apply/patch:292: trailing whitespace. /usr/share/koha/testclone/.git/rebase-apply/patch:425: trailing whitespace. error: patch failed: C4/Items.pm:78 error: C4/Items.pm: patch does not apply fatal: sha1 information is lacking or useless (C4/Serials.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001.
Testing the second one: Applying: Bug 6875 de-nesting C4::Items error: patch failed: C4/Items.pm:1218 error: C4/Items.pm: patch does not apply fatal: sha1 information is lacking or useless (C4/Items.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001.
Testing the third one: Applying: bug 6875 de-nesting C4::Member /usr/share/koha/testclone/.git/rebase-apply/patch:81: trailing whitespace. FROM borrowers /usr/share/koha/testclone/.git/rebase-apply/patch:89: trailing whitespace. FROM borrowers /usr/share/koha/testclone/.git/rebase-apply/patch:90: trailing whitespace. LEFT JOIN categories ON borrowers.categorycode=categories.categorycode /usr/share/koha/testclone/.git/rebase-apply/patch:271: trailing whitespace. GetMemberFlags error: patch failed: C4/Members.pm:27 error: C4/Members.pm: patch does not apply error: patch failed: opac/sco/sco-main.pl:95 error: opac/sco/sco-main.pl: patch does not apply error: patch failed: t/db_dependent/lib/KohaTest/Members.pm:14 error: t/db_dependent/lib/KohaTest/Members.pm: patch does not apply Using index info to reconstruct a base tree... <stdin>:81: trailing whitespace. FROM borrowers <stdin>:89: trailing whitespace. FROM borrowers <stdin>:90: trailing whitespace. LEFT JOIN categories ON borrowers.categorycode=categories.categorycode <stdin>:271: trailing whitespace. GetMemberFlags warning: 4 lines add whitespace errors. Falling back to patching base and 3-way merge... Auto-merged C4/ILSDI/Services.pm Auto-merged C4/Members.pm CONFLICT (content): Merge conflict in C4/Members.pm Auto-merged C4/SIP/ILS/Patron.pm Auto-merged circ/circulation.pl Auto-merged circ/returns.pl Auto-merged members/deletemem.pl Auto-merged opac/sco/sco-main.pl Auto-merged t/db_dependent/lib/KohaTest/Members.pm Failed to merge in the changes. Patch failed at 0001.
Additional comment on the third one: I see reference to t/db_dependent/lib/KohaTest/Members.pm. I already sent Paul a request to delete those failing tests in that subdir. They are not working (anymore). Please do not include it here.
Testing the 4th: Applying: Bug 6875 cleaning opac-detail.pl error: patch failed: opac/opac-detail.pl:302 error: opac/opac-detail.pl: patch does not apply fatal: sha1 information is lacking or useless (opac/opac-detail.pl). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001.
Testing the 5th: Reacting on this line: * still to do = work on C4::VirtualShelves, that can be optimized, definetly ! I would propose to wait with that. I have a patch forthcoming for bug 7310 (large one) with a lot of work for virtual shelves.
Created attachment 7652 [details] [review] [PASSED_QA][PATCH 05]Bug 6875 de-nesting Auth.pm
Patch 06 for Koha.pm marked as Passed QA too.
Patch 7 (cleaning mainpage.pl) Removing useless code. Great! Passed QA too.
Patch 08 for opac-main.pl: Passed QA
Comments on 9th patch for Serials.pm: Does take out 6 use statements, adds 2 requires. Due to circular module dependency between Serials and Biblio, compiling Serials still results in many subroutine redefinition warnings. But this is the case already now (just observing it). Subroutine GetSuppliersWithLateIssues redefined at C4/Serials.pm line 87. Subroutine GetLateIssues redefined at C4/Serials.pm line 110. [etc][etc] Since this patch makes a move in the right direction, marking it as Passed QA.
Patch 10 for Heading.pm: Passed QA
Patch 11 for VirtualShelves.pm: Passed QA (will rebase my work for 7310)
Created attachment 7654 [details] [review] Bug 6875 de nesting C4::Biblio C4::Biblio is used in many many places. The goal of this cleaning is to do from C4::Biblio a package with as many dependancies as possible. * C4::Heading is called only in 1 place, highly rarely used (only in 1 misc/link_bibs_to_authorities.pl), moving to require * PrepareItemrecordDisplay is a sub that is more related to Items, moving it here. It means some scripts that used this sub must be checked against use C4::Items * C4::Items is needed in EmbedItemsInMarcBiblio, moving it only in this sub, and switching to require * 2 subs are totally useless z3950_extended_services and set_service_options, removing them
Created attachment 7655 [details] [review] Bug 6875 de-nesting C4::Items C4::Branch is used only in CheckItemPresave, moving from a use to a require in the sub C4::Reserve: 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; } C4::Acquisition is used only in MoveItemFromBiblio, a sub that is rarely called. Moving from a use to a require in the sub C4::Charset is used only in _parse_unlinked_item_subfields_from_xml. Moving from a use to require in the sub
Created attachment 7656 [details] [review] [HAS_SOME_PROBLEM]bug 6875 de-nesting C4::Member * working on GetMemberDetails. This sub must be optimized, as it is called on each page by C4::Auth. The sub call everytime patronflags, that need to load Reserves and Overdues. The patronflag returns a hash with various flags (overdues, fines, ...). It's not related to permissionflags, that are loaded elsewhere. Most of the time, we do nothing with those flags. The strategy is to split the GetMemberDetails in 2 subs : * GetMemberDetails, same as previously, without the flags * GetMemberFlags, that will return just the flags when needed (and will load when needed Reserves package) The following scripts call GetMemberDetails : * admin/aqbudget.pl = no use is done with flags, the splitting changes nothing * C4/SIP/ILS/Patron.pm = the flags are used later, so we call GetMemberFlags as well * C4/SIP/interactive_members_dump.pl = nothing is done with the flags, we change nothing * C4/Print.pm = used by printslip, that don't do anything with flags, we change nothing * C4/VirtualShelves = used to check permission to access to a virtualshelf, nothing done with flags, we change nothing * C4/Reserves = used in CheckReserves. With the result, we call _GetCircControlBranch only, that does nothing with flags, we change nothing * C4/Auth = used in get_template_and_user. The return from GetMemberDetails is stored in the session, I don't see any change, flags are useless * C4/Circulation.pm = used many times * in CanBookBeIssued, just to retrieve the name/cardnumber/borrowernumber of a book already issued to someone else * in CanBookBeIssued, just to retrieve the name/cardnumber/borrowernumber of a book already reserved by someone else * in AddReturn, the result of GetMemberDetails is returned. It's then used in return.pl, to display patron flags. Retrieving flags with GetMemberFlags in return.pl * in AddRenewal, to retrieve some information, nothing done with flags, we change nothing * in GetRenewCount, to retrieve name & categorycode, nothing done with flags, we change nothing * in ILSDI/Services.pm, GetMemberDetails is called many times. Only once we check for charges, that is in the flags, so retrieving flags with GetMemberFlags * in catalogue/detail.pl, GetMemberDetails is called to retrieve the name of a patron that has placed a hold on a book, we change nothing * in circ/transferstoreceive, GetMemberDetails is called to retrieve the name of a patron, (and address), we change nothing * in circ/circulation.pl, we must retrieve the flags to display them (like for return.pl) * in circ/waitingreserves.pl, GetMemberDetails is called to retrieve the name of the patron, nothing to change * in members/moremember.pl, GetMemberDetails is called to retrieve name, surname, borrowernumber, we change nothing * in members/deletemem.pl, GetMemberDetails is followed by a $flag = $bor->{flags}, replacing by a call to GetMemberDetails * in members/messaging.pl, GetMemberDetails is called to retrieve name, category, nothing to change * members/member-flags.pl is here for permissions, nothing to deal with patron flags, nothing to change * opac/opac-passwd.pl, nothing related to flags, nothing to change * opac/opac-ics.pl, nothing related to flags, nothing to change * opac/opac-renew.pl, used to retrieve only branchcode, nothing to change * opac/opac-account.pl, nothing to change, the core part of this script is to display fines that are retrieved by GetMemberAccountRecords * opac/opac-reserve.pl, nothing related to flags, nothing to change * opac/opac-userdetail.pl, script that retrieve only a few dates and other infos from GetMemberDetails, nothing to change * opac/opac-showreviews.pl, retrieve only name & email from GetMemberDetails, nothing to change * opac/SCO/sco-main.pl, calls canbookbeissued, that need flags, so retrieving them * opac/opac-readingrecord.pl, use GetMemberDetails to retrieve patron name, address... nothing to change * opac/opac-messaging.pl, use GetMemberDetails to retrieve sms informations, nothing to change * opac/opac-userupdate.pl, use GetMemberDetails to display member information that can be updated by the patron, nothing related to flags, nothing to change * opac/opac-privacy.pl, loaded just to retrieve name & privacy flag info, nothing to change * opac/opac-user.pl, use GetMemberDetails to display patron information, but not flags one, nothing to change * opac/opac-suggestion.pl use GetMemberDetails to retrieve the branchcode, nothing to change * reserve/request.pl use GetMemberDetails to retrieve many informations displayed (name, branchcode,...), but none related to flags. Nothing to change GetMemberDetails also launches 2 SQL queries, the 2nd being superfluous : the enrolmentperiod is already available in the 1st query, just return it ! => C4::Reserves & C4::Overdues are now used only in GetMemberFlags sub, that is not always called, so switching from a use to a require inside the sub C4::Accounts is used only in AddMember and ExtendMemberSubscriptionTo, and only if there is a fee for subscription, switching from use to require sub GetMemberAccountRecords the SQL query has been completed. With a LEFT JOIN, we retrieve directly the title & biblionumber. Thus the call to GetBiblioByItemNumber is now useless and the use C4::Biblio can be removed
Created attachment 7657 [details] [review] [FAILED_QA]Bug 6875 cleaning opac-detail.pl C4::Tags and C4::Reviews used only if the feature is activated. Changing the use to a require inside the if (after testing it's activated) For libraries that have activated both, that means no change, but for others, it's an improvement
Created attachment 7658 [details] [review] C4::Members follow-up Overdues already loaded, must set full sub patch
Patches that do no apply have been rebased and re-submitted. Patches that have been pushed have been obsoleted Please signoff/QA again on those patches. I haven't addressed comment #44: > Additional comment on the third one: > I see reference to t/db_dependent/lib/KohaTest/Members.pm. > I already sent Paul a request to delete those failing tests in that subdir. > They are not working (anymore). Please do not include it here. I still have to remove the tests, so, until it's done, those lines are useless but harmless. I've seen comment #39 > One patch fails QA on account of breaking circulation: > De-nesting C4::Member but could not figure what's happening for now. I made a follow-up that fixes a part of the problem (execution error), but the page seems broken, with many informations empty (1 issue, not displayed, patron surname empty...) Please ignore this patch if you're testing them. You can signoff/QA the others though
Created attachment 7770 [details] [review] [PASSED_QA]Bug 6875 Biblio module Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> No test plan included, but tested some opac and cataloging functions. Code looks good. Marked as Passed QA. NOTE: Includes changes to obsolete test files. Should actually be removed.
Created attachment 7771 [details] [review] [PASSED_QA]Bug 6875 Items module Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Checked opac-detail and cataloging. Code looks good. Marked as Passed QA.
QA Comment on patch cleaning up opac-detail.pl: Use of C4::Tags is replaced by later require. Introduces following problem: [error] opac-detail.pl: Too late to run INIT block at /usr/share/koha/testclone/C4/Tags.pm line 64. Marked this one as Failed QA in name of patch attachment.
Updating status to Passed QA for the two patches marked as such. Please set status back afterwards.
Comment on attachment 7770 [details] [review] [PASSED_QA]Bug 6875 Biblio module this patch has been pushed
Comment on attachment 7771 [details] [review] [PASSED_QA]Bug 6875 Items module this patch has been pushed
The patch pushed to master has a bug: C4::Items calls _find_value() that is in Biblio.
(In reply to comment #66) > The patch pushed to master has a bug: C4::Items calls _find_value() that is in > Biblio. Good catch, Srdjan! Anyone already working on this one? Paul perhaps??
(In reply to comment #67) > (In reply to comment #66) > > The patch pushed to master has a bug: C4::Items calls _find_value() that is in > > Biblio. > Good catch, Srdjan! Anyone already working on this one? Paul perhaps?? yep. I plan to work on this in the next hours
Created attachment 7808 [details] [review] Bug 6875 follow-up for Items/Biblio de-nesting the sub _find_value is used only in PrepareItemRecord sub, that has been moved to Items package This patch moves the _find_value in Items as well.
How to test the attachment 7808 [details] [review] : before applying the patch: * reach a serial that has "serial recieve create items" * reach serials-collection.pl page * check a serial that has already be recieved, and click on "Edit Serials" => Perl error After applying the patch = works fine.
Created attachment 7844 [details] [review] [PASSED_QA] Bug 6875 follow-up for Items/Biblio de-nesting the sub _find_value is used only in PrepareItemRecord sub, that has been moved to Items package This patch moves the _find_value in Items as well. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Editing an already arrived serial issue with an attached item resulted in an error. After applying the patch it's fixed.
Changing status so last patch can go through QA.
Changing status to Passed QA for Follow-up for Items/Biblio de-nesting
Comment on attachment 7844 [details] [review] [PASSED_QA] Bug 6875 follow-up for Items/Biblio de-nesting this patch has been pushed
Created attachment 8095 [details] [review] C4::Members follow-up Overdues already loaded, must set full sub patch http://bugs.koha-community.org/show_bug.cgi?id=6875 Signed-off-by: Liz Rea <wizzyrea@gmail.com> Fixes issue with circ/branchoverdues.pl Passes tests.
setting to signed off for C4::Members follow-up.
(In reply to comment #76) > setting to signed off for C4::Members follow-up. It is only useful in connection with the first Members patch. And that appears to be still in construction. And could be done in one pass then. Changing status to reflect that.
De-nesting C4::Items has resulted in bulkmarcimport.pl throwing the error: ERROR: Adding items to bib 1 failed: Undefined subroutine &C4::Items::GetBranchName called at /home/sekjal/kohaclone/C4/Items.pm line 656, <GEN13> line 435. GetBranchName is not scoped properly. Adding the C4::Branch:: prefix will fix this. Patch forthcoming.
Created attachment 8133 [details] [review] Bug 6875 Followup: De-nesting C4::Items breaks bulkmarcimport Removing 'use C4::Branch' in favour of a subroutine-specific 'require C4::Branch' causes the GetBranchName subroutine to return an error, as it's not defined in C4::Items. Adding "C4::Branch::" scoping fixes the error, which is what's done here. To confirm problem: 1. Attempt to run bulkmarcimport.pl before applying the patch. You should get ERROR: Adding items to bib 435 failed: Undefined subroutine &C4::Items::GetBranchName called at /home/sekjal/kohaclone/C4/Items.pm line 656, <GEN13> line 435. To test: 2. apply patch 3. run bulkmarcimport again. Error should disappear. Signed-off-by: Ian Walls <koha.sekjal@gmail.com>
Created attachment 8134 [details] [review] Bug 6875 Followup: De-nesting C4::Items breaks bulkmarcimport Removing 'use C4::Branch' in favour of a subroutine-specific 'require C4::Branch' causes the GetBranchName subroutine to return an error, as it's not defined in C4::Items. Adding "C4::Branch::" scoping fixes the error, which is what's done here. To confirm problem: 1. Attempt to run bulkmarcimport.pl before applying the patch. You should get ERROR: Adding items to bib 435 failed: Undefined subroutine &C4::Items::GetBranchName called at /home/sekjal/kohaclone/C4/Items.pm line 656, <GEN13> line 435. To test: 2. apply patch 3. run bulkmarcimport again. Error should disappear. Signed-off-by: Ian Walls <koha.sekjal@gmail.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Note that this problem only appears when importing records with item (952) fields.
Created attachment 8135 [details] [review] Bug 6875 Followup: De-nesting C4::Items breaks bulkmarcimport Removing 'use C4::Branch' in favour of a subroutine-specific 'require C4::Branch' causes the GetBranchName subroutine to return an error, as it's not defined in C4::Items. Adding "C4::Branch::" scoping fixes the error, which is what's done here. To confirm problem: 1. Attempt to run bulkmarcimport.pl before applying the patch. You should get ERROR: Adding items to bib 435 failed: Undefined subroutine &C4::Items::GetBranchName called at /home/sekjal/kohaclone/C4/Items.pm line 656, <GEN13> line 435. To test: 2. apply patch 3. run bulkmarcimport again. Error should disappear. Signed-off-by: Ian Walls <koha.sekjal@gmail.com> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Note that this problem only appears when importing records with item (952) fields.
Comment on attachment 8135 [details] [review] Bug 6875 Followup: De-nesting C4::Items breaks bulkmarcimport this patch has been pushed
C4::Members follow-up patch passed QA.
Comment on attachment 8095 [details] [review] C4::Members follow-up this patch has been pushed (on master branch directly)
Why is this assigned? Are their still problems with it?
Also, are THERE problems with it.
Time to ask again! There are no more open bugs on which this one depends. Can it be considered resolved?
+1 closing