Bugzilla – Attachment 194008 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
9fd46df.patch (text/plain), 20.13 KB, created by
Martin Renvoize (ashimema)
on 2026-02-26 15:27:55 UTC
(
hide
)
Description:
Bug 35104: Strip non-XML characters gracefully and notify user
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2026-02-26 15:27:55 UTC
Size:
20.13 KB
patch
obsolete
>From 9fd46df3b08645358b2b64e3814f6813fb06be74 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >Date: Thu, 26 Feb 2026 10:23:19 +0000 >Subject: [PATCH] Bug 35104: Strip non-XML characters gracefully and notify > user > >Instead of always throwing when MARCXML contains invalid characters, >attempt recovery via StripNonXmlChars first. If stripping produces a >parseable record, save the clean version and set stripped_nonxml on the >object; only throw Koha::Exceptions::Metadata::Invalid when the record >is truly unrecoverable. > >Propagate the strip notification through ModBiblioMarc -> ModBiblio / >AddBiblio via an optional warnings arrayref in $options. Entry points >(addbiblio.pl, merge.pl) collect warnings and display a prominent alert >asking the user to review the saved record. > >Update unit tests: replace the embedded-ESC 'Invalid MARCXML handling' >subtest with a chr(31)-based 'MARCXML with strippable non-XML characters' >subtest and add a stripped_nonxml=undef assertion for clean records. > >Sponsored-by: OpenFifth >--- > C4/Biblio.pm | 13 ++- > Koha/Biblio/Metadata.pm | 60 ++++++++++-- > cataloguing/addbiblio.pl | 93 +++++++++++++------ > cataloguing/merge.pl | 8 +- > .../prog/en/modules/cataloguing/addbiblio.tt | 12 +++ > .../prog/en/modules/cataloguing/merge.tt | 11 ++- > t/db_dependent/Koha/Biblio/Metadata.t | 46 +++++---- > 7 files changed, 181 insertions(+), 62 deletions(-) > >diff --git a/C4/Biblio.pm b/C4/Biblio.pm >index 60d56e6e937..629ed885c4a 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 ) { >@@ -2998,6 +3004,11 @@ sub ModBiblioMarc { > } > )->store; > >+ if ( $m_rs->stripped_nonxml && ref $options->{warnings} eq 'ARRAY' ) { >+ push @{ $options->{warnings} }, >+ { type => 'nonxml_stripped', message => $m_rs->stripped_nonxml }; >+ } >+ > 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 926f5d85bc5..2dca4d7641c 100644 >--- a/Koha/Biblio/Metadata.pm >+++ b/Koha/Biblio/Metadata.pm >@@ -26,6 +26,7 @@ use C4::Items qw( GetMarcItem ); > > use Koha::Database; > use Koha::Exceptions::Metadata; >+use Koha::Logger; > use Koha::RecordSources; > > use base qw(Koha::Object); >@@ -45,6 +46,13 @@ Koha::Metadata - Koha Metadata Object class > Metadata specific store method to catch errant characters prior > to committing to the database. > >+If the MARCXML cannot be parsed but can be recovered by stripping non-XML >+characters (C<StripNonXmlChars>), the stripped version is saved and >+C<stripped_nonxml> is set on the object so callers can notify the user. >+ >+If the MARCXML cannot be recovered at all, a >+I<Koha::Exceptions::Metadata::Invalid> exception is thrown. >+ > =cut > > sub store { >@@ -62,20 +70,56 @@ sub store { > my $marcxml_error = $@; > chomp $marcxml_error; > unless ($marcxml) { >- warn $marcxml_error; >- Koha::Exceptions::Metadata::Invalid->throw( >- id => $self->id, >- biblionumber => $self->biblionumber, >- format => $self->format, >- schema => $self->schema, >- decoding_error => $marcxml_error, >- ); >+ >+ # Attempt recovery by stripping non-XML characters >+ my $stripped_metadata = StripNonXmlChars( $self->metadata ); >+ my $stripped_marcxml = eval { MARC::Record::new_from_xml( $stripped_metadata, 'UTF-8', $self->schema ); }; >+ >+ if ($stripped_marcxml) { >+ >+ # Stripping fixed it รข save the clean version and record what happened >+ Koha::Logger->get->warn( >+ sprintf( >+ "Non-XML characters stripped from bibliographic record (biblionumber=%s): %s", >+ $self->biblionumber // 'N/A', $marcxml_error >+ ) >+ ); >+ $self->metadata($stripped_metadata); >+ $self->{_stripped_nonxml} = $marcxml_error; >+ } else { >+ >+ # Truly unrecoverable >+ Koha::Logger->get->warn($marcxml_error); >+ Koha::Exceptions::Metadata::Invalid->throw( >+ id => $self->id, >+ biblionumber => $self->biblionumber, >+ format => $self->format, >+ schema => $self->schema, >+ decoding_error => $marcxml_error, >+ ); >+ } > } > } > > return $self->SUPER::store; > } > >+=head3 stripped_nonxml >+ >+ if ( my $error = $metadata->stripped_nonxml ) { >+ # warn user that non-XML characters were removed on last store() >+ } >+ >+Returns the original decode error string if non-XML characters were stripped >+during the most recent call to C<store()>, or C<undef> otherwise. >+ >+=cut >+ >+sub stripped_nonxml { >+ my $self = shift; >+ return $self->{_stripped_nonxml}; >+} >+ > =head3 record > > my $record = $metadata->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/t/db_dependent/Koha/Biblio/Metadata.t b/t/db_dependent/Koha/Biblio/Metadata.t >index 6e5af530cb7..ad3138df690 100755 >--- a/t/db_dependent/Koha/Biblio/Metadata.t >+++ b/t/db_dependent/Koha/Biblio/Metadata.t >@@ -18,7 +18,7 @@ > use Modern::Perl; > > use Test::NoWarnings; >-use Test::More tests => 6; >+use Test::More tests => 7; > use Test::Exception; > use Test::MockModule; > use Test::Warn; >@@ -28,6 +28,8 @@ use t::lib::Mocks; > > use C4::Biblio qw( AddBiblio ); > use Koha::Database; >+use MARC::Record; >+use MARC::Field; > > BEGIN { > use_ok('Koha::Biblio::Metadatas'); >@@ -73,40 +75,44 @@ EOX > > lives_ok { $record->store } > 'Valid MARCXML record stores successfully'; >+ is( $record->stripped_nonxml, undef, 'stripped_nonxml is undef for clean record' ); > > $schema->storage->txn_rollback; > }; > >- subtest 'Invalid MARCXML handling' => sub { >+ subtest 'MARCXML with strippable non-XML characters' => sub { >+ plan tests => 3; > $schema->storage->txn_begin; >- my $invalid_marcxml = <<'EOX'; >-<?xml version="1.0" encoding="UTF-8"?> >-<record xmlns="http://www.loc.gov/MARC21/slim"> >- <leader>00925nam a22002411a 4500</leader> >- <controlfield tag="001">1234567</controlfield> >- <datafield tag="245" ind1="1" ind2="0"> >- <subfield code="a">This string will break your record</subfield> >- </datafield> >- <!-- Invalid XML structure --> >-</record> >-EOX >+ >+ # Build MARCXML containing a non-XML control character (chr(31) = Unit Separator) >+ my $bad_title = 'Title with' . chr(31) . ' bad character'; >+ my $clean_title = 'Title with bad character'; >+ >+ # Generate the MARCXML string with the embedded bad character >+ my $marc_record = MARC::Record->new; >+ $marc_record->append_fields( MARC::Field->new( '245', '', '', 'a' => $bad_title ) ); >+ my $marcxml_with_bad_char = $marc_record->as_xml_record('MARC21'); > > my $record = Koha::Biblio::Metadata->new( > { > format => 'marcxml', >- metadata => $invalid_marcxml, >+ metadata => $marcxml_with_bad_char, > biblionumber => $biblio->{biblionumber}, > schema => 'MARC21', > } > ); > >- throws_ok { $record->store } >- 'Koha::Exceptions::Metadata::Invalid', >- 'Invalid MARCXML throws expected exception'; >+ lives_ok { $record->store } 'MARCXML with strippable characters stores successfully'; >+ >+ ok( $record->stripped_nonxml, 'stripped_nonxml is set after stripping' ); >+ >+ my $stored = Koha::Biblio::Metadatas->find( { biblionumber => $biblio->{biblionumber}, format => 'marcxml' } ); >+ is( >+ $stored->record->field('245')->subfield('a'), >+ $clean_title, >+ 'Stored record has control character removed' >+ ); > >- my $thrown = $@; >- ok( $thrown->decoding_error, 'Exception contains decoding error message' ); >- is( $thrown->biblionumber, $biblio->{biblionumber}, 'Exception contains correct biblionumber' ); > $schema->storage->txn_rollback; > }; > >-- >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