From 5e5891c73a1d9cca0885620e7c4ca15501203923 Mon Sep 17 00:00:00 2001 From: Martin Renvoize 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
 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 - human-readable field reference, e.g. C<245$a> or C<001>
+
+=item * C - ordinal (decimal) value of the bad character
 
-    my @fields = _nonxml_affected_fields( $marcxml_string );
+=item * C - 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, so the
-set of affected fields reported here exactly matches what would be stripped.
+The same character class is used as L, 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{]*\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->( "$tag\$$1", $2 );
         }
     }
 
     # Scan control fields
     while ( $marcxml =~ m{]*\btag="(\w+)"[^>]*>(.*?)}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