replace LWS with a single SP in header values

[punytan]

:)

[miyagawa]

OK, merged. Did we need the URI->new stuff for Location when replacing LWS with SP is sufficient?

[punytan]

Query parameters in URI should be percent encoded before finalize is called because it can contain %0D, %0A or something else.
Without URI->new->as_string, the passed URI will be broken by replacement of LWS, \r and \n.

And, we can't recognize whether we should pass percent encoded parameters or not from the documentation of redirect and location.
A problem will occur again: We need to re-implement redirect and location method in some framework that inherits Plack::Response.

If we omit this process, I expect framework developers should implement the same code.

[miyagawa]

Without URI->new->as_string, the passed URI will be broken by replacement of LWS, \r and \n

That's fine, because you've given an invalid URI in the first place. What is more important here is to not generate invalid PSGI headers (because newlines in header values is invalid in PSGI), rather than not generate invalid URI.

If we omit this process, I expect framework developers should implement the same code.

Yeah, but then we could better just kill the redirect() and location() method instead altogether.

I really, really hate to put framework-like code inside Plack, and this is why I separated the Plack::Request and Response from the core dist, and then writing middleware becomes kind of painful, so i merged it back, but i guess it's time to do it again. sigh

[miyagawa]

And, we can't recognize whether we should pass percent encoded parameters or not from the documentation of redirect and location.

Fair point, but is it then also not obvious (or at least, undocumented either) where: $r->location($url) would normalize the URL but $r->header(Location => $url) wouldn't? At least, the document of the location method is clear - it sets/gets Location header. Doesn't say anything about normalize the given URI.

[miyagawa]

We need to re-implement redirect and location method in some framework that inherits Plack::Response.

I honestly don't believe it. Let's say we have a $foo variable that contains some binary variables, such as UTF-8 bytes for "テスト". Of course it might contain newlines, too.

$res->header("X-Foo" => $foo);
$res->header("Location" => "http://example.com/?foo=$foo");
$res->location("http://example.com/?foo=$foo");

To me it does sound a little weird, or at least inconsistent, that only the location method normalizes the given string.

You're right that most frameworks need to implement the method to abstract this, but I'm sure it would be different from redirect or location - it would actually be more like uri_for - that takes parameters in hash/list and encodes them appropriately, and then pass to the location method etc.

$res->redirect( $self->uri_for("/action", { foo => $foo }) )

I confirmed this with Catalyst - Catalyst::Response doesn't really special-case redirect method.

[punytan]

That's fine, because you've given an invalid URI in the first place. What is more important here is to not generate invalid PSGI headers (because newlines in header values is invalid in PSGI), rather than not generate invalid URI.

I agree. You convinced me that you are right.
That was my misunderstanding.

Yeah, but then we could better just kill the redirect() and location() method instead altogether.

I think that deleting redirect() and location() methods is not so bad substitution if you hate to put framework-like code.

[miyagawa]

Great that we found an agreement :) I added a note to make the documentation less confusing just in case.

Just for the note: the current code still makes a room for developers to give invalid URI to the Location field, but it's totally fine - you can also give non-integer value to Content-Length, or invalid MIME type to Content-Type, and so on - it is not Plack::Response's responsibility to validate them.

[punytan]

Just for the note: the current code still makes a room for developers to give invalid URI to the Location field, but it's totally fine

Yep, that's fine :)