Bug #96941 for URI: Usage of \C now gives deprecation warning in 5.21.2 and higher

Fri Jul 04 14:16:04 2014 ether [...] cpan.org - Ticket created

Subject:

Usage of \C now gives deprecation warning in 5.21.2 and higher

URI::Escape uses \C to parse the string to be escaped and should be fixed. Given how this breaks UTF-8 string parsing, I'm wondering if its usage has always been buggy; perhaps we need more tests for UTF-8.

Sat Jul 05 16:16:19 2014 IKEGAMI [...] cpan.org - Correspondence added

On Fri Jul 04 14:16:04 2014, ETHER wrote: Show quoted text

> URI::Escape uses \C to parse the string to be escaped and should be > fixed. > > Given how this breaks UTF-8 string parsing, I'm wondering if its usage > has always been buggy; perhaps we need more tests for UTF-8.

\C is only used by an undocumented function that's not used by the module. The function is indeed buggy as it can encode identical strings differently. $ perl -MURI::Escape -wE'$_="\xE9"; utf8::downgrade($_); say URI::Escape::escape_char($_);' %E9 $ perl -MURI::Escape -wE'$_="\xE9"; utf8::upgrade($_); say URI::Escape::escape_char($_);' %C3%A9 The following version of escape_char behaves identically, but doesn't use \C: # XXX escape_char is buggy as it assigns meaning to the string's storage format. sub escape_char { if (utf8::is_utf8($_[0])) { my $s = $_[0]; utf8::encode($s); unshift(@_, $s); } return join '', @URI::Escape::escapes{$_[0] =~ /(.)/g}; }

Sat Jul 05 16:16:20 2014 The RT System itself - Status changed from 'new' to 'open'

Sat Jul 05 16:23:51 2014 IKEGAMI [...] cpan.org - Correspondence added

On Sat Jul 05 16:16:19 2014, ikegami wrote: Show quoted text

> \C is only used by an undocumented function that's not used by the > module. > > The function is indeed buggy as it can encode identical strings > differently.

It is used by URI->new, though. URI->new is buggy :( $ perl -MURI -wE'$_="http://foo/\xE9"; utf8::upgrade($_); say URI->new($_);' http://foo/%C3%A9 $ perl -MURI -wE'$_="http://foo/\xE9"; utf8::downgrade($_); say URI->new($_);' http://foo/%E9

Tue Jul 08 11:56:04 2014 IKEGAMI [...] cpan.org - Cc ikegami added

Tue Jul 08 11:57:18 2014 IKEGAMI [...] cpan.org - Correspondence added

On Sat Jul 05 16:16:19 2014, ikegami wrote: Show quoted text

> The following version of escape_char behaves identically, but doesn't > use \C: > > # XXX escape_char is buggy as it assigns meaning to the string's > storage format. > sub escape_char { > if (utf8::is_utf8($_[0])) { > my $s = $_[0]; > utf8::encode($s); > unshift(@_, $s); > } > > return join '', @URI::Escape::escapes{$_[0] =~ /(.)/g}; > }

That should be /(.)/sg.

Tue Jul 08 13:48:40 2014 ether [...] cpan.org - Taken

Fri Jul 11 23:43:05 2014 ether [...] cpan.org - Correspondence added

Thanks, 1.62 released!

Fri Jul 11 23:43:09 2014 ether [...] cpan.org - Status changed from 'open' to 'resolved'

Sun Jul 13 00:05:40 2014 ether [...] cpan.org - Correspondence added

Unfortunately this fix causes other tests to fail on perls earlier than 5.12.1: https://rt.cpan.org/Ticket/Display.html?id=97177 I'm not sure if this is a resolvable situation, but regardless, the way in which strings are parsed in this module need to be reconsidered with respect to the utf8 flags on the string.

Sun Jul 13 00:05:57 2014 ether [...] cpan.org - Reference to ticket #97177 added

Sun Jul 13 00:30:53 2014 ikegami [...] adaelis.com - Correspondence added

CC:	IKEGAMI [...] cpan.org
Subject:	Re: [rt.cpan.org #96941] Usage of \C now gives deprecation warning in 5.21.2 and higher
Date:	Sun, 13 Jul 2014 00:30:44 -0400
To:	bug-URI [...] rt.cpan.org
From:	Eric Brine <ikegami [...] adaelis.com>

I'll have a look at it tomorrow. On Sun, Jul 13, 2014 at 12:05 AM, Karen Etheridge via RT < bug-URI@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=96941 > > > Unfortunately this fix causes other tests to fail on perls earlier than > 5.12.1: https://rt.cpan.org/Ticket/Display.html?id=97177 > > I'm not sure if this is a resolvable situation, but regardless, the way in > which strings are parsed in this module need to be reconsidered with > respect to the utf8 flags on the string. >

Sun Jul 13 03:10:33 2014 ikegami [...] adaelis.com - Correspondence added

CC:	ether [...] cpan.org, IKEGAMI [...] cpan.org
Subject:	Re: [rt.cpan.org #96941] Usage of \C now gives deprecation warning in 5.21.2 and higher
Date:	Sun, 13 Jul 2014 03:10:24 -0400
To:	bug-URI [...] rt.cpan.org
From:	Eric Brine <ikegami [...] adaelis.com>

On Sun, Jul 13, 2014 at 12:05 AM, Karen Etheridge via RT < bug-URI@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=96941 > > > Unfortunately this fix causes other tests to fail on perls earlier than > 5.12.1: https://rt.cpan.org/Ticket/Display.html?id=97177

Your encountering a bug in utf8::is_utf8 that occurs when escape_char($1) is performed. Fix follows: (Tested with 5.10.1) # XXX FIXME escape_char is buggy as it assigns meaning to the string's storage format. sub escape_char { # Old versions of utf8::is_utf8() didn't properly handle magical vars (e.g. $1). # The following forces a fetch to occur beforehand. my $dummy = substr($_[0], 0, 0); if (utf8::is_utf8($_[0])) { my $s = shift; utf8::encode($s); unshift(@_, $s); } return join '', @URI::Escape::escapes{$_[0] =~ /(.)/sg}; } Note that split // is faster than /./sg, so you might want to use: (Tested with 5.10.1) return join '', @URI::Escape::escapes{split //, $_[0]}; One of your test files uses subtest() from Test::More. Note that subtest() wasn't in Test::More 0.92, the Test::More that comes with 5.10.1. You might want to bump the prereq version of Test::More to 0.94. - Eric

Sun Jul 13 17:18:14 2014 ether [...] cpan.org - Correspondence added

CC:	bug-URI [...] rt.cpan.org, IKEGAMI [...] cpan.org
Subject:	Re: [rt.cpan.org #96941] Usage of \C now gives deprecation warning in 5.21.2 and higher
Date:	Sun, 13 Jul 2014 14:18:00 -0700
To:	Eric Brine <ikegami [...] adaelis.com>
From:	Karen Etheridge <ether [...] cpan.org>

On Sun, Jul 13, 2014 at 03:10:24AM -0400, Eric Brine wrote: Show quoted text

> Your encountering a bug in utf8::is_utf8 that occurs when escape_char($1) > is performed. Fix follows: (Tested with 5.10.1)

All released in 1.64. thanks!

Sun Jul 13 17:19:15 2014 ether [...] cpan.org - Correspondence added

CC:	bug-URI [...] rt.cpan.org, IKEGAMI [...] cpan.org
Subject:	Re: [rt.cpan.org #96941] Usage of \C now gives deprecation warning in 5.21.2 and higher
Date:	Sun, 13 Jul 2014 14:19:06 -0700
To:	Eric Brine <ikegami [...] adaelis.com>
From:	Karen Etheridge <ether [...] cpan.org>

(resent, due to mailer issue) On Sun, Jul 13, 2014 at 03:10:24AM -0400, Eric Brine wrote: Show quoted text

> Your encountering a bug in utf8::is_utf8 that occurs when escape_char($1) > is performed. Fix follows: (Tested with 5.10.1) > # Old versions of utf8::is_utf8() didn't properly handle magical vars (e.g. $1). > # The following forces a fetch to occur beforehand. > my $dummy = substr($_[0], 0, 0);

Interesting; can you explain what's going on here and why adding this line helps? Show quoted text

> if (utf8::is_utf8($_[0])) { > my $s = shift;

This change looks like we were mistakenly adding an extra argument to @_ previously... Show quoted text

> Note that split // is faster than /./sg, so you might want to use: (Tested > with 5.10.1)

I was considering that earlier (wondering why we were even matching on \C or . in the first place), but on its own it didn't help anything. But, agreed that it's faster. Show quoted text

> One of your test files uses subtest() from Test::More. Note that subtest() > wasn't in Test::More 0.92, the Test::More that comes with 5.10.1. You might > want to bump the prereq version of Test::More to 0.94.

Thanks, well spotted!

Thu Oct 23 17:57:08 2014 ether [...] cpan.org - Fixed in 1.64 added