From 6726b1a2544bd31a67a31d7c52c98384fdf5f867 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 8 Jul 2026 12:40:23 +0100 Subject: [PATCH] Bug 35104: Catch invalid metadata and surface repair warnings in AddBiblio/ModBiblio AddBiblio/ModBiblio/ModBiblioMarc gain an optional warnings arrayref option: any repair messages recorded by Koha::Biblio::Metadata->store() (see previous commit) are appended to it via object_messages, so a caller can show the cataloguer what was automatically fixed instead of the fix happening invisibly. AddBiblio also now distinguishes Koha::Exceptions::Metadata::Invalid from other errors in its catch block and rethrows it, so a caller can tell "this record is unrecoverable" apart from an unexpected internal error and react accordingly (see following bugs for callers doing so). The inline StripNonXmlChars calls in TransformHtmlToMarc are removed, since stripping is now handled centrally and consistently by Koha::Biblio::Metadata->store() rather than ad-hoc at every call site. Signed-off-by: Baptiste Bayche Signed-off-by: Baptiste Wojtkowski Sponsored-by: OpenFifth Signed-off-by: Nick Clemens Signed-off-by: Pedro Amorim --- C4/Biblio.pm | 27 +++++++++++++++++++++------ t/db_dependent/Biblio.t | 36 +++++++++++++++++++++++++++++++++++- 2 files changed, 56 insertions(+), 7 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index 4f84b45bec..c0d7baa7b9 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -89,7 +89,6 @@ use C4::Charset qw( nsb_clean SetMarcUnicodeFlag SetUTF8Flag - StripNonXmlChars ); use C4::Languages; use C4::Linker; @@ -301,6 +300,7 @@ sub AddBiblio { { skip_record_index => 1, record_source_id => $options->{record_source_id}, + ( ref $options->{warnings} eq 'ARRAY' ? ( warnings => $options->{warnings} ) : () ), } ); @@ -330,8 +330,14 @@ sub AddBiblio { $indexer->index_records( $biblionumber, "specialUpdate", "biblioserver" ); } } catch { - warn $_; - ( $biblionumber, $biblioitemnumber ) = ( undef, undef ); + my $error = $_; + if ( ref($error) eq 'Koha::Exceptions::Metadata::Invalid' ) { + ( $biblionumber, $biblioitemnumber ) = ( undef, undef ); + $error->rethrow; + } else { + warn $error; + ( $biblionumber, $biblioitemnumber ) = ( undef, undef ); + } }; return ( $biblionumber, $biblioitemnumber ); } @@ -399,7 +405,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 ) { @@ -2352,7 +2363,6 @@ sub TransformHtmlToMarc { # between 001 and 009 (included) } elsif ( $fval ne '' ) { - $fval = StripNonXmlChars($fval); #Strip out any non-XML characters like control characters $newfield = MARC::Field->new( $tag, $fval, ); } @@ -2377,7 +2387,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; @@ -3030,6 +3039,12 @@ sub ModBiblioMarc { } )->store; + if ( ref $options->{warnings} eq 'ARRAY' ) { + for my $msg ( grep { $_->type eq 'warning' } @{ $m_rs->object_messages } ) { + push @{ $options->{warnings} }, { type => $msg->message, message => $msg->payload }; + } + } + unless ($skip_record_index) { my $indexer = Koha::SearchEngine::Indexer->new( { index => $Koha::SearchEngine::BIBLIOS_INDEX } ); $indexer->index_records( $biblionumber, "specialUpdate", "biblioserver" ); diff --git a/t/db_dependent/Biblio.t b/t/db_dependent/Biblio.t index a239968f8c..1b2e81a7c2 100755 --- a/t/db_dependent/Biblio.t +++ b/t/db_dependent/Biblio.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 26; +use Test::More tests => 27; use Test::NoWarnings; use Test::MockModule; use Test::Warn; @@ -1230,6 +1230,40 @@ subtest 'ModBiblio on invalid record' => sub { like( $action_log->diff, qr/_marc/, "MARC field changes are captured in the diff column" ); }; +subtest 'ModBiblio on record with strippable non-XML characters' => sub { + plan tests => 4; + + t::lib::Mocks::mock_preference( "CataloguingLog", 1 ); + + # We create a record with an invalid control character (chr(31)) in the MARC + my $record = MARC::Record->new(); + my $field = MARC::Field->new( '650', '', '', 'a' => '00' . chr(31) . 'aD000015937' ); + $record->append_fields($field); + + my ($biblionumber) = C4::Biblio::AddBiblio( $record, '' ); + + # New behaviour: strippable non-XML characters are silently fixed; the + # stripping event is reported via the warnings arrayref, not Perl warn() + my @warnings; + C4::Biblio::ModBiblio( $record, $biblionumber, '', { warnings => \@warnings } ); + like( + $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_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_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 = + Koha::ActionLogs->search( { object => $biblionumber, module => 'Cataloguing', action => 'MODIFY' } ); + is( $action_logs->count, 1, "Modification of biblio was successfully recorded in action logs" ); +}; + subtest 'UpdateTotalIssues on Invalid record' => sub { plan tests => 2;