Bug 6752 - Be stricter with utf-8 output
Summary: Be stricter with utf-8 output
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 3.8
Hardware: All All
: P3 enhancement (vote)
Assignee: Colin Campbell
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks: 6895
  Show dependency treegraph
 
Reported: 2011-08-19 10:42 UTC by Colin Campbell
Modified: 2013-12-05 20:04 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed Patch (17.44 KB, patch)
2011-08-19 11:16 UTC, Colin Campbell
Details | Diff | Splinter Review
Signed-off patch (17.55 KB, patch)
2011-09-01 12:07 UTC, Frédéric Demians
Details | Diff | Splinter Review
Patch rebased against current HEAD (15.19 KB, patch)
2011-11-07 14:28 UTC, Colin Campbell
Details | Diff | Splinter Review
Updated patch (14.62 KB, patch)
2011-12-23 10:44 UTC, Colin Campbell
Details | Diff | Splinter Review
signed-off patch (14.63 KB, patch)
2012-01-19 09:58 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2011-08-19 10:42:43 UTC
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
Comment 1 Colin Campbell 2011-08-19 11:16:26 UTC Comment hidden (obsolete)
Comment 2 Colin Campbell 2011-08-19 11:17:16 UTC
Patch attached: tests now pass
Comment 3 Frédéric Demians 2011-09-01 12:07:15 UTC Comment hidden (obsolete)
Comment 4 Paul Poulain 2011-10-06 14:31:44 UTC
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 !
Comment 5 Ian Walls 2011-10-12 21:00:22 UTC
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
Comment 6 Chris Cormack 2011-10-19 04:02:40 UTC
I don't feel I have enough time to test this thoroughly, holding for 3.6.1
Comment 7 Paul Poulain 2011-11-06 10:18:05 UTC
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.
Comment 8 Colin Campbell 2011-11-07 14:28:40 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2011-12-21 06:30:39 UTC
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".
Comment 10 Colin Campbell 2011-12-23 10:44:34 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2012-01-19 09:58:49 UTC
Created attachment 7246 [details] [review]
signed-off patch
Comment 12 Paul Poulain 2012-01-27 11:10:24 UTC
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.
Comment 13 Paul Poulain 2012-01-27 11:12:49 UTC
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.
Comment 14 Jared Camins-Esakov 2012-12-31 00:39:45 UTC
There have been no further reports of problems so I am marking this bug resolved.