Bug 12343

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.

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.