Bugzilla – Attachment 194166 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: Record each stripped character as its own error with field and position context
5e5891c.patch (text/plain), 12.50 KB, created by
Martin Renvoize (ashimema)
on 2026-02-27 19:24:12 UTC
(
hide
)
Description:
Bug 35104: Record each stripped character as its own error with field and position context
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2026-02-27 19:24:12 UTC
Size:
12.50 KB
patch
obsolete
>From 5e5891c73a1d9cca0885620e7c4ca15501203923 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >Date: Fri, 27 Feb 2026 13:36:31 +0000 >Subject: [PATCH] Bug 35104: Record each stripped character as its own error > with field and position context > >Previously a single error row was written with a deduplicated list of >affected field references. This was an improvement over the raw XML parser >error (which only reported the first bad character), but still lost the >per-character detail the parser provided. > >Replace _nonxml_affected_fields() with _find_nonxml_chars(), which scans >every datafield subfield and control field in the original MARC XML and >returns one hashref per bad-character occurrence, carrying: > - field: human-readable field reference (e.g. "245$a") > - char_ord: decimal ordinal of the illegal character > - position: 1-based character offset within the field value > >store() now writes one biblio_metadata_errors row per occurrence with a >message in the form: > > 245$a: invalid char value 31 (U+001F) at position 10 > >This combines field/subfield identification (introduced in the previous >commit) with the character value and in-field location that the XML parser >previously reported only for the first bad character. > >The logger warn retains the original XML parser error string alongside the >deduplicated field list, so operators still see the full technical detail. >A fallback to storing the raw parser error is kept for the unlikely case >where the regex scan finds no matches despite stripping being needed. > >catalogue/detail.pl and catalogue/MARCdetail.pl now join all per-character >messages with newlines so the existing <pre> alert block shows all errors. > >Sponsored-by: OpenFifth >--- > Koha/Biblio/Metadata.pm | 108 ++++++++++++++++++-------- > catalogue/MARCdetail.pl | 7 +- > catalogue/detail.pl | 7 +- > t/db_dependent/Biblio.t | 15 ++-- > t/db_dependent/Koha/Biblio/Metadata.t | 10 +-- > 5 files changed, 97 insertions(+), 50 deletions(-) > >diff --git a/Koha/Biblio/Metadata.pm b/Koha/Biblio/Metadata.pm >index f72c8f47c42..a7a202fb9cc 100644 >--- a/Koha/Biblio/Metadata.pm >+++ b/Koha/Biblio/Metadata.pm >@@ -84,12 +84,13 @@ sub store { > > if ($stripped_marcxml) { > >- # Stripping fixed it â save the clean version and record all affected fields >- my @affected = _nonxml_affected_fields( $self->metadata ); >- my $field_context = >- @affected >- ? join( ', ', @affected ) >- : 'unknown location'; >+ # Stripping fixed it â enumerate every bad character for the error log >+ my @nonxml_chars = _find_nonxml_chars( $self->metadata ); >+ my $field_context = do { >+ my %seen; >+ join( ', ', grep { !$seen{$_}++ } map { $_->{field} } @nonxml_chars ) >+ || 'unknown location'; >+ }; > > Koha::Logger->get->warn( > sprintf( >@@ -98,7 +99,7 @@ sub store { > ) > ); > $self->metadata($stripped_metadata); >- $nonxml_error_message = "Non-XML characters stripped from: $field_context"; >+ $nonxml_error_message = \@nonxml_chars || $marcxml_error; > } else { > > # Truly unrecoverable >@@ -119,13 +120,35 @@ sub store { > # Sync nonxml_stripped error rows: clear any existing, then re-add if needed > $self->metadata_errors->search( { error_type => 'nonxml_stripped' } )->delete; > if ($nonxml_error_message) { >- Koha::Biblio::Metadata::Error->new( >- { >- metadata_id => $self->id, >- error_type => 'nonxml_stripped', >- message => $nonxml_error_message, >+ my @occurrences = >+ ref $nonxml_error_message eq 'ARRAY' >+ ? @{$nonxml_error_message} >+ : (); >+ >+ if (@occurrences) { >+ for my $occ (@occurrences) { >+ Koha::Biblio::Metadata::Error->new( >+ { >+ metadata_id => $self->id, >+ error_type => 'nonxml_stripped', >+ message => sprintf( >+ "%s: invalid char value %d (U+%04X) at position %d", >+ $occ->{field}, $occ->{char_ord}, $occ->{char_ord}, $occ->{position} >+ ), >+ } >+ )->store; > } >- )->store; >+ } else { >+ >+ # Fallback: couldn't pinpoint individual chars â store the parser error >+ Koha::Biblio::Metadata::Error->new( >+ { >+ metadata_id => $self->id, >+ error_type => 'nonxml_stripped', >+ message => $nonxml_error_message, >+ } >+ )->store; >+ } > } > > return $self; >@@ -360,50 +383,67 @@ sub _embed_items { > return $record; > } > >-=head3 _nonxml_affected_fields >+=head3 _find_nonxml_chars >+ >+ my @occurrences = _find_nonxml_chars( $marcxml_string ); >+ >+Scans a raw MARC XML string for every individual character that is illegal >+in XML 1.0. Returns a list of hashrefs (one per character occurrence) with >+the following keys: >+ >+=over 4 >+ >+=item * C<field> - human-readable field reference, e.g. C<245$a> or C<001> >+ >+=item * C<char_ord> - ordinal (decimal) value of the bad character > >- my @fields = _nonxml_affected_fields( $marcxml_string ); >+=item * C<position> - 1-based character offset within the field value > >-Scans a raw MARC XML string for every field and subfield that contains at >-least one character illegal in XML 1.0. Returns a deduplicated list of >-human-readable references such as C<245$a> (for subfields) or C<001> (for >-control fields), in document order. >+=back > >-The same character class is used as L<C4::Charset/StripNonXmlChars>, so the >-set of affected fields reported here exactly matches what would be stripped. >+The same character class is used as L<C4::Charset/StripNonXmlChars>, so >+every occurrence returned here is exactly one that would be stripped. > > =cut > >-sub _nonxml_affected_fields { >+sub _find_nonxml_chars { > my ($marcxml) = @_; > > # Characters illegal in XML 1.0 â identical to the set stripped by StripNonXmlChars > my $non_xml_re = qr/[^\x09\x0A\x0D\x{0020}-\x{D7FF}\x{E000}-\x{FFFD}\x{10000}-\x{10FFFF}]/; > >- my @affected; >- my %seen; >+ my @occurrences; >+ >+ my $scan = sub { >+ my ( $field_ref, $val ) = @_; >+ my $pos = 1; >+ for my $char ( split //, $val ) { >+ if ( $char =~ $non_xml_re ) { >+ push @occurrences, >+ { >+ field => $field_ref, >+ char_ord => ord($char), >+ position => $pos, >+ }; >+ } >+ $pos++; >+ } >+ }; > > # Scan every subfield inside every datafield > while ( $marcxml =~ m{<datafield\b[^>]*\btag="(\w+)"[^>]*>(.*?)</datafield>}gs ) { > my ( $tag, $content ) = ( $1, $2 ); > while ( $content =~ m{<subfield\b[^>]*\bcode="(\w)"[^>]*>(.*?)</subfield>}gs ) { >- my ( $code, $val ) = ( $1, $2 ); >- my $ref = "$tag\$$code"; >- if ( $val =~ $non_xml_re && !$seen{$ref}++ ) { >- push @affected, $ref; >- } >+ $scan->( "$tag\$$1", $2 ); > } > } > > # Scan control fields > while ( $marcxml =~ m{<controlfield\b[^>]*\btag="(\w+)"[^>]*>(.*?)</controlfield>}gs ) { >- my ( $tag, $val ) = ( $1, $2 ); >- if ( $val =~ $non_xml_re && !$seen{$tag}++ ) { >- push @affected, $tag; >- } >+ $scan->( $1, $2 ); > } > >- return @affected; >+ return @occurrences; > } > > =head3 _type >diff --git a/catalogue/MARCdetail.pl b/catalogue/MARCdetail.pl >index 51538494625..dbc3b42203a 100755 >--- a/catalogue/MARCdetail.pl >+++ b/catalogue/MARCdetail.pl >@@ -121,8 +121,11 @@ if ( $query->cookie("searchToOrder") ) { > } > > $template->param( ocoins => $biblio_object->get_coins ); >-my $nonxml_error = $biblio_object->metadata->metadata_errors->search( { error_type => 'nonxml_stripped' } )->next; >-$template->param( nonxml_stripped => $nonxml_error ? $nonxml_error->message : undef ); >+my $nonxml_errors = $biblio_object->metadata->metadata_errors->search( { error_type => 'nonxml_stripped' } ); >+if ( $nonxml_errors->count ) { >+ my @messages = map { $_->message } $nonxml_errors->as_list; >+ $template->param( nonxml_stripped => join( "\n", @messages ) ); >+} > > #count of item linked > my $itemcount = $biblio_object->items->count; >diff --git a/catalogue/detail.pl b/catalogue/detail.pl >index f14c9c3fb0d..b74fcb382a7 100755 >--- a/catalogue/detail.pl >+++ b/catalogue/detail.pl >@@ -177,8 +177,11 @@ my $marcflavour = C4::Context->preference("marcflavour"); > $template->param( 'SpineLabelShowPrintOnBibDetails' => C4::Context->preference("SpineLabelShowPrintOnBibDetails") ); > > $template->param( ocoins => !$invalid_marc_record ? $biblio->get_coins : undef ); >-my $nonxml_error = $biblio->metadata->metadata_errors->search( { error_type => 'nonxml_stripped' } )->next; >-$template->param( nonxml_stripped => $nonxml_error ? $nonxml_error->message : undef ); >+my $nonxml_errors = $biblio->metadata->metadata_errors->search( { error_type => 'nonxml_stripped' } ); >+if ( $nonxml_errors->count ) { >+ my @messages = map { $_->message } $nonxml_errors->as_list; >+ $template->param( nonxml_stripped => join( "\n", @messages ) ); >+} > > # some useful variables for enhanced content; > # in each case, we're grabbing the first value we find in >diff --git a/t/db_dependent/Biblio.t b/t/db_dependent/Biblio.t >index 5fc533b172e..5de58268210 100755 >--- a/t/db_dependent/Biblio.t >+++ b/t/db_dependent/Biblio.t >@@ -1205,7 +1205,7 @@ subtest 'GetFrameworkCode' => sub { > }; > > subtest 'ModBiblio on record with strippable non-XML characters' => sub { >- plan tests => 3; >+ plan tests => 4; > > t::lib::Mocks::mock_preference( "CataloguingLog", 1 ); > >@@ -1221,15 +1221,16 @@ subtest 'ModBiblio on record with strippable non-XML characters' => sub { > my @warnings; > C4::Biblio::ModBiblio( $record, $biblionumber, '', { warnings => \@warnings } ); > like( >- $warnings[0]{message}, qr/Non-XML characters stripped from: /, >- 'Non-XML character stripping reported via warnings arrayref' >+ $warnings[0]{message}, qr/650\$a: invalid char value 31 \(U\+001F\) at position \d+/, >+ 'Non-XML character stripping reported via warnings arrayref with field and char context' > ); > >- my $metadata = Koha::Biblios->find($biblionumber)->metadata; >- my $nonxml_error = $metadata->metadata_errors->search( { error_type => 'nonxml_stripped' } )->next; >+ my $metadata = Koha::Biblios->find($biblionumber)->metadata; >+ my @nonxml_errors = $metadata->metadata_errors->search( { error_type => 'nonxml_stripped' } )->as_list; >+ ok( scalar @nonxml_errors, 'nonxml_stripped errors persisted in biblio_metadata_errors after ModBiblio' ); > like( >- $nonxml_error->message, qr/Non-XML characters stripped from: /, >- 'nonxml_stripped error message persisted in biblio_metadata_errors after ModBiblio' >+ $nonxml_errors[0]->message, qr/650\$a: invalid char value 31 \(U\+001F\) at position \d+/, >+ 'error message identifies field, char value, and position' > ); > > my $action_logs = >diff --git a/t/db_dependent/Koha/Biblio/Metadata.t b/t/db_dependent/Koha/Biblio/Metadata.t >index 96b26fe02ab..7b0a6af48a8 100755 >--- a/t/db_dependent/Koha/Biblio/Metadata.t >+++ b/t/db_dependent/Koha/Biblio/Metadata.t >@@ -107,12 +107,12 @@ EOX > > lives_ok { $record->store } 'MARCXML with strippable characters stores successfully'; > >- my $error = $record->metadata_errors->search( { error_type => 'nonxml_stripped' } )->next; >- ok( $error, 'nonxml_stripped error recorded after stripping' ); >+ my @errors = $record->metadata_errors->search( { error_type => 'nonxml_stripped' } )->as_list; >+ ok( scalar @errors, 'nonxml_stripped error(s) recorded after stripping' ); > like( >- $error->message, >- qr/245\$a/, >- 'error message identifies the affected MARC field and subfield' >+ $errors[0]->message, >+ qr/245\$a: invalid char value 31 \(U\+001F\) at position \d+/, >+ 'error message identifies field, char value, and position' > ); > > my $stored = Koha::Biblio::Metadatas->find( { biblionumber => $biblio->{biblionumber}, format => 'marcxml' } ); >-- >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