getkirby-v2 / toolkit

This is the deprecated toolkit for Kirby v2.
http://getkirby.com
81 stars 50 forks source link

Combine multiple HTTP header fields in remote, most BC solution. #235

Closed Zegnat closed 7 years ago

Zegnat commented 7 years ago

This fixes #234. At least a little.

Per RFC 2616, section 4.2 headers that share the same field-name can be combined by appending subsequent field-values as comma separated values.

There are a lot of nuances being skipped in the Kirby Toolkit implementation. E.g. header field names are case-insensitive so Link and linK are the same. The current implementation doesn’t do anything about this and neither does this fix.

A better fix might have been to create a sub-array with all the case-normalised headers that came in under a field name. That would make sure weird header formats like Set-Cookie don’t horribly break comma separated values. The reason I didn’t do this is because it would change the API in a BC-breaking way.

All this fix is doing is naïvely making sure no data is discarded. The array structures are left as they are, getting around any API changes and BC breaks.

lukasbestle commented 7 years ago

Thanks for your pull request, but I think we will go the array route instead. It makes more sense syntactically. Thanks for pointing out the case-insensitivity issue, we will fix this as well.

Zegnat commented 7 years ago

I would be willing to file a new PR that returns headers as an array, if that would be the best way to get a fix landed quickly. If you want to fix case-insensitivity someone will have to make a call on how they want to normalise header names though. Maybe all uppercase, much like PHP’s “HTTP_HEADERFIELD”s in $_SERVER?

lukasbestle commented 7 years ago

Feel free to send a PR that returns the headers as an array, but please make sure that an array only gets generated if there is actually more than one header of that type.

Regarding normalisation: You are right, that's a bit complicated. I would have said "take the normalization of the first found header in the input data", but then the method isn't predictable anymore. So that's not great. Maybe we should leave out this feature for now.

Zegnat commented 7 years ago

Just to be clear, this would be the expected result?

In:

Link: </post_token>; rel="token_endpoint"
Link: </webmention>; rel="webmention"
linK: </style.css>; rel="stylesheet"

Out:

[
    'Link' => [
        '</post_token>; rel="token_endpoint"',
        '</webmention>; rel="webmention"'
    ],
    'linK' => '</style.css>; rel="stylesheet"'
]

That is: no normalisation, only use an array when we have duplicates. I’ll send in a PR later today.