From: Tomas Doran <bobtfish@bobtfish.net>
Date: Wed, 27 Jan 2010 21:04:17 +0000 (+0000)
Subject: Fix / => %2F in uri_for so that it only works for action objects as previously advert... 
X-Git-Tag: 5.80019~5
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=catagits%2FCatalyst-Runtime.git;a=commitdiff_plain;h=2689f8a4d4914dc316857f50eea2cf03788b7030

Fix / => %2F in uri_for so that it only works for action objects as previously advertised as this has been screwing people. Also fix multiple / => %2F conversion in the same arg/capture
---

diff --git a/Changes b/Changes
index f68cba7..d2d16d6 100644
--- a/Changes
+++ b/Changes
@@ -1,5 +1,17 @@
 # This file documents the revision history for Perl extension Catalyst.
 
+  Bug fixed:
+   - Calls to $c->uri_for with private paths as strings (e.g.
+     $c->uri_for('controller/action', 'arg1', 'arg2') ) no longer have
+     / encoded to %2F. This is due to $c->uri_for('static', 'css/foo', $bar)
+     which should not be encoded.
+     Calls with an action object (rather than a string), or uri_for action
+     will still encode / in args and captures to %2F
+
+   - The above noted / => %2F encoding in uri_for_action or uri_for with
+     an action object has been fixed to not just encode the first slash in
+     any set of args/captures.
+
   New features:
    - Allow passing additional arguments to action constructors.
 
diff --git a/lib/Catalyst.pm b/lib/Catalyst.pm
index 0165a44..1c1102d 100644
--- a/lib/Catalyst.pm
+++ b/lib/Catalyst.pm
@@ -1259,8 +1259,19 @@ sub uri_for {
         $path .= '/';
     }
 
+    undef($path) if (defined $path && $path eq '');
+
+    my $params =
+      ( scalar @args && ref $args[$#args] eq 'HASH' ? pop @args : {} );
+
+    carp "uri_for called with undef argument" if grep { ! defined $_ } @args;
+    s/([^$URI::uric])/$URI::Escape::escapes{$1}/go for @args;
+    if (blessed $path) { # Action object only.
+        s|/|%2F|g for @args;
+    }
+
     if ( blessed($path) ) { # action object
-        my $captures = [ map { s|/|%2F|; $_; }
+        my $captures = [ map { s|/|%2F|g; $_; }
                         ( scalar @args && ref $args[0] eq 'ARRAY'
                          ? @{ shift(@args) }
                          : ()) ];
@@ -1274,15 +1285,6 @@ sub uri_for {
         $path = '/' if $path eq '';
     }
 
-    undef($path) if (defined $path && $path eq '');
-
-    my $params =
-      ( scalar @args && ref $args[$#args] eq 'HASH' ? pop @args : {} );
-
-    carp "uri_for called with undef argument" if grep { ! defined $_ } @args;
-    s/([^$URI::uric])/$URI::Escape::escapes{$1}/go for @args;
-    s|/|%2F| for @args;
-
     unshift(@args, $path);
 
     unless (defined $path && $path =~ s!^/!!) { # in-place strip
diff --git a/t/aggregate/live_component_controller_action_chained.t b/t/aggregate/live_component_controller_action_chained.t
index fef26ef..b0be898 100644
--- a/t/aggregate/live_component_controller_action_chained.t
+++ b/t/aggregate/live_component_controller_action_chained.t
@@ -1085,7 +1085,8 @@ sub run_tests {
             'request ' . $path . ' ok');
         # Just check that the path matches, as who the hell knows or cares
         # where the app is based (live tests etc)
-        ok( index($content, $path) > 1, 'uri can round trip through uri_for' );
+        ok( index($content, $path) > 1, 'uri can round trip through uri_for' )
+            or diag("Expected $path, got $content");
     }
 }
 
diff --git a/t/aggregate/unit_core_uri_for.t b/t/aggregate/unit_core_uri_for.t
index e70ac55..da40bea 100644
--- a/t/aggregate/unit_core_uri_for.t
+++ b/t/aggregate/unit_core_uri_for.t
@@ -1,16 +1,17 @@
 use strict;
 use warnings;
-
+use FindBin qw/$Bin/;
+use lib "$FindBin::Bin/../lib";
 use Test::More;
 use URI;
 
-use_ok('Catalyst');
+use_ok('TestApp');
 
 my $request = Catalyst::Request->new( {
                 base => URI->new('http://127.0.0.1/foo')
               } );
-
-my $context = Catalyst->new( {
+my $dispatcher = TestApp->dispatcher;
+my $context = TestApp->new( {
                 request => $request,
                 namespace => 'yada',
               } );
@@ -144,12 +145,20 @@ TODO: {
               "uri_for() doesn't mess up query parameter hash in the caller");
 }
 
-# 5.80018 is only encoding the first of the / in the arg. See line 1271.
-is(
-    Catalyst::uri_for( $context, 'controller/action', 'foo/bar/baz' )->as_string,
-    'http://127.0.0.1/controller/action/foo%2Fbar%2Fbaz',
-    'Escape both forward slashes in the arg as %2F'
-);
+
+{
+    my $path_action = $dispatcher->get_action_by_path(
+                       '/action/path/six'
+                     );
+
+    # 5.80018 is only encoding the first of the / in the arg.
+    is(
+        Catalyst::uri_for( $context, $path_action, 'foo/bar/baz' )->as_string,
+        'http://127.0.0.1/action/path/six/foo%2Fbar%2Fbaz',
+        'Escape all forward slashes in args as %2F'
+    );
+}
+
 
 done_testing;