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

Support an optional charset parameter in Content-Type on requests #874

Closed
wants to merge 2 commits into from

Conversation

ethanresnick
Copy link
Member

A fix for #837.

@tkellen
Copy link
Member

tkellen commented Sep 9, 2015

I'm 👍 to this (needs to be added to the normative statements json file tho).

@ethanresnick
Copy link
Member Author

Awesome. I'll add it to the json file too. In that file, can I change an existing id, or will that break some tests? Specifically, I was thinking of breaking up the request-content-type statement into two normative statements (request-base-content-type and request-content-type-params) since we now have two normative statements in the text. But if that will cause problems, I can just add the new normative sentence to the request-content-type statement.

@tkellen
Copy link
Member

tkellen commented Sep 10, 2015

We have not yet implemented a complete test suite using this, so i think it's okay to change the ID in this instance. In the future it'll be really important that we don't, though.

@ethanresnick
Copy link
Member Author

@tkellen Ok cool. I changed the id and pushed the normative json.

@tkellen
Copy link
Member

tkellen commented Sep 15, 2015

@dgeb, you good with this?

@ethanresnick
Copy link
Member Author

@tkellen One thing to note before merging, even if @dgeb gives a +1 on the text, is that we should probably merge this change into the 1.1 draft (once that's live), rather than putting it directly into the 1.0 text, like the PR currently does. I'll update the PR once the versioning goes live.

@ethanresnick
Copy link
Member Author

@tkellen @dgeb Ok guys, this is now rebased against version 1.1

@ethanresnick
Copy link
Member Author

Given that Firefox is about to fix this bug, and it's the last client that has this problem, I'm thinking we should instead merge a narrower version that allows the server to ignore charset but doesn't allow the client to (compliantly) send it.

@tkellen
Copy link
Member

tkellen commented Sep 21, 2015

Works for me!

@tkellen
Copy link
Member

tkellen commented Sep 21, 2015

Seems like we might want to mention this in the FAQ as well?

@ethanresnick
Copy link
Member Author

Ok, I've rebased this all to be the minimal workable change. Lmk if it's good to merge.

Re FAQs: I'm thinking this problem will probably go away fast enough on it's own that it may not be worth it to give it an FAQ item (that we'd just have to remove later). But if we find that people keep asking, I'm definitely up for adding one.

@ethanresnick
Copy link
Member Author

Ping @dgeb

@dgeb
Copy link
Member

dgeb commented Sep 26, 2015

My only nit is in the line:

Servers SHOULD accept requests that include the charset parameter but SHOULD ignore its value.

The clause "but SHOULD ignore its value" feels like an overreach. If we're allowing the charset parameter then shouldn't we allow it to be respected?

@dgeb
Copy link
Member

dgeb commented Sep 26, 2015

I am not sure that any of this is strictly necessary. It seems like the spec could happily steer clear of any of this. Firefox clients could temporarily be non-compliant, and servers could helpfully provide some wiggle room in their compliance check. Very soon this won't be an issue for anyone. I don't see a reason to formalize any of this in the spec unless we consider it desirable on its own merits.

@ethanresnick
Copy link
Member Author

I am not sure that any of this is strictly necessary... servers could helpfully provide some wiggle room in their compliance check

Right, the reason to add this to the base spec is that, without it, a server can't provide wiggle room without becoming non-compliant itself. So allowing that wiggle room on servers is what this does, but it still leaves bad clients technically non-compliant.

If we're allowing the charset parameter then shouldn't we allow it to be respected?

The reason I said it should be ignored is because the charset for valid JSON can be (more reliably) determined just from the content itself. (See #889.)

@dgeb
Copy link
Member

dgeb commented Sep 26, 2015

Since we're now targeting 1.1 with this change, and it is directly related to a clause in place to make the spec future proof for extensions (also targeting 1.1), maybe we can arrive at a joint solution that is inclusive of specific media type parameters and ignores others?

Even if we can't, is there any harm in waiting on this until we decide the extension mechanism?

@ethanresnick
Copy link
Member Author

maybe we can arrive at a joint solution that is inclusive of specific media type parameters and ignores others?

Maybe...if we do that, we'd be saying that we'll never add another media type parameter, but maybe that's fine. It simply would simplify processing logic!

Even if we can't, is there any harm in waiting on this until we decide the extension mechanism?

No, there's no harm in that! I'm happy to wait :)

@dgeb
Copy link
Member

dgeb commented Sep 26, 2015

No, there's no harm in that! I'm happy to wait :)

Thanks @ethanresnick :)

@ethanresnick
Copy link
Member Author

I'm going to give this a close (and open a new PR to fix the normative-statements typo), as Firefox fixed their bug months ago and no one has brought this up as an issue since.

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

Successfully merging this pull request may close these issues.

3 participants