Bugzilla – Attachment 194164 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: Replace nonxml_stripped column with biblio_metadata_errors table
c5d403c.patch (text/plain), 16.32 KB, created by
Martin Renvoize (ashimema)
on 2026-02-27 19:24:09 UTC
(
hide
)
Description:
Bug 35104: Replace nonxml_stripped column with biblio_metadata_errors table
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2026-02-27 19:24:09 UTC
Size:
16.32 KB
patch
obsolete
>From c5d403c8bd92fb42b41683385543d6d2faa12f7b Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >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<StripNonXmlChars>), the stripped version is saved and >-C<nonxml_stripped> is persisted on the row so callers and the UI can notify the user. >+characters (C<StripNonXmlChars>), the stripped version is saved and a >+C<nonxml_stripped> error row is written to C<biblio_metadata_errors> so >+callers and the UI can notify the user. Any pre-existing C<nonxml_stripped> >+error is removed when the record parses cleanly. > > If the MARCXML cannot be recovered at all, a > I<Koha::Exceptions::Metadata::Invalid> exception is thrown. >@@ -58,6 +62,8 @@ I<Koha::Exceptions::Metadata::Invalid> 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<Koha::Biblio::Metadata::Errors> 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 <https://www.gnu.org/licenses>. >+ >+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 <https://www.gnu.org/licenses>. >+ >+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 >
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