Many IO handles are opened in Koha with the parameter ':utf8' the result is to flag strings output ( or input )as being utf-8 strings. However the it does accept invalid utf8. To validate the output the parameter ':encoding(UTF-8) is required. We should at least be strict with what we output. As the use of :utf8 is flagged by Perl::Critic policies as a violation the QA test 00-percritic.t fails three tests. We should be stricter with output encodings
Created attachment 5058 [details] [review] Proposed Patch
Patch attached: tests now pass
Created attachment 5252 [details] [review] Signed-off patch I understand Colin concern and approve his solution which is obviously valid from a Perl strictness point of view. I haven't tested all modified scripts in order to detect any side effect, but don't think it's necessary.
QA comment : * if it's to fix a perlcritic I won't object * I would have preffered to have included this patch earlier, with opportunity to have more time to test more cases * OTH, Frédéric has also diacritics concern Marking passed QA, but would not object if chris requires more testing !
From what I can tell, this safely implements the stricter UTF-8 checks across numerous affected files; several files (C4/tests/Record_test.pl and C4/Installer.pm, most notably) do not have this change made; they are slurping in UTF-8 *input* however, which is outside the scope of this report. Any replacement of file handles with variables is done universally, so no mis-matches there. Marking as Passed QA
I don't feel I have enough time to test this thoroughly, holding for 3.6.1
sorry, but it does not apply anymore against master : 11:08 ~/koha.dev/koha-community (new/bug_6752 $%)$ git am -3 /tmp/Signed-off-patch-OH345M.patch Applying: Bug 6752: Be stricter with utf-8 encoding of output Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging admin/aqplan.pl Auto-merging circ/overdue.pl CONFLICT (content): Merge conflict in circ/overdue.pl Auto-merging circ/ypattrodue-attr-search-authvalue.pl CONFLICT (content): Merge conflict in circ/ypattrodue-attr-search-authvalue.pl Auto-merging circ/ysearch.pl CONFLICT (content): Merge conflict in circ/ysearch.pl Auto-merging misc/migration_tools/rebuild_zebra.pl Auto-merging misc/translator/xgettext.pl CONFLICT (content): Merge conflict in misc/translator/xgettext.pl Auto-merging tools/export.pl Failed to merge in the changes. Patch failed at 0001 Bug 6752: Be stricter with utf-8 encoding of output I planned to test at least some of them before pushing, and will do that once you submit a rebases patch.
Created attachment 6248 [details] [review] Patch rebased against current HEAD
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y Applying: Bug 6752: Be stricter with utf-8 encoding of output Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging cataloguing/ysearch.pl CONFLICT (content): Merge conflict in cataloguing/ysearch.pl Auto-merging misc/cronjobs/overdue_notices.pl Auto-merging misc/migration_tools/rebuild_zebra.pl Failed to merge in the changes. Patch failed at 0001 Bug 6752: Be stricter with utf-8 encoding of output When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort".
Created attachment 6928 [details] [review] Updated patch Updated patch to remove one case that has already been corrected by a subsequent patch
Created attachment 7246 [details] [review] signed-off patch
This patch has been tested by many people (the initial one, or the rebased one) Marking as "passed QA", i'll push it in a few seconds. However, I keep it for 3.8. If there is an undetected side-effect, we will have time to see it. chris_n, if you object you can push for 3.6, but I think you should not.
Patch pushed. Note that Jonathan forgot to add his signoff to the "signed-off patch" he uploaded, I've added it myself (with my signoff) before pushing.
There have been no further reports of problems so I am marking this bug resolved.