Skip to content
This repository was archived by the owner on Mar 24, 2026. It is now read-only.

Verification change for charset#1810

Merged
msmith-techempower merged 2 commits intomasterfrom
text-html-charset
Jan 7, 2016
Merged

Verification change for charset#1810
msmith-techempower merged 2 commits intomasterfrom
text-html-charset

Conversation

@ssmith-techempower
Copy link
Copy Markdown
Contributor

Previously, we would flag responses that set a charset as warnings as this extra header data was unnecessary and may impact performance. After some internal discussions, it's been decided that the charset should be considered mandatory for text/html, which is the content type for the fortunes test. This PR changes the verification script to reflect this. Failure to include charset for text/html or including charset for other content types is flagged as a warning.

@NateBrady23
Copy link
Copy Markdown
Member

   WARN for http://127.0.0.1:8080/plaintext

     Content encoding found "text/plain; charset=utf-8" where "text/plain" is acceptable.
     Additional response bytes may negatively affect benchmark performance.
     See http://frameworkbenchmarks.readthedocs.org/en/latest/Project-Information/Framework-Tests/#specific-test-requirements

@ssmith-techempower then this should not be a warning, correct?

Edit: Ah, maybe I have misunderstood. I assumed we were taking off the warning for all text responses, including text/plain as it has the same implications on browser security as text/html.

@msmith-techempower
Copy link
Copy Markdown
Member

@nbrady-techempower I do not remember the specifics, but the comment in the code is what was decided upon: # "text/html" requires charset to be set. The others do not I think you're right.

@ssmith-techempower I think you need to remove this warning as it is now a requirement and should therefore be an error.

…erification will now accept the correct content type regardless of whether or not charset is included
@ssmith-techempower
Copy link
Copy Markdown
Contributor Author

After some internal discussion, we've decided that charset should be optional for plaintext, so we will consider both "text/plain" and "text/plain; charset=utf-8" as being correct.

@msmith-techempower
Copy link
Copy Markdown
Member

Hard to know who this would affect, but since most of the tests we expect to pass are, I say LGTM!

msmith-techempower added a commit that referenced this pull request Jan 7, 2016
@msmith-techempower msmith-techempower merged commit d854b89 into master Jan 7, 2016
@NateBrady23 NateBrady23 deleted the text-html-charset branch June 14, 2016 21:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants