Mailing List Archive: Catalyst: Dev

uri_for method is broken in latest release

Index | Next | Previous | View Threaded

typester at cpan

Jan 15, 2008, 10:14 PM

Post #1 of 7 (1897 views)
Permalink

uri_for method is broken in latest release

Hi,

I noticed that current uri_for returned broken uri when multibyte chars
was passed.
Though I also checked svn repo, it still broken.

Here is a patch and test for it. (both attached)

And one more thing, why current uri_for doesn't do canonical?

In https page, I got 'https://example.com:443/foo/bar' from uri_for.
I think this is ugly.

--
Daisuke Murase <typester [at] cpan>

Attachments:

uri_for_multibytechar.patch (0.52 KB)

uri_for_multibytechar.t (0.74 KB)

dbix-class at trout

Jan 15, 2008, 11:27 PM

Post #2 of 7 (1782 views)
Permalink

Re: uri_for method is broken in latest release [In reply to]

On Wed, Jan 16, 2008 at 03:14:26PM +0900, Daisuke Murase wrote:
> Hi,
>
> I noticed that current uri_for returned broken uri when multibyte chars
> was passed.
> Though I also checked svn repo, it still broken.
>
> Here is a patch and test for it. (both attached)

Brian, can you have a quick play with this and apply it?

> And one more thing, why current uri_for doesn't do canonical?

For RSS/atom feeds etc.

I think 5.80 should probably include an action_uri method that provides
an easier interface to the uri_for($action, ...) style and that will
likely default to not including host/port but provide an option to do so.

If anybody wants to have a go at writing such a method, patches would be
very welcome :)

> In https page, I got 'https://example.com:443/foo/bar' from uri_for.
> I think this is ugly.

--
Matt S Trout Need help with your Catalyst or DBIx::Class project?
Technical Director http://www.shadowcat.co.uk/catalyst/
Shadowcat Systems Ltd. Want a managed development or deployment platform?
http://chainsawblues.vox.com/ http://www.shadowcat.co.uk/servers/

_______________________________________________
Catalyst-dev mailing list
Catalyst-dev [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

brian.cassidy at nald

Jan 16, 2008, 4:18 AM

Post #3 of 7 (1782 views)
Permalink

Re: uri_for method is broken in latest release [In reply to]

Matt S Trout wrote:
> On Wed, Jan 16, 2008 at 03:14:26PM +0900, Daisuke Murase wrote:
>> Hi,
>>
>> I noticed that current uri_for returned broken uri when multibyte chars
>> was passed.
>> Though I also checked svn repo, it still broken.
>>
>> Here is a patch and test for it. (both attached)
>
> Brian, can you have a quick play with this and apply it?

Will do.

-Brian

_______________________________________________
Catalyst-dev mailing list
Catalyst-dev [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

brian.cassidy at nald

Jan 16, 2008, 4:37 AM

Post #4 of 7 (1776 views)
Permalink

Re: uri_for method is broken in latest release [In reply to]

Daisuke Murase wrote:
> Hi,
>
> I noticed that current uri_for returned broken uri when multibyte chars
> was passed.
> Though I also checked svn repo, it still broken.
>
> Here is a patch and test for it. (both attached)

Does this mean that req->uri_with() is also broken?

Patch applied [1]

-Brian

[1] http://dev.catalyst.perl.org/svnweb/Catalyst/revision/?rev=7392

_______________________________________________
Catalyst-dev mailing list
Catalyst-dev [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

claco at chrislaco

Jan 16, 2008, 5:50 AM

Post #5 of 7 (1779 views)
Permalink

Re: uri_for method is broken in latest release [In reply to]

Daisuke Murase wrote:
> Hi,
>
> I noticed that current uri_for returned broken uri when multibyte chars
> was passed.
> Though I also checked svn repo, it still broken.
>
> Here is a patch and test for it. (both attached)
>
>
> And one more thing, why current uri_for doesn't do canonical?
>
> In https page, I got 'https://example.com:443/foo/bar' from uri_for.
> I think this is ugly.
>

Yes, I'm a broken record. Yes, I should really file an RT for this. Yes
I probably just volunteered myself for tests. I'm bitching again here
just so I don't forget. :-)

uri_for needs serious love when it comes to ending slashes.

uri_for('foo', '/') and uri_for($action, '/') both do different things,
and in the end, adding a '/' to both of those methods params usually
just yields foo// instead of foo/

This seems like a bug and writing [% c.uri_for('foo') _ '/' %] seems
silly sometimes imho.

-=Chris

Attachments:

signature.asc (0.18 KB)

typester at cpan

Jan 16, 2008, 6:23 PM

Post #6 of 7 (1782 views)
Permalink

Re: uri_for method is broken in latest release [In reply to]

On Wed, Jan 16, 2008 at 08:37:03 -0400, Brian Cassidy wrote:
> Does this mean that req->uri_with() is also broken?

Right, I did quick review and found it was also broken.
So I wrote a patch again (against trunk 5.70, attached)

The point at this problem is that calling utf8::encode/decode without
checking utf8::is_utf8 is harmful.

--
Daisuke Murase <typester [at] cpan>

Attachments:

uri_with.patch (1.69 KB)

brian.cassidy at nald

Jan 17, 2008, 5:00 AM

Post #7 of 7 (1775 views)
Permalink

Re: uri_for method is broken in latest release [In reply to]

Daisuke Murase wrote:
> On Wed, Jan 16, 2008 at 08:37:03 -0400, Brian Cassidy wrote:
>> Does this mean that req->uri_with() is also broken?
>
> Right, I did quick review and found it was also broken.
> So I wrote a patch again (against trunk 5.70, attached)
>
> The point at this problem is that calling utf8::encode/decode without
> checking utf8::is_utf8 is harmful.

Applied [1].

-Brian

[1] http://dev.catalyst.perl.org/svnweb/Catalyst/revision/?rev=7400

_______________________________________________
Catalyst-dev mailing list
Catalyst-dev [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads