Bugzilla – Attachment 194222 Details for
Bug 35104
We should warn when attempting to save MARC records that contain characters invalid in XML
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 35104: Strip non-XML characters gracefully and notify user on save
93e2c6c.patch (text/plain), 21.70 KB, created by
Martin Renvoize (ashimema)
on 2026-03-01 16:23:49 UTC
(
hide
)
Description:
Bug 35104: Strip non-XML characters gracefully and notify user on save
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2026-03-01 16:23:49 UTC
Size:
21.70 KB
patch
obsolete
>From 93e2c6c77b26e69194bcf3c8188d881aa25d1c2d Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >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<Koha::Exceptions::Metadata::Invalid> 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<store> triggered non-XML character >-stripping. Returns false (including before C<store> 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 @@ > <pre class="problem">[% INVALID_METADATA | html %]</pre> > </div> > [% END %] >+ [% IF ( NONXML_STRIPPED ) %] >+ <div class="alert alert-warning"> >+ <h1>Record saved with modifications</h1> >+ <p >+ ><strong >+ >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.</strong >+ ></p >+ > >+ <pre class="problem">[% NONXML_STRIPPED | html %]</pre> >+ </div> >+ [% END %] > > [% IF marc21_fixlen %] > <div class="alert alert-warning"><h1>WARNING</h1><p>The following fixed-length control field(s) have an invalid length: [% marc21_fixlen.failed.join(',') | html %]. Please fix.</p></div> >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' %] >- <p>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.</p> >+ <p>The merged record could not be saved because it contains characters that are not valid XML. The merge has been aborted.</p> > <pre class="problem">[% error.value | html %]</pre> > [% ELSE %] > [% error | html %] >@@ -73,6 +73,15 @@ > [% END %] > [% ELSE %] > <div class="alert alert-info">The merge was successful. <a href="/cgi-bin/koha/catalogue/MARCdetail.pl?biblionumber=[% ref_biblionumber | uri %]">View the merged record.</a></div> >+ [% IF NONXML_STRIPPED %] >+ <div class="alert alert-warning"> >+ <strong >+ >Non-XML characters were automatically stripped from the merged record before saving. Please <a href="/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=[% ref_biblionumber | uri %]">review the record</a> to ensure >+ the data is correct.</strong >+ > >+ <pre class="problem">[% NONXML_STRIPPED | html %]</pre> >+ </div> >+ [% END %] > <div class="page-section"> > <h3>Report</h3> > <table> >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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 35104
:
157625
|
157627
|
157628
|
157642
|
159615
|
159620
|
159621
|
159630
|
159810
|
159811
|
159812
|
159813
|
166431
|
173145
|
173146
|
176820
|
176821
|
176850
|
176851
|
176852
|
176860
|
193985
|
193986
|
193987
|
193988
|
193989
|
193990
|
193991
|
193992
|
193993
|
193994
|
193995
|
193996
|
193997
|
193998
|
193999
|
194000
|
194001
|
194004
|
194005
|
194006
|
194007
|
194008
|
194009
|
194010
|
194011
|
194012
|
194013
|
194152
|
194153
|
194154
|
194155
|
194156
|
194157
|
194158
|
194159
|
194160
|
194161
|
194162
|
194163
|
194164
|
194165
|
194166
|
194167
|
194168
|
194169
|
194170
|
194171
|
194172
|
194213
|
194214
|
194215
|
194216
|
194217
|
194218
|
194219
|
194220
|
194221
| 194222 |
194223
|
194224
|
194225
|
194226
|
194227