From 93e2c6c77b26e69194bcf3c8188d881aa25d1c2d Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Sat, 28 Feb 2026 17:13:10 +0000 Subject: [PATCH] Bug 35104: Strip non-XML characters gracefully and notify user on save Wire the new Metadata::store graceful stripping through every path that saves a bibliographic record: * C4::Biblio::ModBiblioMarc collects stripping warnings via an optional warnings arrayref and pushes a 'nonxml_stripped' entry (using the new stripped_on_last_store() flag so pre-existing errors are not mistaken for a fresh stripping event). TransformHtmlToMarc no longer pre-empts the store-level check by stripping silently on its own. * cataloguing/addbiblio.pl handles the NONXML_STRIPPED warning: when stripping occurred the record is shown back in the editor together with a prominent "Record saved with modifications" notice so the cataloguer can review the result before continuing. * cataloguing/merge.pl propagates the same pattern for merge operations. * Koha::Import::Record::process() catches the Metadata::Invalid exception that is still thrown for truly unrecoverable MARCXML. * misc/migration_tools/bulkmarcimport.pl logs a warning when stripping occurs during a bulk import. Sponsored-by: OpenFifth --- C4/Biblio.pm | 25 ++++- Koha/Biblio/Metadata.pm | 58 +++++------- Koha/Import/Record.pm | 6 +- cataloguing/addbiblio.pl | 93 +++++++++++++------ cataloguing/merge.pl | 8 +- .../prog/en/modules/cataloguing/addbiblio.tt | 12 +++ .../prog/en/modules/cataloguing/merge.tt | 11 ++- misc/migration_tools/bulkmarcimport.pl | 16 ++++ 8 files changed, 159 insertions(+), 70 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index 60d56e6e937..a45ae587049 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -298,6 +298,7 @@ sub AddBiblio { { skip_record_index => 1, record_source_id => $options->{record_source_id}, + ( ref $options->{warnings} eq 'ARRAY' ? ( warnings => $options->{warnings} ) : () ), } ); @@ -394,7 +395,12 @@ sub ModBiblio { ( exists $options->{record_source_id} ) ? ( record_source_id => $options->{record_source_id} ) : () - ) + ), + ( + ( ref $options->{warnings} eq 'ARRAY' ) + ? ( warnings => $options->{warnings} ) + : () + ), }; if ( !$record ) { @@ -2344,7 +2350,6 @@ sub TransformHtmlToMarc { if ( $fval ne '' && $newfield ) { $newfield->add_subfields( $fkey => $fval ); } elsif ( $fval ne '' ) { - $fval = StripNonXmlChars($fval); #Strip out any non-XML characters like control characters $newfield = MARC::Field->new( $tag, $ind1, $ind2, $fkey => $fval ); } $j += 2; @@ -2998,6 +3003,22 @@ sub ModBiblioMarc { } )->store; + if ( ref $options->{warnings} eq 'ARRAY' ) { + my ($nonxml_msg) = grep { $_->message eq 'nonxml_stripped' } @{ $m_rs->object_messages }; + if ($nonxml_msg) { + my @occurrences = @{ $nonxml_msg->payload // [] }; + my $message = @occurrences + ? sprintf( + "%s: invalid char value %d (U+%04X) at position %d\n%s", + $occurrences[0]{field}, $occurrences[0]{char_ord}, + $occurrences[0]{char_ord}, $occurrences[0]{position}, + $occurrences[0]{snippet} + ) + : 'non-XML characters stripped'; + push @{ $options->{warnings} }, { type => 'nonxml_stripped', message => $message }; + } + } + unless ($skip_record_index) { my $indexer = Koha::SearchEngine::Indexer->new( { index => $Koha::SearchEngine::BIBLIOS_INDEX } ); $indexer->index_records( $biblionumber, "specialUpdate", "biblioserver" ); diff --git a/Koha/Biblio/Metadata.pm b/Koha/Biblio/Metadata.pm index b702171cfd2..15287878e40 100644 --- a/Koha/Biblio/Metadata.pm +++ b/Koha/Biblio/Metadata.pm @@ -64,7 +64,10 @@ I exception is thrown. sub store { my $self = shift; - my $nonxml_error_message; + # Reset messages so object_messages reflects only this store call + $self->{_messages} = []; + + my $nonxml_chars; # arrayref of bad-char occurrences when stripping was needed # Check marcxml will roundtrip if ( $self->format eq 'marcxml' ) { @@ -87,10 +90,10 @@ sub store { if ($stripped_marcxml) { # Stripping fixed it – enumerate every bad character for the error log - my @nonxml_chars = _find_nonxml_chars( $self->metadata ); + my @found = _find_nonxml_chars( $self->metadata ); my $field_context = do { my %seen; - join( ', ', grep { !$seen{$_}++ } map { $_->{field} } @nonxml_chars ) + join( ', ', grep { !$seen{$_}++ } map { $_->{field} } @found ) || 'unknown location'; }; @@ -101,7 +104,7 @@ sub store { ) ); $self->metadata($stripped_metadata); - $nonxml_error_message = \@nonxml_chars || $marcxml_error; + $nonxml_chars = \@found; } else { # Truly unrecoverable @@ -119,18 +122,13 @@ sub store { $self->SUPER::store; - # If this save triggered fresh stripping, add to any existing nonxml_stripped - # errors with the new set. If the save was clean (no stripping needed), leave - # any existing errors alone – they are review flags requiring explicit human - # resolution and should not be silently cleared by a routine re-save. - if ($nonxml_error_message) { - my @occurrences = - ref $nonxml_error_message eq 'ARRAY' - ? @{$nonxml_error_message} - : (); - - if (@occurrences) { - for my $occ (@occurrences) { + # If this save triggered fresh stripping, record each bad character as a DB error + # row and signal the event via object_messages. If the save was clean (no stripping + # needed), leave any existing DB error rows alone – they are review flags requiring + # explicit human resolution and should not be silently cleared by a routine re-save. + if ($nonxml_chars) { + if (@$nonxml_chars) { + for my $occ (@$nonxml_chars) { Koha::Biblio::Metadata::Error->new( { metadata_id => $self->id, @@ -145,36 +143,28 @@ sub store { } } else { - # Fallback: couldn't pinpoint individual chars – store the parser error + # Fallback: couldn't pinpoint individual chars – store a generic message Koha::Biblio::Metadata::Error->new( { metadata_id => $self->id, error_type => 'nonxml_stripped', - message => $nonxml_error_message, + message => 'non-XML characters stripped (details unavailable)', } )->store; } - } - $self->{_stripped_on_last_store} = $nonxml_error_message ? 1 : 0; + $self->add_message( + { + message => 'nonxml_stripped', + type => 'warning', + payload => $nonxml_chars, + } + ); + } return $self; } -=head3 stripped_on_last_store - - if ( $metadata->stripped_on_last_store ) { ... } - -Returns true if the most recent call to C triggered non-XML character -stripping. Returns false (including before C has ever been called). - -=cut - -sub stripped_on_last_store { - my $self = shift; - return $self->{_stripped_on_last_store} // 0; -} - =head3 metadata_errors my $errors = $metadata->metadata_errors; diff --git a/Koha/Import/Record.pm b/Koha/Import/Record.pm index 9b1e1c3f675..a492c5e0702 100644 --- a/Koha/Import/Record.pm +++ b/Koha/Import/Record.pm @@ -20,6 +20,7 @@ use Modern::Perl; use Carp; use MARC::Record; +use C4::Charset qw( StripNonXmlChars ); use C4::Context; use C4::Biblio qw(ModBiblio); use C4::AuthoritiesMarc qw(GuessAuthTypeCode ModAuthority); @@ -56,7 +57,10 @@ sub get_marc_record { $format = 'UNIMARCAUTH'; } - my $record = MARC::Record->new_from_xml( $self->marcxml, 'UTF-8', $format ); + my $record = eval { MARC::Record->new_from_xml( $self->marcxml, 'UTF-8', $format ) }; + unless ($record) { + $record = eval { MARC::Record->new_from_xml( StripNonXmlChars( $self->marcxml ), 'UTF-8', $format ) }; + } return $record; } diff --git a/cataloguing/addbiblio.pl b/cataloguing/addbiblio.pl index c9baa96b3ea..4bb600a029a 100755 --- a/cataloguing/addbiblio.pl +++ b/cataloguing/addbiblio.pl @@ -703,8 +703,9 @@ if ( $op eq "cud-addbiblio" ) { # it is not a duplicate (determined either by Koha itself or by user checking it's not a duplicate) if ( !$duplicatebiblionumber or $confirm_not_duplicate ) { my $oldbibitemnum; + my @save_warnings; my $encode_error = try { - if ( $is_a_modif ) { + if ($is_a_modif) { ModBiblio( $record, $biblionumber, @@ -714,11 +715,13 @@ if ( $op eq "cud-addbiblio" ) { source => $z3950 ? 'z3950' : 'intranet', categorycode => $logged_in_patron->categorycode, userid => $logged_in_patron->userid, - } + }, + warnings => \@save_warnings, } ); } else { - ( $biblionumber, $oldbibitemnum ) = AddBiblio( $record, $frameworkcode ); + ( $biblionumber, $oldbibitemnum ) = + AddBiblio( $record, $frameworkcode, { warnings => \@save_warnings } ); } return 0; } catch { @@ -730,41 +733,71 @@ if ( $op eq "cud-addbiblio" ) { } return 1; }; - if (!$encode_error && ($redirect eq "items" || ($mode ne "popup" && !$is_a_modif && $redirect ne "view" && $redirect ne "just_save"))){ - if ($frameworkcode eq 'FA'){ - print $input->redirect( - '/cgi-bin/koha/cataloguing/additem.pl?' - .'biblionumber='.$biblionumber - .'&frameworkcode='.$frameworkcode - .'&circborrowernumber='.$fa_circborrowernumber - .'&branch='.$fa_branch - .'&barcode='.uri_escape_utf8($fa_barcode) - .'&stickyduedate='.$fa_stickyduedate - .'&duedatespec='.$fa_duedatespec + + my ($nonxml_stripped) = grep { $_->{type} eq 'nonxml_stripped' } @save_warnings; + if ( $nonxml_stripped && !$encode_error ) { + $record = Koha::Biblios->find($biblionumber)->metadata->record; + build_tabs( $template, $record, $dbh, $encoding, $input ); + $template->param( + biblionumber => $biblionumber, + popup => $mode, + frameworkcode => $frameworkcode, + itemtype => $frameworkcode, + borrowernumber => $loggedinuser, + tab => scalar $input->param('tab'), + NONXML_STRIPPED => $nonxml_stripped->{message}, ); + output_html_with_http_headers $input, $cookie, $template->output; exit; - } - else { - print $input->redirect( + } + if ( + !$encode_error + && ( $redirect eq "items" + || ( $mode ne "popup" && !$is_a_modif && $redirect ne "view" && $redirect ne "just_save" ) ) + ) + { + if ( $frameworkcode eq 'FA' ) { + print $input->redirect( '/cgi-bin/koha/cataloguing/additem.pl?' + . 'biblionumber=' + . $biblionumber + . '&frameworkcode=' + . $frameworkcode + . '&circborrowernumber=' + . $fa_circborrowernumber + . '&branch=' + . $fa_branch + . '&barcode=' + . uri_escape_utf8($fa_barcode) + . '&stickyduedate=' + . $fa_stickyduedate + . '&duedatespec=' + . $fa_duedatespec ); + exit; + } else { + print $input->redirect( "/cgi-bin/koha/cataloguing/additem.pl?biblionumber=$biblionumber&frameworkcode=$frameworkcode&searchid=$searchid" - ); - exit; + ); + exit; } - } elsif(!$encode_error && (($is_a_modif || $redirect eq "view") && $redirect ne "just_save")){ + } elsif ( !$encode_error && ( ( $is_a_modif || $redirect eq "view" ) && $redirect ne "just_save" ) ) { my $defaultview = C4::Context->preference('IntranetBiblioDefaultView'); - my $views = { C4::Search::enabled_staff_search_views }; - if ($defaultview eq 'isbd' && $views->{can_view_ISBD}) { - print $input->redirect("/cgi-bin/koha/catalogue/ISBDdetail.pl?biblionumber=$biblionumber&searchid=$searchid"); - } elsif ($defaultview eq 'marc' && $views->{can_view_MARC}) { - print $input->redirect("/cgi-bin/koha/catalogue/MARCdetail.pl?biblionumber=$biblionumber&frameworkcode=$frameworkcode&searchid=$searchid"); - } elsif ($defaultview eq 'labeled_marc' && $views->{can_view_labeledMARC}) { - print $input->redirect("/cgi-bin/koha/catalogue/labeledMARCdetail.pl?biblionumber=$biblionumber&searchid=$searchid"); + my $views = {C4::Search::enabled_staff_search_views}; + if ( $defaultview eq 'isbd' && $views->{can_view_ISBD} ) { + print $input->redirect( + "/cgi-bin/koha/catalogue/ISBDdetail.pl?biblionumber=$biblionumber&searchid=$searchid"); + } elsif ( $defaultview eq 'marc' && $views->{can_view_MARC} ) { + print $input->redirect( + "/cgi-bin/koha/catalogue/MARCdetail.pl?biblionumber=$biblionumber&frameworkcode=$frameworkcode&searchid=$searchid" + ); + } elsif ( $defaultview eq 'labeled_marc' && $views->{can_view_labeledMARC} ) { + print $input->redirect( + "/cgi-bin/koha/catalogue/labeledMARCdetail.pl?biblionumber=$biblionumber&searchid=$searchid"); } else { - print $input->redirect("/cgi-bin/koha/catalogue/detail.pl?biblionumber=$biblionumber&searchid=$searchid"); + print $input->redirect( + "/cgi-bin/koha/catalogue/detail.pl?biblionumber=$biblionumber&searchid=$searchid"); } exit; - } - elsif ( !$encode_error && ( $redirect eq "just_save" ) ) { + } elsif ( !$encode_error && ( $redirect eq "just_save" ) ) { my $tab = $input->param('current_tab'); print $input->redirect( "/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=$biblionumber&framework=$frameworkcode&tab=$tab&searchid=$searchid" diff --git a/cataloguing/merge.pl b/cataloguing/merge.pl index a48f4bbe0f1..ba2fe290d32 100755 --- a/cataloguing/merge.pl +++ b/cataloguing/merge.pl @@ -92,8 +92,9 @@ if ( $op eq 'cud-merge' ) { # Modifying the reference record my $invalid_metadata; + my @merge_warnings; my $modded = try { - ModBiblio($record, $ref_biblionumber, $frameworkcode); + ModBiblio( $record, $ref_biblionumber, $frameworkcode, { warnings => \@merge_warnings } ); return 1; } catch { my $exception = $_; @@ -105,6 +106,8 @@ if ( $op eq 'cud-merge' ) { return 0; }; + my ($nonxml_stripped) = grep { $_->{type} eq 'nonxml_stripped' } @merge_warnings; + my $report_header = {}; if ($modded) { foreach my $biblionumber ( $ref_biblionumber, @biblionumbers ) { @@ -159,7 +162,8 @@ if ( $op eq 'cud-merge' ) { result => 1, report_records => \@report_records, report_header => $report_header, - ref_biblionumber => scalar $input->param('ref_biblionumber') + ref_biblionumber => scalar $input->param('ref_biblionumber'), + ( $nonxml_stripped ? ( NONXML_STRIPPED => $nonxml_stripped->{message} ) : () ), ); #------------------------- diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt index 809cc3abfa4..46d4827e9a8 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt @@ -805,6 +805,18 @@
[% INVALID_METADATA | html %]
[% END %] + [% IF ( NONXML_STRIPPED ) %] +
+

Record saved with modifications

+

Non-XML characters were automatically stripped from this record before saving. The record has been saved successfully, but please review it to ensure the data is correct. If data appears to be missing or incorrect, edit + and re-save the record.

+
[% NONXML_STRIPPED | html %]
+
+ [% END %] [% IF marc21_fixlen %]

WARNING

The following fixed-length control field(s) have an invalid length: [% marc21_fixlen.failed.join(',') | html %]. Please fix.

diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt index a7d22b8be6e..a567e2c42b5 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt @@ -62,7 +62,7 @@ [% IF error.code == 'CANNOT_MOVE' %] The following items could not be moved from the old record to the new one: [% error.value | html %] [% ELSIF error.code == 'INVALID_METADATA' %] -

This record had encoding issues, non-xml characters have been stripped in order to allow editing. Please be cautious when saving that some data may have been removed from the record.

+

The merged record could not be saved because it contains characters that are not valid XML. The merge has been aborted.

[% error.value | html %]
[% ELSE %] [% error | html %] @@ -73,6 +73,15 @@ [% END %] [% ELSE %]
The merge was successful. View the merged record.
+ [% IF NONXML_STRIPPED %] +
+ Non-XML characters were automatically stripped from the merged record before saving. Please review the record to ensure + the data is correct. +
[% NONXML_STRIPPED | html %]
+
+ [% END %]

Report

diff --git a/misc/migration_tools/bulkmarcimport.pl b/misc/migration_tools/bulkmarcimport.pl index 9ff0935d72f..d764bdeea26 100755 --- a/misc/migration_tools/bulkmarcimport.pl +++ b/misc/migration_tools/bulkmarcimport.pl @@ -565,6 +565,9 @@ RECORD: while () { } # Create biblio, unless we already have it (either match or ISBN) + my @import_warnings; + $mod_biblio_options->{warnings} = \@import_warnings; + $add_biblio_options->{warnings} = \@import_warnings; if ($matched_record_id) { eval { $biblioitemnumber = Koha::Biblios->find($matched_record_id)->biblioitem->biblioitemnumber; }; if ($update) { @@ -625,6 +628,19 @@ RECORD: while () { ) if ($logfile); next RECORD; } + for my $w ( grep { $_->{type} eq 'nonxml_stripped' } @import_warnings ) { + warn sprintf( + "WARNING: Non-XML characters stripped from biblio %s: %s", + $record_id // $originalid, $w->{message} + ); + printlog( + { + id => $record_id // $originalid, + op => "import", + status => "warning : non-XML characters stripped" + } + ) if ($logfile); + } my $record_has_added_items = 0; if ($record_id) { $yamlhash->{$originalid} = $record_id if $yamlfile; -- 2.53.0