From 10a9579dbb2a842b97091324d70fcb44a0e7d1c7 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Fri, 27 Feb 2026 13:25:09 +0000 Subject: [PATCH] Bug 35104: Improve nonxml_stripped error context to cover all affected fields The previous implementation stored only the first XML parser error message (e.g. "PCDATA invalid Char value 31 at line X, column Y"), which had two problems: - The XML parser stops at the first bad character, so records with multiple affected fields only reported one - The message gave no indication of which MARC field/subfield to fix Add _nonxml_affected_fields(), which scans the raw MARC XML string for every datafield subfield and control field that contains at least one character illegal in XML 1.0 (the same set stripped by StripNonXmlChars), and returns a deduplicated list of field references (e.g. ["245$a", "520$b"]). The stored error message now reads "Non-XML characters stripped from: 245$a" so cataloguers can immediately see which fields need attention, while the logger warn includes the field context alongside the original XML error for technical debugging. Sponsored-by: OpenFifth --- Koha/Biblio/Metadata.pm | 60 +++++++++++++++++++++++++-- t/db_dependent/Biblio.t | 6 +-- t/db_dependent/Koha/Biblio/Metadata.t | 11 +++-- 3 files changed, 66 insertions(+), 11 deletions(-) diff --git a/Koha/Biblio/Metadata.pm b/Koha/Biblio/Metadata.pm index bce132d00c5..f72c8f47c42 100644 --- a/Koha/Biblio/Metadata.pm +++ b/Koha/Biblio/Metadata.pm @@ -84,15 +84,21 @@ sub store { if ($stripped_marcxml) { - # Stripping fixed it – save the clean version + # 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'; + Koha::Logger->get->warn( sprintf( - "Non-XML characters stripped from bibliographic record (biblionumber=%s): %s", - $self->biblionumber // 'N/A', $marcxml_error + "Non-XML characters stripped from bibliographic record (biblionumber=%s) in %s: %s", + $self->biblionumber // 'N/A', $field_context, $marcxml_error ) ); $self->metadata($stripped_metadata); - $nonxml_error_message = $marcxml_error; + $nonxml_error_message = "Non-XML characters stripped from: $field_context"; } else { # Truly unrecoverable @@ -354,6 +360,52 @@ sub _embed_items { return $record; } +=head3 _nonxml_affected_fields + + my @fields = _nonxml_affected_fields( $marcxml_string ); + +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. + +The same character class is used as L, so the +set of affected fields reported here exactly matches what would be stripped. + +=cut + +sub _nonxml_affected_fields { + 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; + + # Scan every subfield inside every datafield + while ( $marcxml =~ m{]*\btag="(\w+)"[^>]*>(.*?)}gs ) { + my ( $tag, $content ) = ( $1, $2 ); + while ( $content =~ m{]*\bcode="(\w)"[^>]*>(.*?)}gs ) { + my ( $code, $val ) = ( $1, $2 ); + my $ref = "$tag\$$code"; + if ( $val =~ $non_xml_re && !$seen{$ref}++ ) { + push @affected, $ref; + } + } + } + + # Scan control fields + while ( $marcxml =~ m{]*\btag="(\w+)"[^>]*>(.*?)}gs ) { + my ( $tag, $val ) = ( $1, $2 ); + if ( $val =~ $non_xml_re && !$seen{$tag}++ ) { + push @affected, $tag; + } + } + + return @affected; +} + =head3 _type =cut diff --git a/t/db_dependent/Biblio.t b/t/db_dependent/Biblio.t index 3af45df7475..5fc533b172e 100755 --- a/t/db_dependent/Biblio.t +++ b/t/db_dependent/Biblio.t @@ -1221,15 +1221,15 @@ subtest 'ModBiblio on record with strippable non-XML characters' => sub { my @warnings; C4::Biblio::ModBiblio( $record, $biblionumber, '', { warnings => \@warnings } ); like( - $warnings[0]{message}, qr/parser error : PCDATA invalid Char value 31/, + $warnings[0]{message}, qr/Non-XML characters stripped from: /, 'Non-XML character stripping reported via warnings arrayref' ); my $metadata = Koha::Biblios->find($biblionumber)->metadata; my $nonxml_error = $metadata->metadata_errors->search( { error_type => 'nonxml_stripped' } )->next; like( - $nonxml_error->message, qr/parser error : PCDATA invalid Char value 31/, - 'nonxml_stripped error persisted in biblio_metadata_errors after ModBiblio' + $nonxml_error->message, qr/Non-XML characters stripped from: /, + 'nonxml_stripped error message persisted in biblio_metadata_errors after ModBiblio' ); my $action_logs = diff --git a/t/db_dependent/Koha/Biblio/Metadata.t b/t/db_dependent/Koha/Biblio/Metadata.t index 7acba793a9e..96b26fe02ab 100755 --- a/t/db_dependent/Koha/Biblio/Metadata.t +++ b/t/db_dependent/Koha/Biblio/Metadata.t @@ -84,7 +84,7 @@ EOX }; subtest 'MARCXML with strippable non-XML characters' => sub { - plan tests => 3; + plan tests => 4; $schema->storage->txn_begin; # Build MARCXML containing a non-XML control character (chr(31) = Unit Separator) @@ -107,9 +107,12 @@ EOX lives_ok { $record->store } 'MARCXML with strippable characters stores successfully'; - ok( - $record->metadata_errors->search( { error_type => 'nonxml_stripped' } )->count, - 'nonxml_stripped error recorded after stripping' + my $error = $record->metadata_errors->search( { error_type => 'nonxml_stripped' } )->next; + ok( $error, 'nonxml_stripped error recorded after stripping' ); + like( + $error->message, + qr/245\$a/, + 'error message identifies the affected MARC field and subfield' ); my $stored = Koha::Biblio::Metadatas->find( { biblionumber => $biblio->{biblionumber}, format => 'marcxml' } ); -- 2.53.0