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

Vendor cgi.FieldStorage #54

Closed
wants to merge 2 commits into from

Conversation

cjwatson
Copy link
Contributor

@cjwatson cjwatson commented Aug 4, 2020

This is approximately cgi.FieldStorage from Python 3.8, but adjusted to be compatible with both Python 2 and 3, and with some parts removed that BrowserRequest doesn't need.

Fixes #39.

I also applied the patch from python/cpython#21457 to fix parsing of simple request bodies with Content-Length and no Content-Disposition.

My preference would be to apply #55 instead of this, which I think is much easier to follow and much less code to maintain. However, since the issue has been a bit controversial, I'm posting both so that other people can see what they think.

This is approximately cgi.FieldStorage from Python 3.8, but adjusted to
be compatible with both Python 2 and 3, and with some parts removed that
BrowserRequest doesn't need.

Fixes zopefoundation#39.
Copy link
Member

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

LGTM, which is not meant to imply I prefer this approach to #55.

I didn't actually read all the code in _fieldstorage.py. I tried, but my eyes gazed over.


# The Python 2.6 cgi module mixes the query string and POST values
# together. We do not want this.
env = self._environ
if self.method == 'POST' and self._environ['QUERY_STRING']:
if self.method == 'POST' and 'QUERY_STRING' in self._environ:
Copy link
Member

Choose a reason for hiding this comment

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

(Note: my comments here are not meant to imply that I prefer this approach to #55)

The comment above about what Python 2.6 cgi module does could be updated to refer to what our vendored copy of the Python 3.8 cgi module does.

@jugmac00
Copy link
Member

I also applied the patch from python/cpython#21457 to fix parsing of simple request bodies with Content-Length and no Content-Disposition.

There seems to be a problem with the mentioned patch:

python/cpython#21457 (comment)

@icemac
Copy link
Member

icemac commented Oct 16, 2020

According to the contributor agreement vendoring code is not so easy. It reads: "It is assumed that you wrote the software or changes yourself; if other people wrote or changed parts, we will likely need Assignment Agreements from them as well."
So prefer #55, too.

@cjwatson
Copy link
Contributor Author

I'm withdrawing this in favour of #55.

@cjwatson cjwatson closed this Nov 20, 2020
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.

cgi is going to be deprecated in Python 3.8 and removed in 3.10
4 participants