[dw-free] For XMLRPC, clear UTF8 flag from all strings, not just subject and event

[kareila]

[commit: http://hg.dwscoalition.org/dw-free/rev/c1e5d7213dfc]

http://bugs.dwscoalition.org/show_bug.cgi?id=2310

Before, we were only checking utf8 encoding for subject and event. It's
better to handle all utf8 strings from the request properly, using the
Encode module.

Codemerge from LiveJournal; prepared for Dreamwidth by denise.

Files modified:

• cgi-bin/Apache/LiveJournal.pm

--------------------------------------------------------------------------------
diff -r 99ed26f60316 -r c1e5d7213dfc cgi-bin/Apache/LiveJournal.pm
--- a/cgi-bin/Apache/LiveJournal.pm	Wed Jun 30 02:12:57 2010 +0800
+++ b/cgi-bin/Apache/LiveJournal.pm	Tue Jun 29 14:37:20 2010 -0500
@@ -1804,6 +1804,7 @@ sub anti_squatter
 }
 
 package LJ::Protocol;
+use Encode();
 
 sub xmlrpc_method {
     my $method = shift;
@@ -1818,10 +1819,9 @@ sub xmlrpc_method {
     }
     my $error = 0;
     if (ref $req eq "HASH") {
-        foreach my $key ('subject', 'event') {
-            # get rid of the UTF8 flag in scalars
-            $req->{$key} = LJ::no_utf8_flag ( $req->{$key} )
-                if $req->{$key};
+        # get rid of the UTF8 flag in scalars
+        while ( my ($k, $v) = each %$req ) {
+            $req->{$k} = Encode::encode_utf8($v) if Encode::is_utf8($v);
         }
     }
     my $res = LJ::Protocol::do_request($method, $req, \$error);
--------------------------------------------------------------------------------

Comments

[kareila]

And I would expect you to read the comments on the bug before second-guessing me, really.

[mark]

Nice!

I did read the comments to the bug, and they are pretty unclear. Sophie said "technically not the right way to do this" and then later you said "thumbs up on IRC". So, from my reading, there's no useful information in the comments to that bug. If there were, then I wouldn't have commented to you.

I did my due diligence, and then when I couldn't figure it out, I asked you. In a very nice, professional fashion. I would ask that you provide the same courtesy.

[kareila]

Sorry, I wouldn't characterize "this scares me a lot" as either nice or professional. It doesn't sound like you trust my judgment, honestly.

[mark]

Actually, it was both nice and professional.

I have worked on this codebase for nearly a decade and I know which of the areas are most likely to cause problems and be poorly understood. Encoding is a top three on that list.

If you get the encoding wrong, or do something that Perl or the databases don't expect, the cleanup process is a giant mess. (Been there, done that.) It's difficult, time consuming, and things are just broken while we sort through it. It's also vastly misunderstood or not understood at all by the majority of devs. Very few people understand it, which isn't a bad thing, but it does mean changing this is difficult and -- yes -- quite a bit scary.

I expect the committers to understand what it is they are committing. You are signing off on that patch, saying, "If something breaks, you can call me too". If that's not the case, you shouldn't be putting your name to it. I know that some things are particularly difficult and hard to work with, encoding, clustering, user moves, translation, dversions, etc, that's fine -- not everybody can be an effective committer on everything. I'd rather have people stay away from patches they don't understand than just commit stuff and hope for the best.

At the end of the day, though, my #1 job here on Dreamwidth is to make sure that we have a stable, secure, available site. This means I will be questioning commits from time to time, and if that is going to bother you, we're going to have to address that and figure something out. I am never going to be able to stop questioning things that I think need to be examined.

[kareila]

I apologize for overreacting, then.

However, I would prefer you to be more active in the review process, so that you're less likely to be put in the position of having to question commits after the fact.

[sophie]

For the record:

Perl's internal encoding that it tries to use is called "utf8" (no hyphen), and when the utf8 flag of a scalar is on, Perl treats the contents of that scalar as being in this encoding.

Calling Encode::encode_utf8($v) does the same as Encode::encode('utf8', $v);. What encode() actually does is it takes the encoding given, and returns a version of $v encoded as a series of bytes in that encoding, with the utf8 flag off. Because Perl treats utf8-flagged scalars as this encoding anyway, it's basically just flipping the flag off, but it's doing so in a way that's guaranteed to work with future versions of Perl.

The reason I said that it's technically not the right way to do this is because Perl's internal utf8 encoding is more lax than proper 'UTF-8' (hyphen), and as such it's possible that there would be invalid UTF-8 characters in a string encoded as 'utf8'. When outputting for display, strings should be encoded as 'UTF-8' (or its canonical name in Perl, 'utf-8-strict'), and not 'utf8'. As such, encode_utf8 is technically wrong, but it does what we need it to do in this case, which is to flip the flag without making any other changes, and it does so in a way that we know that any encode_utf8() calls in the code need to be changed as part of bug 443.

Hopefully that makes it clear why I okayed it.