#8381 closed (fixed)
Common middleware should use path_info
| Reported by: | Joost Cassee | Owned by: | nobody |
|---|---|---|---|
| Component: | Uncategorized | Version: | dev |
| Severity: | Keywords: | middleware | |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
The CommonMiddleware works its magic on request.path. Since changeset [8015] this should be request.path_info. Trivial patch attached.
Attachments (2)
Change History (8)
by , 16 years ago
| Attachment: | 8381-r8423.diff added |
|---|
comment:1 by , 16 years ago
| milestone: | → 1.0 |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
comment:2 by , 16 years ago
Yes, I am using these changes on my own Django installation. The website uses my localeurl application that modifies request.path_info to strip a language prefix, so I run into these things. :-) This patch (and the one from #8376) work for me.
comment:3 by , 16 years ago
| Patch needs improvement: | set |
|---|
This patch doesn't seem to be completely correct yet. The problem is when the new URL is constructed, it doesn't include the script name portion any longer. So the redirect (and any other uses of it) will be to the wrong location. Somewhere in the construction of newurl I would expect to see request.META['SCRIPT_NAME'], or, even better, a reuse of request.path once you've determined it does need a trailing slash added.
I suspect the solution might actually be as simple as not making the change on line 46 and only needing to make the one on line 57. But could you check that out, please? If you get to the redirect line, it looks like your current patch will lose the script name in the redirect. That shouldn't happen.
comment:4 by , 16 years ago
| Patch needs improvement: | unset |
|---|
You are correct. I was testing the patch on my non-SCRIPT_NAME testing system. Thanks for your sharp eye!
Your solution is almost correct, I just had to fix the resolve check for the new url. I have updated the patch and tested it on my SCRIPT_NAME-enabled testing system.
by , 16 years ago
| Attachment: | 8381-r8423-2.diff added |
|---|
comment:5 by , 16 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
Have you actually tested this on a system with a non-empty SCRIPT_NAME or are you generating these patches just from code auditing? They need to be tested on an actual install before committing. I realise you can't write anything for Django's test suite for this -- such tests would be too fake to actually convey any confidence in affirming real-world behviour -- but can you at least confirm you've tested it manually.