The standard UNIMARC requires than the 9th character (starting from 0) in labels must be blank (while it may be 'a' in marc21) the problem is that C4::Charset::SetMarcUnicodeFlag (called in particular when we import a record) always add 'a' char in the 9th label'pos whereas it should do it just for MARC21 and NORMARC (not for UNIMARC) : C4::Charset::SetMarcUnicodeFlag add 'a' char in the 9th label character for MARC21 and NORMARC (it's normal), but just before doing this it call "$marc_record->encoding('UTF-8')" which is a MARC::Record function which, when called with 'UTF-8' parameter, do only one thing : add 'a' char in the 9th label character
Created attachment 60547 [details] [review] Bug 18152 : fix unimarc label in SetMarcUnicodeFlag The standard UNIMARC requires than the 9th character (starting from 0) in labels must be blank (while it may be 'a' in marc21) the problem is that C4::Charset::SetMarcUnicodeFlag (called in particular when we import a record) always add 'a' char in the 9th label'pos whereas it should do it just for MARC21 and NORMARC (not for UNIMARC) : C4::Charset::SetMarcUnicodeFlag add 'a' char in the 9th label character for MARC21 and NORMARC (it's normal), but just before doing this it call "$marc_record->encoding('UTF-8')" which is a MARC::Record function which, when called with 'UTF-8' parameter, do only one thing : add 'a' char in the 9th label character This patch only removes this incorrect function call, so, when we import a bib record in UNIMARC : it no longer adds erroneous character (this does not change anything for MARC21 and NORMARC because SetMarcUnicodeFlag explicitly adds 'a' char in the 9th label for them)
Created attachment 60548 [details] to test import
Created attachment 60549 [details] marc21-with-a-char-in-label-pos9.mrc
Created attachment 60550 [details] marc21-without-a-char-in-label-pos9.mrc
Created attachment 60551 [details] unimarc-without-a-char-in-label-pos9.mrc
to test : before applying the patch : - change marcflavour syspref in UNIMARC - import unimarc-without-a-char-in-label-pos9.mrc in koha (see below how to): a 'a' char will be add in label pos.9 (it's the bug) - change marcflavour syspref in MARC21 - import marc21-with-a-char-in-label-pos9.mrc (record already have a 'a' char in label pos.9)in koha : nothing is change, label keep 'a' char in label pos.9 (it's correct for MARC21 records) - import marc21-without-a-char-in-label-pos9.mrc in koha : a 'a' char will be add in label pos.9 (it's correct for MARC21 records) - apply patch - change marcflavour syspref in UNIMARC - import unimarc-without-a-char-in-label-pos9.mrc in koha (see below how to): label pos.9 is blank (bug fixed \o/) - change marcflavour syspref in MARC21 - import marc21-with-a-char-in-label-pos9.mrc (record already have a 'a' char in label pos.9)in koha : nothing is change, label keep 'a' char in label pos.9 (it's correct for MARC21 records, It's like before the patch) - import marc21-without-a-char-in-label-pos9.mrc in koha : a 'a' char will be add in label pos.9 (like before the patch, it's correct for MARC21 records) Note : how to import a record in Koha : Home › Tools › Stage MARC records for import, find file, click on "upload",verify character encoding is UTF-8, click on "stage for import" ; Home › Tools › Manage staged MARC records : choose your file and clic on "import" button ; then you can click below on file name to see it (and note (or not) the presence of char 'a' in label pos.9)
Hello Stéphane, Maybe a dumb question: what side-effects this bug can cause?
Hi Jonathan, In fact... I don't know. That's invalid and being invalid is evil. more info : We're using the exporter to send the iso2709 file to another (proprietary) ILS, and the file is rejected, as it is invalid. "La loi est dure mais c'est la loi" ;-)
Ok! So I let you find a signoffer at the hackfest :)
Created attachment 62246 [details] [review] Bug 18152 : fix unimarc label in SetMarcUnicodeFlag The standard UNIMARC requires than the 9th character (starting from 0) in labels must be blank (while it may be 'a' in marc21) the problem is that C4::Charset::SetMarcUnicodeFlag (called in particular when we import a record) always add 'a' char in the 9th label'pos whereas it should do it just for MARC21 and NORMARC (not for UNIMARC) : C4::Charset::SetMarcUnicodeFlag add 'a' char in the 9th label character for MARC21 and NORMARC (it's normal), but just before doing this it call "$marc_record->encoding('UTF-8')" which is a MARC::Record function which, when called with 'UTF-8' parameter, do only one thing : add 'a' char in the 9th label character This patch only removes this incorrect function call, so, when we import a bib record in UNIMARC : it no longer adds erroneous character (this does not change anything for MARC21 and NORMARC because SetMarcUnicodeFlag explicitly adds 'a' char in the 9th label for them) Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Additional questions: 1. MARC::Record does not handle UNIMARC records correctly, so we should never call ->encoding('utf-8') from Koha, but SetMarcUnicodeFlag instead, right? 2. Sounds like the code in SetMarcUnicodeFlag should then be: If MARC21 or NORMARC: $marc_record->encoding('utf-8') else # unimarc do our stuffs 3. Please fix author's name of the patch
Answers to the above questions : - Question 1 : I never said "MARC::Record does not handle UNIMARC records correctly" ! MARC::Record->encoding('utf-8') add 'a' char in the 9th label'pos, it's ok in MARC21 and NORMARC but not in UNIMARC (requires than the 9th character in labels must be blank) C4::Charset::SetMarcUnicodeFlag always call MARC::Record->encoding('utf-8') but it should only do for MARC21 and NORMARC - Question 2 : In SetMarcUnicodeFlag, if MARC21 or NORMARC already do «substr($leader, 9, 1) = 'a'» in addition to MARC::Record->encoding('utf-8') (Which does exactly the same thing for UNIMARC MARC21 NORMARC) We had the choice between : - move the «MARC::Record->encoding('utf-8')»'s call instead of «substr($leader, 9, 1) = 'a'» (it's your proposition) - delete the «MARC::Record->encoding('utf-8')»'s call and keep «substr($leader, 9, 1) = 'a'» (chosen solution) Both are equivalent because «MARC::Record->encoding('utf-8')» only do one thing : «substr($leader, 9, 1) = 'a'» - Question 3 : I do not understand the problem and I do not know how to do this
Stephane, if you look at your patch: From 9f6f0e05c9c44164d4c2d8f00f4bbd2afd87b847 Mon Sep 17 00:00:00 2001 From: Koha User <support@biblibre.com> <<<<<<< THIS IS WRONG Date: Wed, 22 Feb 2017 12:25:33 +0100 Set your name => https://wiki.koha-community.org/wiki/Version_Control_Using_Git#Set_your_name_.26_mail_address
Created attachment 63008 [details] [review] Bug 18152 : fix unimarc label in SetMarcUnicodeFlag only fix author's name of the patch
(In reply to stephane.delaune from comment #12) "MARC::Record does not handle UNIMARC records correctly" and "MARC::Record->encoding('utf-8') add 'a' char in the 9th label'pos, it's ok in MARC21 and NORMARC but not in UNIMARC" mean the same thing to me
Created attachment 63040 [details] [review] Bug 18152: Add tests
Created attachment 63041 [details] [review] Bug 18152: Leader 9 for UNIMARC records must be blank
Paul, Stéphane, Regarding the description of this bug we are expecting the Leader 9 for UNIMARC records to be blank, does it make sense to set it in SetMarcUnicodeFlag? Let me know if you agree with the last patch, otherwise I will adapt the tests to make them pass without it.
(In reply to Jonathan Druart from comment #18) > Regarding the description of this bug we are expecting the Leader 9 for > UNIMARC records to be blank, does it make sense to set it in > SetMarcUnicodeFlag? I do not think it makes sense actually, must be done elsewhere.
(In reply to Jonathan Druart from comment #19) > (In reply to Jonathan Druart from comment #18) > > Regarding the description of this bug we are expecting the Leader 9 for > > UNIMARC records to be blank, does it make sense to set it in > > SetMarcUnicodeFlag? > > I do not think it makes sense actually, must be done elsewhere. What does not make sense is that SetMarcUnicodeFlag adds an invalid character where it should not (which can be considered a bug). To do so elsewhere is to leave a bug in place to do corrective action elsewhere: a nonsense
I do not understand your last comment.
Created attachment 63102 [details] [review] Bug 18152 : fix unimarc label in SetMarcUnicodeFlag The standard UNIMARC requires than the 9th character (starting from 0) in labels must be blank (while it may be 'a' in marc21) the problem is that C4::Charset::SetMarcUnicodeFlag (called in particular when we import a record) always add 'a' char in the 9th label'pos whereas it should do it just for MARC21 and NORMARC (not for UNIMARC) : C4::Charset::SetMarcUnicodeFlag add 'a' char in the 9th label character for MARC21 and NORMARC (it's normal), but just before doing this it call "$marc_record->encoding('UTF-8')" which is a MARC::Record function which, when called with 'UTF-8' parameter, do only one thing : add 'a' char in the 9th label character This patch only removes this incorrect function call, so, when we import a bib record in UNIMARC : it no longer adds erroneous character (this does not change anything for MARC21 and NORMARC because SetMarcUnicodeFlag explicitly adds 'a' char in the 9th label for them) Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 63103 [details] [review] Bug 18152: Add tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Seen with Stéphane: It does not make really sense to modify position 9 of leader for UNIMARC records in SetMarcUnicodeFlag, this flag is not related to encoding. I have opened bug 18533 to remove the MARC::Record->encoding calls everywhere and replace them with SetMarcUnicodeFlag.
Pushed to master for 17.05, thanks Stephane, Jonathan!
These patches have been pushed to 16.11.x and will be in 16.11.08.
Pushed to 3.22.x for 3.22.21
Pushed to 16.05.x, for 16.05.13 release
This patch introduced a regression, please provide a patch ASAP on bug 18910.
(In reply to Jonathan Druart from comment #29) > This patch introduced a regression, please provide a patch ASAP on bug 18910. I suggest to revert these 2 patches, Rmaints please take a look at bug 18910.
Was reverted from 16.11.x for the 16.11.10 release.