Apparently older versions of Koha considered a MARC subfield in the frameworks that had a code longer than one character to be reserved for internal use. This no longer applies, so the routine and the last couple uses of it can be removed.
This bug is mentioned in: Bug 5404 Followup http://lists.koha-community.org/pipermail/koha-patches/2010-November/013024.html
$ git grep subfield_is_koha_internal_p C4/Acquisition.pm:use C4::Koha qw( subfield_is_koha_internal_p ); C4/Acquisition.pm: next if ( subfield_is_koha_internal_p($subfield) ); C4/Items.pm: next if ( subfield_is_koha_internal_p($subfield) ); C4/Koha.pm: &subfield_is_koha_internal_p C4/Koha.pm:sub subfield_is_koha_internal_p { authorities/authorities-home.pl:use C4::Koha; # XXX subfield_is_koha_internal_p authorities/authorities.pl:use C4::Koha; # XXX subfield_is_koha_internal_p cataloguing/addbiblio.pl:use C4::Koha; # XXX subfield_is_koha_internal_p cataloguing/addbiblio.pl:use C4::Branch; # XXX subfield_is_koha_internal_p cataloguing/additem.pl:use C4::Koha; # XXX subfield_is_koha_internal_p cataloguing/additem.pl:use C4::Branch; # XXX subfield_is_koha_internal_p cataloguing/additem.pl: next if subfield_is_koha_internal_p($subfieldtag); cataloguing/additem.pl: next if subfield_is_koha_internal_p($subtag); labels/label-item-search.pl:use C4::Koha qw(GetItemTypes); # XXX subfield_is_koha_internal_p opac/opac-authorities-home.pl:use C4::Koha; # XXX subfield_is_koha_internal_p reports/guided_reports.pl:use C4::Branch; # XXX subfield_is_koha_internal_p tools/batchMod.pl:use C4::Koha; # XXX subfield_is_koha_internal_p tools/batchMod.pl:use C4::Branch; # XXX subfield_is_koha_internal_p tools/batchMod.pl: next if subfield_is_koha_internal_p($subfield);
Created attachment 47968 [details] [review] Bug 5404: C4::Koha - remove subfield_is_koha_internal_p The commit b5ecefd485a75d54a5fa26fff5a0cc890541e2c3 Date: Mon Feb 3 18:46:00 2003 +0000 had a funny description: Added function to check if a MARC subfield name is "koha-internal" (instead of checking it for 'lib' and 'tag' everywhere); temporarily added to Koha.pm "Temporarily", since 2003, everything is relative, isn't it? :) The thing is that GetMarcStructure returns hash like field_200 => { subfield_a => { %attributes_of_subfield_a }, %attributes_of_field_200 } The attributes for field_200 can be 'repeatable', 'mandatory', 'tag', 'lib'. We don't want to loop on these values when looping on subfields. Since there are just { k => v } with v is a scalar (string), it's easier to test if we are processing a subfield testing the reference. At some places, we don't need to test that, we are looping on values from MARC::Field->subfields which are always valid subfields. Test plan: 1/ Edit items using the batch item mod tool 2/ display and edit items via the cataloguing module. You should not see any changes between before and after the patch applied. Tech notes: We need to check what we are processing when we loop on 'subfields' from GetMarcStructure, not from MARC::Field->subfields.
Created attachment 48099 [details] [review] Bug 5404: C4::Koha - remove subfield_is_koha_internal_p The commit b5ecefd485a75d54a5fa26fff5a0cc890541e2c3 Date: Mon Feb 3 18:46:00 2003 +0000 had a funny description: Added function to check if a MARC subfield name is "koha-internal" (instead of checking it for 'lib' and 'tag' everywhere); temporarily added to Koha.pm "Temporarily", since 2003, everything is relative, isn't it? :) The thing is that GetMarcStructure returns hash like field_200 => { subfield_a => { %attributes_of_subfield_a }, %attributes_of_field_200 } The attributes for field_200 can be 'repeatable', 'mandatory', 'tag', 'lib'. We don't want to loop on these values when looping on subfields. Since there are just { k => v } with v is a scalar (string), it's easier to test if we are processing a subfield testing the reference. At some places, we don't need to test that, we are looping on values from MARC::Field->subfields which are always valid subfields. Test plan: 1/ Edit items using the batch item mod tool 2/ display and edit items via the cataloguing module. You should not see any changes between before and after the patch applied. Tech notes: We need to check what we are processing when we loop on 'subfields' from GetMarcStructure, not from MARC::Field->subfields. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
QA Comment: I am not sure if this really is a better solution. I also doubt about the remark in comment 1 "Apparently older versions of Koha considered a MARC subfield in the frameworks that had a code longer than one character to be reserved for internal use". To me it just seems to be a trick to skip the internal fields lib, tab, mandatory and repeatable (all length>1). In terms of design it would have been easier to put all subfields a level deeper instead of on the same level as lib, etc. I understand about not changing the design of the structure here. But now we move the test length<>1 from a subroutine into the code and check if it is a ref or not. I probably would like to still have the test in the subroutine (with all benefits of a subroutine); another name would be fine. If someone wants to add some other property to the structure and suppose that is an arrayref or a hashref or an object, your test would fail. I would propose to test (in the sub): [a] skip if the field name matches lib|tab|mandatory|repeatable or [b] proceed if the field is a ref called MARC::Field (instead of testing if ref is true, we test ref eq ..) Obviously, I agree with removing the call for a loop on only real subfields. Changing status
(In reply to Marcel de Rooy from comment #5) > or [b] proceed if the field is a ref called MARC::Field (instead of testing > if ref is true, we test ref eq ..) Oops. That is not true.
(In reply to Marcel de Rooy from comment #5) > QA Comment: > I am not sure if this really is a better solution. > I also doubt about the remark in comment 1 "Apparently older versions of > Koha considered a MARC subfield in the frameworks that had a code longer > than one character to be reserved for internal use". To me it just seems to > be a trick to skip the internal fields lib, tab, mandatory and repeatable > (all length>1). > > In terms of design it would have been easier to put all subfields a level > deeper instead of on the same level as lib, etc. > > I understand about not changing the design of the structure here. Yes, that would be a next step. > But now we move the test length<>1 from a subroutine into the code and check > if it is a ref or not. > I probably would like to still have the test in the subroutine (with all > benefits of a subroutine); another name would be fine. Not sure it's needed, as the test is very simple. Where would you move this subroutine? > If someone wants to add some other property to the structure and suppose > that is an arrayref or a hashref or an object, your test would fail. Yes but it works for the moment :) The structure has not changed for years. > I would propose to test (in the sub): > [a] skip if the field name matches lib|tab|mandatory|repeatable Imo, that would make the test less strong and too specific.
(In reply to Jonathan Druart from comment #7) > Not sure it's needed, as the test is very simple. > Where would you move this subroutine? If we would ever have to change this very simple test, it would also be very easy to overlook a simple test. I would keep it close to GetMarcStructure. > Yes but it works for the moment :) The structure has not changed for years. True > Imo, that would make the test less strong and too specific. OK So, I prefer to move the ref-test to a subroutine in Biblio.pm IsMarcStructureInternal ? or something ? That would be the only place to document this test.
Created attachment 48436 [details] [review] Bug 5404: Move the test to a new IsMarcStructureInternal sub Same test plan as previous patch
Josef: Could you please add your signoff on the follow-up (if possible..) ? This patch is on my QA list for Friday.
I just tried the patch and I believe there is typo in C4/Acquisition.pm on line 2999: next if IsMarcStructureInternal(tagslib->{$tag}{$subfield}); there is missing $ before variable "tagslib" Beside that the patch is working, so I will be happy to add sign-off when this will be fixed. Could you do that Jonathan? Thanks
Created attachment 48450 [details] [review] Bug 5404: Move the test to a new IsMarcStructureInternal sub
(In reply to Josef Moravec from comment #11) > Could you do that Jonathan? Done! Sorry about that, I have caught the error with qa tools but forgotten to amend the patch.
Created attachment 48478 [details] [review] Bug 5404: Move the test to a new IsMarcStructureInternal sub Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Everything looks good, I'm signing off. Thanks Jonathan
Created attachment 48673 [details] [review] Bug 5404: C4::Koha - remove subfield_is_koha_internal_p The commit b5ecefd485a75d54a5fa26fff5a0cc890541e2c3 Date: Mon Feb 3 18:46:00 2003 +0000 had a funny description: Added function to check if a MARC subfield name is "koha-internal" (instead of checking it for 'lib' and 'tag' everywhere); temporarily added to Koha.pm "Temporarily", since 2003, everything is relative, isn't it? :) The thing is that GetMarcStructure returns hash like field_200 => { subfield_a => { %attributes_of_subfield_a }, %attributes_of_field_200 } The attributes for field_200 can be 'repeatable', 'mandatory', 'tag', 'lib'. We don't want to loop on these values when looping on subfields. Since there are just { k => v } with v is a scalar (string), it's easier to test if we are processing a subfield testing the reference. At some places, we don't need to test that, we are looping on values from MARC::Field->subfields which are always valid subfields. Test plan: 1/ Edit items using the batch item mod tool 2/ display and edit items via the cataloguing module. You should not see any changes between before and after the patch applied. Tech notes: We need to check what we are processing when we loop on 'subfields' from GetMarcStructure, not from MARC::Field->subfields. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 48674 [details] [review] Bug 5404: Move the test to a new IsMarcStructureInternal sub Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 48675 [details] [review] Bug 5404: [QA Follow-up] Add test descriptions Adding descriptions for changes in t/db_dependent/Biblio.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #18) > Created attachment 48675 [details] [review] [review] > Bug 5404: [QA Follow-up] Add test descriptions > > Adding descriptions for changes in t/db_dependent/Biblio.t. Oops forgot that, thanks for the follow-up!
Pushed to Master - Should be in the May 2016 release. Thanks!