Do not short-circuit decode_utf8 with utf8 flags

[miyagawa]

The documentation about decode_utf8 says:

decode_utf8
    $string = decode_utf8($octets [, CHECK]);
Equivalent to "$string = decode("utf8", $octets [, CHECK])". 

which is not true, because it bypasses the whole decoding process if $octet already has utf-8 flags.

I think it's incorrect to bypass the decoding depending on the utf-8 flags, since a) it will emit unreliable, inconsistent results depending on how the byte string has been generated, and b) it's not consistent to decode("utf8", $octets) in any ways.

This patch eliminates the check, and passes all tests.

[miyagawa]

# file encoding: utf-8
use strict;
use Encode qw(decode_utf8 FB_WARN);

my $t1 = "Léon";
my $t2 = "L\x{e9}on"; # latin-1

my $pad = "\x{30C9}";

for my $byte ($t1, $t2) {
    warn "Decoding $byte";
    my $orig = $byte;
    Encode::decode_utf8($orig, FB_WARN);

    warn "Decoding $byte";
    my $orig2 = $byte;
    Encode::decode('utf-8', $orig2, FB_WARN);

    chop(my $new = $byte . $pad);
    warn "Decoding $new after pad/chop";
    Encode::decode_utf8($new, FB_WARN);
}

From the user's point of view, because $t2 is latin-1 string encoded, we expect decode_utf8($t2) to give warnings all the time, since you're trying to decode latin-1 with UTF-8.

Before this patch:

perl ~/tmp/decode_latin1.t
Decoding Léon at /Users/miyagawa/tmp/decode_latin1.t line 11.
Decoding Léon at /Users/miyagawa/tmp/decode_latin1.t line 15.
Decoding Léon after pad/chop at /Users/miyagawa/tmp/decode_latin1.t line 20.
Decoding L�on at /Users/miyagawa/tmp/decode_latin1.t line 11.
utf8 "\xE9" does not map to Unicode at /Users/miyagawa/.plenv/versions/5.18.1/lib/perl5/site_perl/5.18.1/darwin-2level/Encode.pm line 217.
Decoding L�on at /Users/miyagawa/tmp/decode_latin1.t line 15.
utf8 "\xE9" does not map to Unicode at /Users/miyagawa/.plenv/versions/5.18.1/lib/perl5/site_perl/5.18.1/darwin-2level/Encode.pm line 176.
Decoding L�on after pad/chop at /Users/miyagawa/tmp/decode_latin1.t line 20.

The last version, decode_utf8($t2) with pad/chop (i.e. implicit upgrade) doesn't give a warning, and returns the original string as a result. This is not just wrong, but also makes decode_utf8 unreliable and unusable because you, a user of Encode.pm, always have to make sure if the data has a utf-8 flag in it.

After this patch, all the calls to decode_utf8, will give warnings and reliable result, as decode("utf-8") does.

[miyagawa]

I checked
https://rt.cpan.org/Ticket/Display.html?id=87267
https://rt.cpan.org/Ticket/Display.html?id=61671

decode_utf8() is now ISOMORPHICALLY IDENTICAL to decode('utf8' …), which wasn't till 2.40.

While I'm not sure what other issues have been fixed in 2.40, i'm confident decode_utf8 and decode("utf-8") isn't equivalent in the current version.

Let me know if I should better make that above test as an actual unit test. I'm happy to.

[vsespb]

Contradict #10 which is merged too

[miyagawa]

Good catch. I would vote for reverting #10 then since it is not necessary?

—

Sent from Mailbox for iPhone

On Thu, Aug 29, 2013 at 8:05 AM, Victor Efimov [email protected]
wrote:

Contradict #10 which is merged too

Reply to this email directly or view it on GitHub:
#11 (comment)

[vsespb]

I would vote for reverting my patch ( #10 ) too. However @dankogai rejected patches like #11 since Y2010, that's why I sent #10. Also there is testsuites for #11 in RT#87267

[miyagawa]

@vsespb can't see a test suite (.t) on the RT ticket. I will make one up.

[vsespb]

@miyagawa It's plaintext (not attachment) in first message https://rt.cpan.org/Public/Bug/Display.html?id=87267

[miyagawa]

done #12

[anarcat]

for the record, this totally broke unicode support in at least one Perl application I use (ikiwiki), see http://ikiwiki.info/bugs/garbled_non-ascii_characters_in_body_in_web_interface/ for the fun stuff there...

maybe keeping a "safe_decode_utf8" function for backwards compatibility could be useful for all those tools out there that made the mistake of surviving with the hack?

[miyagawa]

@anarcat sorry to hear that, but I'm sure that was due to the broken model of string handlings in the application, where it was accidentally running due to the bug in Encode.pm. If you really liked the previous behavior of Encode#decode_utf8, you can do so by providing a wrapper function that checks Encode::is_utf8($str) first.

[anarcat]

i know it was probably a "bug" in the application, but the reality of this is that this is a major change in the way this function actually works. Perl 5.20 now ships with this and this will break probably hundreds of perl programs that worked because of this handler that is now removed.

i also know how to make my own wrapper, but it seems to me this should have been part of the perldelta for 5.20, at the very least, and ideally provide backwards-compatible wrappers for people.

i am not saying this change is wrong, mind you, I am just saying that things that used to work are now broken and that porting old application to this new paradigm is a significant amount of work that seems to have been overlooked here.

[vsespb]

I am just saying that things that used to work are now broken

that's sad. sorry for your trouble.

will break probably hundreds of perl programs that worked

I hope no. I hope people use things not just because "that works", but only when it's documented (or, at least sane) feature that they use.
But I could be wrong.

this should have been part of the perldelta for 5.20

probably yes, you're right.