Opened 9 years ago

Closed 9 years ago

#24463 closed Cleanup/optimization (fixed)

Remove old functionality from `HttpRequest`

Reported by: Rik Owned by: nobody
Component: HTTP handling Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The scheme() method on HttpRequest returns the scheme the request was on, so http or https. If you look at the implementation of the method:

https://github.com/django/django/blob/master/django/http/request.py#L163

You see that it checks a Django setting and if that doesn't result in https, it falls back to the _get_scheme() method.

Now if you look at the _get_scheme() method:

https://github.com/django/django/blob/master/django/http/request.py#L159

You see it checks if the environment variable "HTTPS" is set to "on". I think is old functionality that doesn't belong in Django anymore. It was probably from the time mod_python was used. These days this functionality is implemented by WSGI:

https://www.python.org/dev/peps/pep-3333/#environ-variables

(search for HTTPS=on)

Also the WSGI handler in Django overwrites the _get_scheme():

https://github.com/django/django/blob/master/django/core/handlers/wsgi.py#L115

And since every HTTP request in Django goes through WSGI, the _get_scheme() method in HttpRequest would never be used.

So I think we should remove this functionality from HttpRequest, to prevent confusion, because when I stumbled upon this piece of code it confused me quite a bit and it took me some time to find out what it might be.

Change History (10)

comment:1 by Collin Anderson, 9 years ago

Personally, I think we should merge all of HttpRequest and WSGIRequest into one class, although currently it looks like HttpRequest is much easier to create requests objects manually.

comment:2 by Rik, 9 years ago

@collinanderson, what's the use case for creating HttpRequest objects manually? And why would it be harder if we merge WSGIRequest into HttpRequest?

comment:3 by Collin Anderson, 9 years ago

If you have some code running outside of a request that wants a request object, you can create a dummy request just by creating an HttpRequest(). It appears that WSGIRequest.init requires a dictionary to be passed in. Maybe that could be made optional?

It also looks like HttpRequest's GET and POST are mutable by default. (Again, that makes sense for hand-crafted HttpRequests.)

comment:4 by Rik, 9 years ago

If you have code running outside a request that needs a request object, it seems to me like a flaw in the design of the program. However, I can understand there are people who have this anyway, and we should try to not break their code by changing this.

About the mutability of GET and POST, what do you mean by that? In Python, any property of a class is mutable.

comment:5 by Collin Anderson, 9 years ago

I'm looking at HttpRequest.__init__. By default, you'll get an error if you try to add or remove items from the request.GET and request.POST dict-likes.

comment:6 by Rik, 9 years ago

Ok, in that case I guess we should keep WSGIRequest and HttpRequest separate. That might also make sense in the case WSGI might ever be replaced or alternatives arise.

Back to the core issue here: could we remove the checking of the environment variable in _get_scheme() in HttpRequest?

comment:7 by Collin Anderson, 9 years ago

Hmm... It would be nice if HttpRequest().scheme worked without an error. Maybe we could simply have it return 'http' and then have a comment saying it's overridden in WSGIRequest?

comment:8 by Rik, 9 years ago

Yeah, that would make sense to me. It would prevent a lot of confusion. Will submit a pull request in a bit.

comment:10 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: newclosed

In ccff08c1:

Fixed #24463 -- Removed mod_python functionality from HttpRequest._get_scheme()

Note: See TracTickets for help on using tickets.
Back to Top