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

Ensure all WSGITransport environs have a SERVER_PROTOCOL #2708

Merged
merged 1 commit into from May 19, 2023

Conversation

lazorchakp
Copy link
Contributor

@lazorchakp lazorchakp commented May 18, 2023

Summary

  • I noticed that WSGI environs sent to applications with httpx.Client(app=app) did not contain a SERVER_PROTOCOL
  • PEP-3333 indicates that this key must be present in the WSGI environ, and popular WSGI frameworks (such as webob) assume it will be as well
  • It looks like httpx defaults to using HTTP/1.1 for a nearly identical case in the ASGITransport, so I did the same here
  • I'm more than happy to discuss any of this further!

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.
    • to my knowledge, this does not require a documentation update

@lazorchakp lazorchakp marked this pull request as ready for review May 18, 2023 20:51
Copy link
Contributor

@zanieb zanieb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for contributing.

Another maintainer may weigh in but it seems unlikely that this was intentionally excluded.

@lazorchakp
Copy link
Contributor Author

Great, thanks! I'll give it a day or so before merging to see if anyone has other thoughts

@tomchristie
Copy link
Member

valid.

@tomchristie tomchristie merged commit abb994c into encode:master May 19, 2023
5 checks passed
@tomchristie
Copy link
Member

Process improvements we can make here...

  • @tomchristie - Thanks for the quick merge, though you don't need to always play that role. If you could allow other maintainers more space here too that'd probs be good for the team as a whole.
  • @encode/maintainers - We ought to start making sure we include CHANGELOG changes as part of functional or bugfix PRs. That'll help ensure that we're properly describing the change from a user perspective, and will also make the release process simpler and less error-prone. We should probably update the pull request template to include this as an item?
  • @lazorchakp would you consider opening a PR to add this to the CHANGELOG under a new "development" heading?

@Kludex
Copy link
Sponsor Member

Kludex commented May 19, 2023

Yes. 👍

@lazorchakp lazorchakp deleted the wsgi-server-protocol branch May 19, 2023 14:12
@trim21 trim21 mentioned this pull request Aug 1, 2023
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.

None yet

4 participants