Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InetHeaderMap doesn't support multiple headers with the same name #380

Closed
japplegame opened this issue Nov 8, 2013 · 7 comments
Closed

Comments

@japplegame
Copy link
Contributor

This is actual for Set-Cookie header. It is impossible to parse client request cookies.

@s-ludwig
Copy link
Member

s-ludwig commented Nov 8, 2013

I'll look into it.

@japplegame
Copy link
Contributor Author

It is critical for me and I'm ready to write pull request immediately, but I don't know how to implement this without breaking existing code. I think headers should not be an associative array, but just a list.

@japplegame
Copy link
Contributor Author

May be we need something like InetHeaderList?

@s-ludwig
Copy link
Member

s-ludwig commented Nov 8, 2013

Wait a little. The current behavior is according to the old RFC822 standard, which is to append all equally named headers to a single comma separated list. So you can implement this right now by just splitting the "Cookie" field by ",".

However, since this is now obsolete behavior (multiple occurrences of the same field are not defined anymore in RFC 5322), it makes sense to make a transition to allow multiple fields with the name name in InetHeaderMap. However, this requires a bit of careful testing to make sure nothing breaks (I think nothing depends on the comma separation right now, though). I already have the support for multiple fields ready and will commit as soon as I can make reasonably sure that it will not impact anything else.

@japplegame
Copy link
Contributor Author

So you can implement this right now by just splitting the "Cookie" field by ","

No. I can't. Because comma used in expires field. For example expires=Fri, 08-Nov-2013 15:22:33 GMT

@s-ludwig
Copy link
Member

s-ludwig commented Nov 8, 2013

Damn, you are right. You could still parse it, but not as easy. Anyway, as said I have a full solution almost ready, so that shouldn't be necessary. Get a coffee and check back again ;)

s-ludwig added a commit that referenced this issue Nov 8, 2013
…parseRFC5322Header. See #380.

parseRFC822Header now has an additional rfc822_compatible parameter that keeps the old behavior and is true by default. So for now, existing code behaves the same as before.

The documentation for both entities has been improved and clarified WRT multiple fields.
@japplegame
Copy link
Contributor Author

It works! Many thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants