From: John Napiorkowski Date: Tue, 28 Jul 2015 15:02:27 +0000 (-0500) Subject: proposal for fragment spec X-Git-Tag: 5.90097~4 X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=catagits%2FCatalyst-Runtime.git;a=commitdiff_plain;h=6b9f9ef742c1ee771df336eed1db42d807c8c59c proposal for fragment spec --- diff --git a/lib/Catalyst.pm b/lib/Catalyst.pm index 6b0c066..f55d436 100644 --- a/lib/Catalyst.pm +++ b/lib/Catalyst.pm @@ -1484,11 +1484,11 @@ sub setup_finalize { $class->setup_finished(1); } -=head2 $c->uri_for( $path?, @args?, \%query_values? ) +=head2 $c->uri_for( $path?, @args?, \%query_values?, $fragment? ) -=head2 $c->uri_for( $action, \@captures?, @args?, \%query_values? ) +=head2 $c->uri_for( $action, \@captures?, @args?, \%query_values?, $fragment? ) -=head2 $c->uri_for( $action, [@captures, @args], \%query_values? ) +=head2 $c->uri_for( $action, [@captures, @args], \%query_values?, $fragment? ) Constructs an absolute L object based on the application root, the provided path, and the additional arguments and query parameters provided. @@ -1550,6 +1550,8 @@ sub uri_for { undef($path) if (defined $path && $path eq ''); + my $fragment = ((scalar(@args) && ref($args[-1]) eq 'SCALAR') ? pop @args : undef ); + my $params = ( scalar @args && ref $args[$#args] eq 'HASH' ? pop @args : {} ); @@ -1631,15 +1633,6 @@ sub uri_for { } my $query = ''; - - # remove and save fragment if there is one - my $fragment; - if ($args =~ s/#(.+)$//) { - $fragment = encode_utf8($1); - $fragment =~ s/([^A-Za-z0-9\-_.!~*'() ])/$URI::Escape::escapes{$1}/go; - $fragment =~ s/ /+/g; - } - if (my @keys = keys %$params) { # somewhat lifted from URI::_query's query_form $query = '?'.join('&', map { @@ -1669,8 +1662,12 @@ sub uri_for { $args = encode_utf8 $args; $args =~ s/([^$URI::uric])/$URI::Escape::escapes{$1}/go; - # re-attach fragment on the end of everything after adding params - $query .= "#$fragment" if $fragment; + if(defined $fragment) { + $fragment = encode_utf8(${$fragment}); + $fragment =~ s/([^A-Za-z0-9\-_.!~*'() ])/$URI::Escape::escapes{$1}/go; + $fragment =~ s/ /+/g; + $query .= "#$fragment"; + } my $res = bless(\"${base}${args}${query}", $class); $res; diff --git a/lib/Catalyst/Delta.pod b/lib/Catalyst/Delta.pod index a47c884..e75a052 100755 --- a/lib/Catalyst/Delta.pod +++ b/lib/Catalyst/Delta.pod @@ -7,6 +7,21 @@ Catalyst::Delta - Overview of changes between versions of Catalyst This is an overview of the user-visible changes to Catalyst between major Catalyst releases. +=head2 VERSION 5.90097 + +=head3 Defined how $c->uri_for adds a URI fragment. + +We now have a specification for creating URIs with fragments (or HTML anchors). +Previously you could do this as a side effect of how we create URIs but this +side effect behavior was never documented or tested, and was broken when we +introduced default UTF-8 encoding. When creating URIs with fragments please +follow the new, supported specification: + + $c->uri_for($action_or_path, \@captures_or_args, @args, \$query, \$fragment); + +This will be a breaking change for some codebases, we recommend testing if +you are creating URLs with fragments. + =head2 VERSION 5.90094 =head3 Multipart form POST with character set headers diff --git a/lib/Catalyst/Upgrading.pod b/lib/Catalyst/Upgrading.pod index 094191d..6055442 100644 --- a/lib/Catalyst/Upgrading.pod +++ b/lib/Catalyst/Upgrading.pod @@ -2,7 +2,44 @@ Catalyst::Upgrading - Instructions for upgrading to the latest Catalyst -=head1 Upgrading to Catalyst 5.90100 +=head1 Upgrading to Catalyst 5.90097 + +In older versions of Catalyst one could construct a L with a fragment (such as +https://localhost/foo/bar#fragment) by using a '#' in the path or final argument, for +example: + + $c->uri_for('/mypath#fragment'); + +or: + + $c->uri_for($action, 'foo#fragment'); + +This behavior was never documented and would break if using the Unicode plugin, or when +adding a query to the arguments: + + $c->uri_for($action, 'foo#fragment', +{ a=>1, b=>2}); + +would define a fragment like "#fragment?a=1&b=2". + +When we introduced UTF-8 encoding by default in Catalyst 5.9008x this side effect behavior +was broken since we started encoding the '#' when it was part of the URI path. + +In version 5.90095 and 5.90096 we attempted to fix this, but all we managed to do was break +people with URIs that included '#' as part of the path data, when it was not expected to +be a fragment delimiter. + +In general L prefers an explicit specification rather than relying on side effects +or domain specific mini languages. As a result we are now defining how to set a fragment +for a URI via ->uri_for: + + $c->uri_for($action_or_path, \@captures_or_args, @args, \$query, \$fragment); + +If you are relying on the previous side effect behavior your URLs will now encode the '#' +delimiter, which is going to be a breaking change for you. You need to alter your code +to match the new specification or modify uri_for for your local case. Patches to solve +this are very welcomed, as long as they don't break existing test cases. + +=head1 Upgrading to Catalyst 5.90095 The method C in L was actually returning the first error. This has been fixed but there is a small chance it could be a breaking issue for you. If this gives diff --git a/t/aggregate/unit_core_uri_for.t b/t/aggregate/unit_core_uri_for.t index 453d6e0..2f4fb11 100644 --- a/t/aggregate/unit_core_uri_for.t +++ b/t/aggregate/unit_core_uri_for.t @@ -61,24 +61,30 @@ is( is( Catalyst::uri_for( $context, '/bar#fragment', { param1 => 'value1' } )->as_string, - 'http://127.0.0.1/foo/bar?param1=value1#fragment', + 'http://127.0.0.1/foo/bar%23fragment?param1=value1', 'URI for path with fragment and query params 1' ); is( Catalyst::uri_for( $context, '/bar#fragment\x{2620}', { param1 => 'value1' } )->as_string, - 'http://127.0.0.1/foo/bar?param1=value1#fragment%5Cx%7B2620%7D', + 'http://127.0.0.1/foo/bar%23fragment%5Cx%7B2620%7D?param1=value1', 'URI for path with fragment and query params 2' ); is( Catalyst::uri_for( $context, '/bar#fragment^%$', { param1 => 'value1' } )->as_string, - 'http://127.0.0.1/foo/bar?param1=value1#fragment%5E%25%24', + 'http://127.0.0.1/foo/bar%23fragment%5E%$?param1=value1', 'URI for path with fragment and query params 3' ); +is( + Catalyst::uri_for( $context, '/bar', { param1 => 'value1' }, \'fragment\x{2620}' )->as_string, + 'http://127.0.0.1/foo/bar?param1=value1#fragment%5Cx%7B2620%7D', + 'URI for path with fragment and query params 2' +); + # test with utf-8 is( diff --git a/t/utf_incoming.t b/t/utf_incoming.t index 2adccb4..5f12ecb 100644 --- a/t/utf_incoming.t +++ b/t/utf_incoming.t @@ -323,7 +323,7 @@ use Catalyst::Test 'MyApp'; is $res->code, 200, 'OK'; is decode_utf8($res->content), "$url", 'correct body'; #should do nothing is $res->content, "$url", 'correct body'; - is $res->content_length, 102, 'correct length'; + is $res->content_length, 104, 'correct length'; is $res->content_charset, 'UTF-8'; {