While testing bug 30789 it was found that passing the already retrieved marc subfield structure into TransformMarckToKohaOneField increased performance by ~20% This seemed odd as the routine is already retrieving from cache unsafe Rather than pass around this shared variable, it seems to make sense to allow TransformMarcToKoha to take a list of fields to process, rather than have a seperate routine that must be called
Created attachment 135232 [details] [review] Bug 30813: Update TransformMarcToKoha to accept a hashref This patch updates all the calls to pass a hasref rather than an array It also removes the no longer used framework parameter To test: prove -v t/Biblio.t t/db_dependent/Biblio/TransformMarcToKoha.t
Created attachment 135233 [details] [review] Bug 30813: Adjust TransformMarcToKoha to take kohafields parameter This adjusts the routine to accept an arrayref of koha fields to process To test: prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/Biblio.t
Created attachment 135234 [details] [review] Bug 30813: Remove TransformMarcToKohaOneField To test: 1 - prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/db_dependent/Breeding.t 2 - Test Z3950 search in advanced catalog editor
Created attachment 135235 [details] [review] Bug 30813: Remove Koha::MetadataRecord::getKohaField This was essentially a wrapper around TransformKohaToMarcOneField After changing and updating, it seems easier not to create the intermediary object and to just transform the data
Created attachment 135239 [details] [review] Bug 30813: Update TransformMarcToKoha to accept a hashref This patch updates all the calls to pass a hasref rather than an array It also removes the no longer used framework parameter To test: prove -v t/Biblio.t t/db_dependent/Biblio/TransformMarcToKoha.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 135240 [details] [review] Bug 30813: Adjust TransformMarcToKoha to take kohafields parameter This adjusts the routine to accept an arrayref of koha fields to process To test: prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/Biblio.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 135241 [details] [review] Bug 30813: Remove TransformMarcToKohaOneField To test: 1 - prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/db_dependent/Breeding.t 2 - Test Z3950 search in advanced catalog editor Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 135242 [details] [review] Bug 30813: Remove Koha::MetadataRecord::getKohaField This was essentially a wrapper around TransformKohaToMarcOneField After changing and updating, it seems easier not to create the intermediary object and to just transform the data Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This works great and serves as another nice cleanup. QA script is happy, all unit tests for biblios and items are passing great. Signing off.
Created attachment 135243 [details] [review] Bug 30813: (follow-up) ImportBatch updates Missed these in the original commits
Created attachment 135302 [details] [review] Bug 30813: Update TransformMarcToKoha to accept a hashref This patch updates all the calls to pass a hasref rather than an array It also removes the no longer used framework parameter To test: prove -v t/Biblio.t t/db_dependent/Biblio/TransformMarcToKoha.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 135303 [details] [review] Bug 30813: Adjust TransformMarcToKoha to take kohafields parameter This adjusts the routine to accept an arrayref of koha fields to process To test: prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/Biblio.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 135304 [details] [review] Bug 30813: Remove TransformMarcToKohaOneField To test: 1 - prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/db_dependent/Breeding.t 2 - Test Z3950 search in advanced catalog editor Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 135305 [details] [review] Bug 30813: Remove Koha::MetadataRecord::getKohaField This was essentially a wrapper around TransformKohaToMarcOneField After changing and updating, it seems easier not to create the intermediary object and to just transform the data Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 135306 [details] [review] Bug 30813: (follow-up) ImportBatch updates Missed these in the original commits Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
If you go to > /cgi-bin/koha/cataloguing/addbooks.pl and perform a search using "New from Z39.50" you get: > Undefined subroutine &C4::Breeding::TransformMarcToKoha called at /kohadevbox/koha/C4/Breeding.pm line 311 to reproduce the error in the search pop-up put as the Title "aa". If you fix the missing import then the search results are still empty however (they should not be empty for this keyword), so there is a bit more to fix this, I will leave it to the author of the patch set. - Koha/MetaSearcher.pm doesn't import C4::Biblio - Koha/MetadataRecord.pm imports now C4::Biblio unnecessarily
Created attachment 135371 [details] [review] Bug 30813: (follow-up) Correct C4::Breeding call This patch changes the call to use the fully qualified name and adds import to C4::Breeding, Koha::MetaSearcher, and removes import from Koha::MetadataRecord Additionally it replaces missing fields from the update to using TransformMarcToKoha Lastly, it reduces the fields used when saving the bredding record to the reservoir
Created attachment 135374 [details] [review] Bug 30813: Update TransformMarcToKoha to accept a hashref This patch updates all the calls to pass a hasref rather than an array It also removes the no longer used framework parameter To test: prove -v t/Biblio.t t/db_dependent/Biblio/TransformMarcToKoha.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 135375 [details] [review] Bug 30813: Adjust TransformMarcToKoha to take kohafields parameter This adjusts the routine to accept an arrayref of koha fields to process To test: prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/Biblio.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 135376 [details] [review] Bug 30813: Remove TransformMarcToKohaOneField To test: 1 - prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/db_dependent/Breeding.t 2 - Test Z3950 search in advanced catalog editor Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 135377 [details] [review] Bug 30813: Remove Koha::MetadataRecord::getKohaField This was essentially a wrapper around TransformKohaToMarcOneField After changing and updating, it seems easier not to create the intermediary object and to just transform the data Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 135378 [details] [review] Bug 30813: (follow-up) ImportBatch updates Missed these in the original commits Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 135379 [details] [review] Bug 30813: (follow-up) Correct C4::Breeding call This patch changes the call to use the fully qualified name and adds import to C4::Breeding, Koha::MetaSearcher, and removes import from Koha::MetadataRecord Additionally it replaces missing fields from the update to using TransformMarcToKoha Lastly, it reduces the fields used when saving the bredding record to the reservoir Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
RM: I like to have another look at this one too still.
Taking a look here too
my $mss = GetMarcSubfieldStructure( '', { unsafe => 1 } ); # Do not change framework @{$kohafields} = keys %{ $mss } unless $kohafields; foreach my $kohafield ( @{$kohafields} ) { Hmm. I dont understand: Why pass kohafields and still do a GetMarcSubfieldSttructure call ?
(In reply to Marcel de Rooy from comment #26) > my $mss = GetMarcSubfieldStructure( '', { unsafe => 1 } ); # Do not > change framework > @{$kohafields} = keys %{ $mss } unless $kohafields; > foreach my $kohafield ( @{$kohafields} ) { > Hmm. I dont understand: Why pass kohafields and still do a > GetMarcSubfieldSttructure call ? Misreading the change. Sorry.
Coming back here very soon. Please keep status.
(In reply to Marcel de Rooy from comment #28) > Coming back here very soon. Please keep status. Ok, do you have an idea of the timing for this one?
(In reply to Tomás Cohen Arazi from comment #29) > (In reply to Marcel de Rooy from comment #28) > > Coming back here very soon. Please keep status. > > Ok, do you have an idea of the timing for this one? Will be going further this afternoon.
Created attachment 135763 [details] [review] Bug 30813: Update TransformMarcToKoha to accept a hashref This patch updates all the calls to pass a hasref rather than an array It also removes the no longer used framework parameter To test: prove -v t/Biblio.t t/db_dependent/Biblio/TransformMarcToKoha.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135764 [details] [review] Bug 30813: Adjust TransformMarcToKoha to take kohafields parameter This adjusts the routine to accept an arrayref of koha fields to process To test: prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/Biblio.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135765 [details] [review] Bug 30813: Remove TransformMarcToKohaOneField To test: 1 - prove -v t/db_dependent/Biblio/TransformMarcToKoha.t t/db_dependent/Breeding.t 2 - Test Z3950 search in advanced catalog editor Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135766 [details] [review] Bug 30813: Remove Koha::MetadataRecord::getKohaField This was essentially a wrapper around TransformKohaToMarcOneField After changing and updating, it seems easier not to create the intermediary object and to just transform the data Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135767 [details] [review] Bug 30813: (follow-up) ImportBatch updates Missed these in the original commits Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135768 [details] [review] Bug 30813: (follow-up) Correct C4::Breeding call This patch changes the call to use the fully qualified name and adds import to C4::Breeding, Koha::MetaSearcher, and removes import from Koha::MetadataRecord Additionally it replaces missing fields from the update to using TransformMarcToKoha Lastly, it reduces the fields used when saving the bredding record to the reservoir Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 135769 [details] [review] Bug 30813: (QA follow-up) Adjust three use statements Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Great work, Nick. Removing that framework parameter was still something far down on my todo list ;) Might still be a report for; will close it.
Why add the qw(); to the use statements?
(In reply to Nick Clemens from comment #39) > Why add the qw(); to the use statements? That is essential to prevent needlessly importing all default exported routines from the module. (EXPORT_OK routines)
(In reply to Marcel de Rooy from comment #40) > (In reply to Nick Clemens from comment #39) > > Why add the qw(); to the use statements? > > That is essential to prevent needlessly importing all default exported > routines from the module. (EXPORT_OK routines) From bug 17600 I understood that EXPORT_OK was 'on demand' i.e. only explicitly named subroutines are imported. Is that incorrect?
(In reply to Nick Clemens from comment #41) > (In reply to Marcel de Rooy from comment #40) > > (In reply to Nick Clemens from comment #39) > > > Why add the qw(); to the use statements? > > > > That is essential to prevent needlessly importing all default exported > > routines from the module. (EXPORT_OK routines) > > From bug 17600 I understood that EXPORT_OK was 'on demand' i.e. only > explicitly named subroutines are imported. Is that incorrect? Ah. You might very well be right. EXPORT versus EXPORT_OK So yes, might not be needed then. But explicitly stating that you dont want to import cannot be wrong either. I would recommend keeping them.
Please explicitly import the subroutines that are used in the scripts/modules, that's the point of bug 17600. That helps to clean the import (remove the 'use' statements when no subroutines are imported).
(In reply to Jonathan Druart from comment #43) > Please explicitly import the subroutines that are used in the > scripts/modules, that's the point of bug 17600. That helps to clean the > import (remove the 'use' statements when no subroutines are imported). No. You may want to require a module without importing its names. So you do use Module qw();
(In reply to Marcel de Rooy from comment #44) > (In reply to Jonathan Druart from comment #43) > > Please explicitly import the subroutines that are used in the > > scripts/modules, that's the point of bug 17600. That helps to clean the > > import (remove the 'use' statements when no subroutines are imported). > > No. > You may want to require a module without importing its names. > So you do use Module qw(); No? I can assure you that it's why I wrote bug 17600. We want to EXPLICITLY export the subroutines we will be using in the scripts/modules.
(In reply to Jonathan Druart from comment #45) > (In reply to Marcel de Rooy from comment #44) > > (In reply to Jonathan Druart from comment #43) > > > Please explicitly import the subroutines that are used in the > > > scripts/modules, that's the point of bug 17600. That helps to clean the > > > import (remove the 'use' statements when no subroutines are imported). > > > > No. > > You may want to require a module without importing its names. > > So you do use Module qw(); > > No? I can assure you that it's why I wrote bug 17600. We want to EXPLICITLY > export the subroutines we will be using in the scripts/modules. You dont understand what I mean. Please read it again. No refers to the case where we do not import. I say no, do not remove the use statement when you want to require the module anyway.
Ok see IRC for a lively discussion on how 17600 was meant ;) The implications here are: The last patch that 'corrected' a few use statements needs tweaking (or its predecessors). Although we do not need to import TransformMarcToKoha there since we call with prefix C4::Biblio, we should do it anyway for transparency/documentation etc. Will adjust the patch.
Created attachment 135803 [details] [review] Bug 30813: (QA follow-up) Adjust three use statements Altough we used the namespace when calling, the intention of bug 17600 was that we do import the routines that we are calling here. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed to master for 22.11. Nice work everyone, thanks!
Created attachment 135825 [details] [review] Bug 30813: (follow-up) Fix tests import Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>