This function iterates through it's argument - which is a hashref - using 'each'. Perl is not guaranteed to return hash keys in any particular order (not to mention that in more recent perl versions, explicit hash key order randomization is to be expected). As a consequence: 1) For biblio records added via acquisition (order from a new/empty record, order from a suggestion), freshly created MARC biblio records doesn't always have 260 $b and 260 $c stored in the proper order 2) Holdings data exported for zebra indexing as 952 fields may have subfields generated in more-or-less random order. While it probably (?) does not affect zebra indexing/searching in any significant way, end result is prone to be somehow ugly (which can be a potential issue e.g. for people running Z39.50 server) and is not guaranteed to be consistent; different records - or even different items in the same record, can have 952 subfields generated in indiscriminate order.
Vaguely remember some earlier discussion about this.. You did not find any references to this in older reports? :)
(In reply to M. de Rooy from comment #1) > Vaguely remember some earlier discussion about this.. You did not find any > references to this in older reports? :) I performed several bugzilla searches, but no luck - they do not seem to yield anything relevant. Also, according to http://splitter.koha-community.org/, there are no C4/Biblio.pm patches for this issue currently awaiting in the queue (?). I'm wondering which approach might be best for fixing this: - just sort (or reverse sort) the darn hash keys - that's a simple one; results would be perhaps not particularly well-sorted, but at least they will be sorted consistently ;) - sort by field tags & subfield codes (but this function is apparently pretty sensitive performance-wise ?) - make it to accept arrayref instead of (or in addition to) hashref as an argument, so the subfield order would be preserved - but that would not be all that helpfull for item/holding data subfields which come from the database in the hashref form.
Created attachment 28631 [details] [review] Bug 12343 - TransformKohaToMarc() is adding MARC subfields in random order TransformKohaToMarc() function in C4/Biblio.pm iterates through it's argument - which is a hashref - using 'each'. Perl is not guaranteed to return hash keys in any particular order (not to mention that in more recent perl versions, explicit hash key order randomization is to be expected). As a consequence: 1) For biblio records added via acquisition (order from a new/empty record, order from a suggestion), freshly created MARC biblio records doesn't always have 260 $b and 260 $c stored in the proper order 2) Holdings data exported for zebra indexing as 952 fields may have subfields generated in more-or-less random order. While it probably (?) does not affect zebra indexing/searching in any significant way, end result is prone to be somehow ugly (which can be a potential issue e.g. for people running Z39.50 server) and is not guaranteed to be consistent; different records - or even different items in the same record, can have 952 subfields generated in indiscriminate order. This patch fixes abovementioned issues via introducting explicit sorting (by subfiled code/letter) for subfield pairs before they are added to the MARC record. To test: 1/ Try to confirm and reproduce both issues (use perl 5.18.1 if possible for more randomly ordered results). 2/ Apply patch. 3/ Redo the tests; ensure that both issues are now fixed and that there are no apparent regressions of any kind (especially regarding to 952 fields generated for zebra [re]indexing).
Created attachment 28719 [details] [review] Bug 12343 - TransformKohaToMarc() is adding MARC subfields in random order TransformKohaToMarc() function in C4/Biblio.pm iterates through it's argument - which is a hashref - using 'each'. Perl is not guaranteed to return hash keys in any particular order (not to mention that in more recent perl versions, explicit hash key order randomization is to be expected). As a consequence: 1) For biblio records added via acquisition (order from a new/empty record, order from a suggestion), freshly created MARC biblio records doesn't always have 260 $b and 260 $c stored in the proper order 2) Holdings data exported for zebra indexing as 952 fields may have subfields generated in more-or-less random order. While it probably (?) does not affect zebra indexing/searching in any significant way, end result is prone to be somehow ugly (which can be a potential issue e.g. for people running Z39.50 server) and is not guaranteed to be consistent; different records - or even different items in the same record, can have 952 subfields generated in indiscriminate order. This patch fixes abovementioned issues via introducting explicit sorting (by subfiled code/letter) for subfield pairs before they are added to the MARC record. To test: 1/ Try to confirm and reproduce both issues (use perl 5.18.1 if possible for more randomly ordered results). 2/ Apply patch. 3/ Redo the tests; ensure that both issues are now fixed and that there are no apparent regressions of any kind (especially regarding to 952 fields generated for zebra [re]indexing). Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment on attachment 28719 [details] [review] Bug 12343 - TransformKohaToMarc() is adding MARC subfields in random order Review of attachment 28719 [details] [review]: ----------------------------------------------------------------- ::: C4/Biblio.pm @@ +2194,4 @@ > while ( my ($name, $value) = each %$hash ) { > next unless my $dtm = $db_to_marc->{''}->{$name}; > next unless ( scalar( @$dtm ) ); > + my ($tag, $letter) = @$dtm; $tag .= ''; Please prefer 1 instruction by line. @@ +2198,2 @@ > foreach my $value ( split(/\s?\|\s?/, $value, -1) ) { > + $value eq '' && next; Please prefer next if $value eq ''; of if ( $value eq '' ) { next; }
Created attachment 28776 [details] [review] Bug 12343 - TransformKohaToMarc() is adding MARC subfields in random order TransformKohaToMarc() function in C4/Biblio.pm iterates through it's argument - which is a hashref - using 'each'. Perl is not guaranteed to return hash keys in any particular order (not to mention that in more recent perl versions, explicit hash key order randomization is to be expected). As a consequence: 1) For biblio records added via acquisition (order from a new/empty record, order from a suggestion), freshly created MARC biblio records doesn't always have 260 $b and 260 $c stored in the proper order 2) Holdings data exported for zebra indexing as 952 fields may have subfields generated in more-or-less random order. While it probably (?) does not affect zebra indexing/searching in any significant way, end result is prone to be somehow ugly (which can be a potential issue e.g. for people running Z39.50 server) and is not guaranteed to be consistent; different records - or even different items in the same record, can have 952 subfields generated in indiscriminate order. This patch fixes abovementioned issues via introducting explicit sorting (by subfiled code/letter) for subfield pairs before they are added to the MARC record. To test: 1/ Try to confirm and reproduce both issues (use perl 5.18.1 if possible for more randomly ordered results). 2/ Apply patch. 3/ Redo the tests; ensure that both issues are now fixed and that there are no apparent regressions of any kind (especially regarding to 952 fields generated for zebra [re]indexing). Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 28777 [details] [review] Bug 12343: Add regression test for TransformKohaToMarc Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 28778 [details] [review] Bug 12343: QA follow-up Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
One thing to note regarding subfield order: in the general case, one cannot count on alphabetical order being the correct order for subfields in a field. For example, the MARC21 246 field puts $i before $a: 246 0# $iAdded title page title on some issues:$aAnnual report However, in the specific case, since TransformKohaToMarc is used only for creating brief records in acquisitions and embedding item fields in the bib record, this isn't a blocker IMO, but should be noted if TransformKohaToMarc gets used more generally in the future.
Patches pushed to master. Thanks Jacek!
Pushed to 3.16.x for inclusion in 3.16.1.