From c5d403c8bd92fb42b41683385543d6d2faa12f7b Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Fri, 27 Feb 2026 13:05:23 +0000 Subject: [PATCH] Bug 35104: Replace nonxml_stripped column with biblio_metadata_errors table Instead of storing a single nonxml_stripped flag directly on biblio_metadata, introduce a normalised biblio_metadata_errors table that can hold multiple typed error rows per metadata record. - Add biblio_metadata_errors table (id, metadata_id FK, error_type, message, created_on) with CASCADE delete from biblio_metadata - Remove the nonxml_stripped column from biblio_metadata - Add Koha::Biblio::Metadata::Error / Errors object classes - Refactor Koha::Biblio::Metadata::store() to write/clear a nonxml_stripped row in biblio_metadata_errors rather than the old column - Add metadata_errors() accessor on Koha::Biblio::Metadata - Update C4::Biblio, catalogue/detail.pl, catalogue/MARCdetail.pl and tests to query biblio_metadata_errors instead of the old column Sponsored-by: OpenFifth --- C4/Biblio.pm | 9 ++-- Koha/Biblio/Metadata.pm | 49 +++++++++++++++---- Koha/Biblio/Metadata/Error.pm | 38 ++++++++++++++ Koha/Biblio/Metadata/Errors.pm | 48 ++++++++++++++++++ catalogue/MARCdetail.pl | 5 +- catalogue/detail.pl | 5 +- .../data/mysql/atomicupdate/bug_35104.pl | 29 ++++++++--- installer/data/mysql/kohastructure.sql | 20 +++++++- t/db_dependent/Biblio.t | 7 +-- t/db_dependent/Koha/Biblio/Metadata.t | 10 +++- 10 files changed, 192 insertions(+), 28 deletions(-) create mode 100644 Koha/Biblio/Metadata/Error.pm create mode 100644 Koha/Biblio/Metadata/Errors.pm diff --git a/C4/Biblio.pm b/C4/Biblio.pm index 1e33fa722b8..3689e1a516a 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -3004,9 +3004,12 @@ sub ModBiblioMarc { } )->store; - if ( $m_rs->nonxml_stripped && ref $options->{warnings} eq 'ARRAY' ) { - push @{ $options->{warnings} }, - { type => 'nonxml_stripped', message => $m_rs->nonxml_stripped }; + if ( ref $options->{warnings} eq 'ARRAY' ) { + my $nonxml_error = $m_rs->metadata_errors->search( { error_type => 'nonxml_stripped' } )->next; + if ($nonxml_error) { + push @{ $options->{warnings} }, + { type => 'nonxml_stripped', message => $nonxml_error->message }; + } } unless ($skip_record_index) { diff --git a/Koha/Biblio/Metadata.pm b/Koha/Biblio/Metadata.pm index 8195553cb3f..bce132d00c5 100644 --- a/Koha/Biblio/Metadata.pm +++ b/Koha/Biblio/Metadata.pm @@ -24,6 +24,8 @@ use C4::Biblio qw( GetMarcFromKohaField ); use C4::Charset qw( StripNonXmlChars ); use C4::Items qw( GetMarcItem ); +use Koha::Biblio::Metadata::Error; +use Koha::Biblio::Metadata::Errors; use Koha::Database; use Koha::Exceptions::Metadata; use Koha::Logger; @@ -47,8 +49,10 @@ 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), the stripped version is saved and -C is persisted on the row so callers and the UI can notify the user. +characters (C), the stripped version is saved and a +C error row is written to C so +callers and the UI can notify the user. Any pre-existing C +error is removed when the record parses cleanly. If the MARCXML cannot be recovered at all, a I exception is thrown. @@ -58,6 +62,8 @@ I exception is thrown. sub store { my $self = shift; + my $nonxml_error_message; + # Check marcxml will roundtrip if ( $self->format eq 'marcxml' ) { @@ -69,11 +75,8 @@ sub store { }; my $marcxml_error = $@; chomp $marcxml_error; - if ($marcxml) { - # Clean parse – clear any previously recorded stripping - $self->nonxml_stripped(undef); - } else { + unless ($marcxml) { # Attempt recovery by stripping non-XML characters my $stripped_metadata = StripNonXmlChars( $self->metadata ); @@ -81,7 +84,7 @@ sub store { if ($stripped_marcxml) { - # Stripping fixed it – save the clean version and record what happened + # Stripping fixed it – save the clean version Koha::Logger->get->warn( sprintf( "Non-XML characters stripped from bibliographic record (biblionumber=%s): %s", @@ -89,7 +92,7 @@ sub store { ) ); $self->metadata($stripped_metadata); - $self->nonxml_stripped($marcxml_error); + $nonxml_error_message = $marcxml_error; } else { # Truly unrecoverable @@ -105,7 +108,35 @@ sub store { } } - return $self->SUPER::store; + $self->SUPER::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, + } + )->store; + } + + return $self; +} + +=head3 metadata_errors + + my $errors = $metadata->metadata_errors; + +Returns a I resultset for errors associated +with this metadata record. + +=cut + +sub metadata_errors { + my ($self) = @_; + return Koha::Biblio::Metadata::Errors->_new_from_dbic( scalar $self->_result->biblio_metadata_errors ); } =head3 record diff --git a/Koha/Biblio/Metadata/Error.pm b/Koha/Biblio/Metadata/Error.pm new file mode 100644 index 00000000000..37ecabb4781 --- /dev/null +++ b/Koha/Biblio/Metadata/Error.pm @@ -0,0 +1,38 @@ +package Koha::Biblio::Metadata::Error; + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use base qw(Koha::Object); + +=head1 NAME + +Koha::Biblio::Metadata::Error - Koha Biblio Metadata Error object class + +=head1 API + +=head2 Internal methods + +=head3 _type + +=cut + +sub _type { + return 'BiblioMetadataError'; +} + +1; diff --git a/Koha/Biblio/Metadata/Errors.pm b/Koha/Biblio/Metadata/Errors.pm new file mode 100644 index 00000000000..3fd752d2ab9 --- /dev/null +++ b/Koha/Biblio/Metadata/Errors.pm @@ -0,0 +1,48 @@ +package Koha::Biblio::Metadata::Errors; + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use Koha::Biblio::Metadata::Error; + +use base qw(Koha::Objects); + +=head1 NAME + +Koha::Biblio::Metadata::Errors - Koha Biblio Metadata Errors object set class + +=head1 API + +=head2 Internal methods + +=head3 _type + +=cut + +sub _type { + return 'BiblioMetadataError'; +} + +=head3 object_class + +=cut + +sub object_class { + return 'Koha::Biblio::Metadata::Error'; +} + +1; diff --git a/catalogue/MARCdetail.pl b/catalogue/MARCdetail.pl index 3b71fbd90fd..51538494625 100755 --- a/catalogue/MARCdetail.pl +++ b/catalogue/MARCdetail.pl @@ -120,8 +120,9 @@ if ( $query->cookie("searchToOrder") ) { ); } -$template->param( ocoins => $biblio_object->get_coins ); -$template->param( nonxml_stripped => $biblio_object->metadata->nonxml_stripped ); +$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 ); #count of item linked my $itemcount = $biblio_object->items->count; diff --git a/catalogue/detail.pl b/catalogue/detail.pl index 99464f0de20..f14c9c3fb0d 100755 --- a/catalogue/detail.pl +++ b/catalogue/detail.pl @@ -176,8 +176,9 @@ my $marcflavour = C4::Context->preference("marcflavour"); $template->param( 'SpineLabelShowPrintOnBibDetails' => C4::Context->preference("SpineLabelShowPrintOnBibDetails") ); -$template->param( ocoins => !$invalid_marc_record ? $biblio->get_coins : undef ); -$template->param( nonxml_stripped => $biblio->metadata->nonxml_stripped ); +$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 ); # some useful variables for enhanced content; # in each case, we're grabbing the first value we find in diff --git a/installer/data/mysql/atomicupdate/bug_35104.pl b/installer/data/mysql/atomicupdate/bug_35104.pl index eafc9eef483..9391d7c861b 100755 --- a/installer/data/mysql/atomicupdate/bug_35104.pl +++ b/installer/data/mysql/atomicupdate/bug_35104.pl @@ -3,21 +3,38 @@ use Koha::Installer::Output qw(say_warning say_success say_info); return { bug_number => "35104", - description => "Add nonxml_stripped column to biblio_metadata", + description => "Add biblio_metadata_errors table for normalised error tracking", up => sub { my ($args) = @_; my ( $dbh, $out ) = @$args{qw(dbh out)}; - unless ( column_exists( 'biblio_metadata', 'nonxml_stripped' ) ) { + # Remove the old nonxml_stripped column if it was added by an earlier version of this patch + if ( column_exists( 'biblio_metadata', 'nonxml_stripped' ) ) { $dbh->do( q{ ALTER TABLE biblio_metadata - ADD COLUMN `nonxml_stripped` mediumtext NULL DEFAULT NULL - COMMENT 'parse error message recorded when non-XML characters were stripped on last store()' - AFTER metadata + DROP COLUMN `nonxml_stripped` } ); - say $out "Added new column 'biblio_metadata.nonxml_stripped'"; + say $out "Dropped column 'biblio_metadata.nonxml_stripped'"; + } + + unless ( TableExists('biblio_metadata_errors') ) { + $dbh->do( + q{ + CREATE TABLE `biblio_metadata_errors` ( + `id` int(11) NOT NULL AUTO_INCREMENT, + `metadata_id` int(11) NOT NULL COMMENT 'FK to biblio_metadata.id', + `error_type` varchar(64) NOT NULL COMMENT 'Error type identifier, e.g. nonxml_stripped', + `message` mediumtext DEFAULT NULL COMMENT 'Error message details', + `created_on` timestamp NOT NULL DEFAULT current_timestamp(), + PRIMARY KEY (`id`), + KEY `biblio_metadata_errors_fk_1` (`metadata_id`), + CONSTRAINT `biblio_metadata_errors_fk_1` FOREIGN KEY (`metadata_id`) REFERENCES `biblio_metadata` (`id`) ON DELETE CASCADE ON UPDATE CASCADE + ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci + } + ); + say $out "Added new table 'biblio_metadata_errors'"; } }, }; diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index 9ffb38c2aa6..c2fd8639601 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -1148,7 +1148,6 @@ CREATE TABLE `biblio_metadata` ( `format` varchar(16) NOT NULL, `schema` varchar(16) NOT NULL, `metadata` longtext NOT NULL, - `nonxml_stripped` mediumtext NULL DEFAULT NULL, `timestamp` timestamp NOT NULL DEFAULT current_timestamp() ON UPDATE current_timestamp(), `record_source_id` int(11) DEFAULT NULL COMMENT 'The record source for the metadata', PRIMARY KEY (`id`), @@ -1160,6 +1159,25 @@ CREATE TABLE `biblio_metadata` ( ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; /*!40101 SET character_set_client = @saved_cs_client */; +-- +-- Table structure for table `biblio_metadata_errors` +-- + +DROP TABLE IF EXISTS `biblio_metadata_errors`; +/*!40101 SET @saved_cs_client = @@character_set_client */; +/*!40101 SET character_set_client = utf8mb4 */; +CREATE TABLE `biblio_metadata_errors` ( + `id` int(11) NOT NULL AUTO_INCREMENT, + `metadata_id` int(11) NOT NULL COMMENT 'FK to biblio_metadata.id', + `error_type` varchar(64) NOT NULL COMMENT 'Error type identifier, e.g. nonxml_stripped', + `message` mediumtext DEFAULT NULL COMMENT 'Error message details', + `created_on` timestamp NOT NULL DEFAULT current_timestamp(), + PRIMARY KEY (`id`), + KEY `biblio_metadata_errors_fk_1` (`metadata_id`), + CONSTRAINT `biblio_metadata_errors_fk_1` FOREIGN KEY (`metadata_id`) REFERENCES `biblio_metadata` (`id`) ON DELETE CASCADE ON UPDATE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; +/*!40101 SET character_set_client = @saved_cs_client */; + -- -- Table structure for table `biblioitems` -- diff --git a/t/db_dependent/Biblio.t b/t/db_dependent/Biblio.t index 2107fba2d02..3af45df7475 100755 --- a/t/db_dependent/Biblio.t +++ b/t/db_dependent/Biblio.t @@ -1225,10 +1225,11 @@ subtest 'ModBiblio on record with strippable non-XML characters' => sub { 'Non-XML character stripping reported via warnings arrayref' ); - my $metadata = Koha::Biblios->find($biblionumber)->metadata; + my $metadata = Koha::Biblios->find($biblionumber)->metadata; + my $nonxml_error = $metadata->metadata_errors->search( { error_type => 'nonxml_stripped' } )->next; like( - $metadata->nonxml_stripped, qr/parser error : PCDATA invalid Char value 31/, - 'nonxml_stripped persisted on biblio_metadata after ModBiblio' + $nonxml_error->message, qr/parser error : PCDATA invalid Char value 31/, + 'nonxml_stripped error 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 177bf94d482..7acba793a9e 100755 --- a/t/db_dependent/Koha/Biblio/Metadata.t +++ b/t/db_dependent/Koha/Biblio/Metadata.t @@ -75,7 +75,10 @@ EOX lives_ok { $record->store } 'Valid MARCXML record stores successfully'; - is( $record->nonxml_stripped, undef, 'nonxml_stripped is undef for clean record' ); + is( + $record->metadata_errors->search( { error_type => 'nonxml_stripped' } )->count, + 0, 'No nonxml_stripped error for clean record' + ); $schema->storage->txn_rollback; }; @@ -104,7 +107,10 @@ EOX lives_ok { $record->store } 'MARCXML with strippable characters stores successfully'; - ok( $record->nonxml_stripped, 'nonxml_stripped is set after stripping' ); + ok( + $record->metadata_errors->search( { error_type => 'nonxml_stripped' } )->count, + 'nonxml_stripped error recorded after stripping' + ); my $stored = Koha::Biblio::Metadatas->find( { biblionumber => $biblio->{biblionumber}, format => 'marcxml' } ); is( -- 2.53.0