You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@perl.apache.org by "Philippe M. Chiasson" <go...@ectoplasm.org> on 2004/10/07 01:39:40 UTC
[Patch mp2] unescape_url_info
as per todo/release:
Apache->unescape_url{_info}:
- not yet implemented. should be moved to Apache::Util (or may be
- APR::URI?)
unescape_url has already been implemented, so this small patch ports the mp1
code to Apache::URI::unescape_url_info
--
--------------------------------------------------------------------------------
Philippe M. Chiasson m/gozer\@(apache|cpan|ectoplasm)\.org/ GPG KeyID : 88C3A5A5
http://gozer.ectoplasm.org/ F9BF E0C2 480E 7680 1AE5 3631 CB32 A107 88C3A5A5
Re: [Patch mp2] unescape_url_info
Posted by "Philippe M. Chiasson" <go...@ectoplasm.org>.
Stas Bekman wrote:
> Philippe M. Chiasson wrote:
>>
>>Stas Bekman wrote:
>>
>>>Philippe M. Chiasson wrote:
>>>[...]
>>>
>>>>>it's probably always a good idea to discuss first before spending
>>>>>time on things :)
>>>>
>>>>So, what do we do about it? Yank it out, document better
>>>>alternatives, like
>>>>CGI::Util::unescape() and possibly implement in Apache/compat.pm as :
>>>> unescape_url() =~ s/\+/ /g;
>>>
>>>Implement it in Apache/compat.pm *and* document to use
>>>CGI::Util::unescape() in mp2?
>>
>>Here it is:
>
> +1 (plus doc of course :)
>
> There is one more things that needs to be updated, it's $methods_compat in
> MethodLookup.pm (which is autogenerated from lib/ModPerl/WrapXS.pm), this
> and the escape_html method should be there (did we forget any other
> recently added to compat.pm methods?)
escape_html is in there. As for other compat methods, I'll look into it, must
be missing some, since I was completely unaware of the need to update that list.
As for the patch itself, it's in, with doc and all.
--
--------------------------------------------------------------------------------
Philippe M. Chiasson m/gozer\@(apache|cpan|ectoplasm)\.org/ GPG KeyID : 88C3A5A5
http://gozer.ectoplasm.org/ F9BF E0C2 480E 7680 1AE5 3631 CB32 A107 88C3A5A5
Re: [Patch mp2] unescape_url_info
Posted by Stas Bekman <st...@stason.org>.
Philippe M. Chiasson wrote:
>
>
> Stas Bekman wrote:
>
>> Philippe M. Chiasson wrote:
>> [...]
>>
>>>> it's probably always a good idea to discuss first before spending
>>>> time on things :)
>>>
>>>
>>>
>>> So, what do we do about it? Yank it out, document better
>>> alternatives, like
>>> CGI::Util::unescape() and possibly implement in Apache/compat.pm as :
>>> unescape_url() =~ s/\+/ /g;
>>
>>
>>
>> Implement it in Apache/compat.pm *and* document to use
>> CGI::Util::unescape() in mp2?
>
>
> Here it is:
+1 (plus doc of course :)
There is one more things that needs to be updated, it's $methods_compat in
MethodLookup.pm (which is autogenerated from lib/ModPerl/WrapXS.pm), this
and the escape_html method should be there (did we forget any other
recently added to compat.pm methods?)
--
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [Patch mp2] unescape_url_info
Posted by "Philippe M. Chiasson" <go...@ectoplasm.org>.
Stas Bekman wrote:
> Philippe M. Chiasson wrote:
> [...]
>
>>>it's probably always a good idea to discuss first before spending time
>>>on things :)
>>
>>
>>So, what do we do about it? Yank it out, document better alternatives, like
>>CGI::Util::unescape() and possibly implement in Apache/compat.pm as :
>> unescape_url() =~ s/\+/ /g;
>
>
> Implement it in Apache/compat.pm *and* document to use
> CGI::Util::unescape() in mp2?
Here it is:
--
--------------------------------------------------------------------------------
Philippe M. Chiasson m/gozer\@(apache|cpan|ectoplasm)\.org/ GPG KeyID : 88C3A5A5
http://gozer.ectoplasm.org/ F9BF E0C2 480E 7680 1AE5 3631 CB32 A107 88C3A5A5
Re: [Patch mp2] unescape_url_info
Posted by Stas Bekman <st...@stason.org>.
Philippe M. Chiasson wrote:
[...]
>> it's probably always a good idea to discuss first before spending time
>> on things :)
>
>
> So, what do we do about it? Yank it out, document better alternatives, like
> CGI::Util::unescape() and possibly implement in Apache/compat.pm as :
> unescape_url() =~ s/\+/ /g;
Implement it in Apache/compat.pm *and* document to use
CGI::Util::unescape() in mp2?
--
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [Patch mp2] unescape_url_info
Posted by "Philippe M. Chiasson" <go...@ectoplasm.org>.
Stas Bekman wrote:
> Philippe M. Chiasson wrote:
>
> [...]
>
>>>also continuing yesterdays story, this function doesn't know to deal
>>>with utf8 strings, right? in which case we might just as well drop it
>>>too? is there a CPAN implementation for that functionality?
>>
>>I am 99.9% sure there is something on CPAN to do that
CGI::Util::unescape does what we are looking for.
>
> [...]
>
>>>But before you do any tweaks, let's see if we keep it at all.
>>
>>I'd be more than happy to yank it out completely. Only reason I ported
>>it forward was because it's listed in todo/release.
>
> it's probably always a good idea to discuss first before spending time on
> things :)
So, what do we do about it? Yank it out, document better alternatives, like
CGI::Util::unescape() and possibly implement in Apache/compat.pm as :
unescape_url() =~ s/\+/ /g;
--
--------------------------------------------------------------------------------
Philippe M. Chiasson m/gozer\@(apache|cpan|ectoplasm)\.org/ GPG KeyID : 88C3A5A5
http://gozer.ectoplasm.org/ F9BF E0C2 480E 7680 1AE5 3631 CB32 A107 88C3A5A5
Re: [Patch mp2] unescape_url_info
Posted by Stas Bekman <st...@stason.org>.
Philippe M. Chiasson wrote:
> As usual, I usually try and complete the documentation once the code
> gets a go ;-)
of course :)
>> also continuing yesterdays story, this function doesn't know to deal
>> with utf8 strings, right? in which case we might just as well drop it
>> too? is there a CPAN implementation for that functionality?
>
>
> I am 99.9% sure there is something on CPAN to do that
and if there is one, we should test that it does the same thing as mp1
did, and then we can consider dropping it? Geoff?
>> Wouldn't it be better to croak here instead?
>
>
> Not really, if you try and unescape_url an undef/empty string, you get
> empty back.
what's empty in the Perl language? undef, ""? why "" will become undef, or
the other way around?
>> But before you do any tweaks, let's see if we keep it at all.
>
> I'd be more than happy to yank it out completely. Only reason I ported
> it forward was because it's listed in todo/release.
it's probably always a good idea to discuss first before spending time on
things :)
--
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [Patch mp2] unescape_url_info
Posted by "Philippe M. Chiasson" <go...@ectoplasm.org>.
Stas Bekman wrote:
> Philippe M. Chiasson wrote:
>
>> as per todo/release:
>> Apache->unescape_url{_info}:
>> - not yet implemented. should be moved to Apache::Util (or may be
>> - APR::URI?)
>
>
> I'd think it should live under ModPerl:: since it's not an Apache API
> function. But I'm not sure if it'll serve the best the user to take it
> away from other similar functions.
>
>> unescape_url has already been implemented, so this small patch ports
>> the mp1 code to Apache::URI::unescape_url_info
>
>
> doc?
As usual, I usually try and complete the documentation once the code
gets a go ;-)
>
> also continuing yesterdays story, this function doesn't know to deal
> with utf8 strings, right? in which case we might just as well drop it
> too? is there a CPAN implementation for that functionality?
I am 99.9% sure there is something on CPAN to do that
>> Index: t/response/TestAPI/uri.pm
>
> [...]
>
>> Apache::URI::unescape_url($url_string);
>>
>> - ok $url_string eq "@c";
>> + ok t_cmp("@c", $url_string, "unescape_url");
>> + }
>> +
>> + # unescape_url_info
>> + {
>> + my @c = qw(one two three);
>> + my $url_string = join '+', @c;
>> +
>> + Apache::URI::unescape_url_info($url_string);
>> +
>> + ok t_cmp("@c", $url_string, "unescape_url_info");
>> }
>
>
> 1) the order is wrong, it should be $receveid, $expected
Yup, you are right, I always get these 2 reversed.
> 2) while you are changing this test, it's probably a good idea not
> to rely on $" being set to " ". Better do an explicit join " ", @c.
Good point.
>> Index: xs/Apache/URI/Apache__URI.h
>> ===================================================================
>> RCS file: /home/cvs/modperl-2.0/xs/Apache/URI/Apache__URI.h,v
>> retrieving revision 1.6
>> diff -u -I$Id -r1.6 Apache__URI.h
>> --- xs/Apache/URI/Apache__URI.h 4 Mar 2004 06:01:13 -0000 1.6
>> +++ xs/Apache/URI/Apache__URI.h 6 Oct 2004 23:38:05 -0000
>> @@ -37,3 +37,40 @@
>>
>> return SvPVX(url);
>> }
>> +
>> +#define HEX2DEC(c) (((c) >= 'A') ? (((c) & 0xdf) - 'A')+10 : ((c) -
>> '0'))
>> +static MP_INLINE char *mpxs_Apache__URI_unescape_url_info(pTHX_ SV
>> *url)
>> +{
>> + STRLEN n_a;
>> + char *u = SvPV_force(url, n_a);
>> + char *trans = u;
>> + char digit;
>> + + if (!u || !*u) {
>> + return NULL;
>> + }
>
>
> Wouldn't it be better to croak here instead?
Not really, if you try and unescape_url an undef/empty string, you get
empty back.
> But before you do any tweaks, let's see if we keep it at all.
I'd be more than happy to yank it out completely. Only reason I ported
it forward was
because it's listed in todo/release.
Re: [Patch mp2] unescape_url_info
Posted by Stas Bekman <st...@stason.org>.
Philippe M. Chiasson wrote:
> as per todo/release:
> Apache->unescape_url{_info}:
> - not yet implemented. should be moved to Apache::Util (or may be
> - APR::URI?)
I'd think it should live under ModPerl:: since it's not an Apache API
function. But I'm not sure if it'll serve the best the user to take it
away from other similar functions.
> unescape_url has already been implemented, so this small patch ports the
> mp1 code to Apache::URI::unescape_url_info
doc?
also continuing yesterdays story, this function doesn't know to deal with
utf8 strings, right? in which case we might just as well drop it too? is
there a CPAN implementation for that functionality?
> Index: t/response/TestAPI/uri.pm
[...]
> Apache::URI::unescape_url($url_string);
>
> - ok $url_string eq "@c";
> + ok t_cmp("@c", $url_string, "unescape_url");
> + }
> +
> + # unescape_url_info
> + {
> + my @c = qw(one two three);
> + my $url_string = join '+', @c;
> +
> + Apache::URI::unescape_url_info($url_string);
> +
> + ok t_cmp("@c", $url_string, "unescape_url_info");
> }
1) the order is wrong, it should be $receveid, $expected
2) while you are changing this test, it's probably a good idea not
to rely on $" being set to " ". Better do an explicit join " ", @c.
> Index: xs/Apache/URI/Apache__URI.h
> ===================================================================
> RCS file: /home/cvs/modperl-2.0/xs/Apache/URI/Apache__URI.h,v
> retrieving revision 1.6
> diff -u -I$Id -r1.6 Apache__URI.h
> --- xs/Apache/URI/Apache__URI.h 4 Mar 2004 06:01:13 -0000 1.6
> +++ xs/Apache/URI/Apache__URI.h 6 Oct 2004 23:38:05 -0000
> @@ -37,3 +37,40 @@
>
> return SvPVX(url);
> }
> +
> +#define HEX2DEC(c) (((c) >= 'A') ? (((c) & 0xdf) - 'A')+10 : ((c) - '0'))
> +static MP_INLINE char *mpxs_Apache__URI_unescape_url_info(pTHX_ SV *url)
> +{
> + STRLEN n_a;
> + char *u = SvPV_force(url, n_a);
> + char *trans = u;
> + char digit;
> +
> + if (!u || !*u) {
> + return NULL;
> + }
Wouldn't it be better to croak here instead?
But before you do any tweaks, let's see if we keep it at all.
--
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org