Opened 17 years ago
Closed 13 years ago
#6213 closed Bug (fixed)
PREPEND_WWW and APPEND_SLASH settings don't work with flatpages middleware
Reported by: | Jason Davies | Owned by: | orthagonal |
---|---|---|---|
Component: | contrib.flatpages | Version: | dev |
Severity: | Normal | Keywords: | APPEND_SLASH PREPEND_WWW flatpages easy-pickings |
Cc: | semente@…, xarquonster@…, jm.bugtracking@…, che@…, sjl | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
The PREPEND_WWW
and APPEND_SLASH
settings don't force a redirect when FlatpageFallbackMiddleware
is being used.
This is because a redirect only happens if the target URL can be resolved using urlresolvers.resolve()
in the code.
This behaviour has occurred since the change in [6852].
If the target URL cannot be resolved immediately using urlresolvers.resolve()
, perhaps we should check in process_response()
to make absolutely sure the response status code will be 404? If some intermediate middleware, e.g. FlatpageFallbackMiddleware
has returned a successful response code, we can then issue a redirect as per the old behaviour.
Attachments (4)
Change History (34)
comment:1 by , 17 years ago
Summary: | PREPEND_WWW and APPEND_SLASH settings don't work with flatpages middleware → PREPEND_WWW setting doesn't work with flatpages middleware |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 17 years ago
Ah, I didn't see #6167, but I don't think it's exactly the same: this ticket concerns the use of FlatpageFallbackMiddleware
whereas #6167 seems to concern the use of the flatpage view (at least from looking at the patch).
It's actually a more general problem, in that if a middleware returns a successful response code (even when no view is mapped to the current URL), then ideally we should honour the APPEND_SLASH
and PREPEND_WWW
settings and issue a redirect.
comment:3 by , 17 years ago
Correct, they are different issues (hence why I didn't close).
To make APPEND_SLASH always append a slash (on a successful response code) goes against the new behaviour. It should only try the slash if it 404s.
Sure, fix PREPEND_WWW like that though (even though I think it's a stupid setting that should belong to a web server, not the Django framework).
comment:4 by , 17 years ago
Ah yes, what I should have said is that in CommonMiddleware
's process_response()
we should append a slash if it 404s and APPEND_SLASH
is set.
The problem is that if you use FlatpageFallbackMiddleware
, redirects do not happen even if they did before the change in [6852], as the change relies on being able to resolve the URL using the URLconf.
For example, if the URL "/foo"
isn't found by FlatpageFallbackMiddleware
, we should still redirect if APPEND_SLASH
is set.
by , 17 years ago
Attachment: | common_middleware.py.diff added |
---|
Patch for CommonMiddleware to fix handling of APPEND_SLASH and PREPEND_WWW
comment:5 by , 17 years ago
Summary: | PREPEND_WWW setting doesn't work with flatpages middleware → PREPEND_WWW and APPEND_SLASH settings don't work with flatpages middleware |
---|
comment:6 by , 17 years ago
Changed the summary back since this ticket still covers the APPEND_SLASH problem.
comment:8 by , 17 years ago
Has patch: | set |
---|---|
Needs tests: | set |
From a brief overview, I'm not 100% sure this fixes #6167 (it will always resolve the non-slash if you're using flatpages as the fallback in urlconf rather than using the middleware).
Go ahead and steal the tests in the other ticket, make sure this ticket fixes every case and then close #6167 as superceeded by this one. (You'll probably need some more tests for the things you're fixing)
comment:9 by , 17 years ago
Keywords: | easy-pickings added |
---|
comment:10 by , 17 years ago
Cc: | added |
---|
by , 16 years ago
Attachment: | common_middleware.3.py.diff added |
---|
Updated patch with slightly improved unit tests (more descriptive docstring comments).
comment:11 by , 16 years ago
Needs tests: | unset |
---|
The unit tests catch the problem reported by this ticket, as well as checking some cases in #6167, which this ticket supercedes.
follow-up: 14 comment:12 by , 16 years ago
Jason: In your patch, file django/middleware/common.py, line 108,
if resolve and new_url[0] == new_url[0] and ...
?
follow-up: 19 comment:13 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:14 by , 16 years ago
Replying to lukeplant:
Jason: In your patch, file django/middleware/common.py, line 108,
if resolve and new_url[0] == new_url[0] and ...
Oops, that should be:
if resolve and new_url[0] == old_url[0] and ...
comment:15 by , 16 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
This issue has still not been fixed: if settings.PREPEND_WWW
is True
and I have a flatpage mapped to '/', http://jasondavies.com/ doesn't redirect to http://www.jasondavies.com/
This is because in the code at the moment, a redirect only happens if the target URL can be resolved using urlresolvers.resolve()
, which obviously doesn't work when FlatpageFallbackMiddleware
is used. My patch fixes this problem by checking for a successful response code from any preceding middleware, and issuing a redirect if necessary as per the old behaviour.
comment:16 by , 16 years ago
Cc: | added |
---|
comment:17 by , 16 years ago
The current solution also leads to an infinite "302 Found" redirect if APPEND_SLASH is True and a flatpage exists at "/".
Should I file a new bug for this?
comment:18 by , 16 years ago
Cc: | added |
---|
follow-up: 21 comment:19 by , 16 years ago
Replying to jacob:
(In [8218]) Fixed #6213: flatpage view now correctly redirects if settings.APPEND_SLASH. Thanks, crankycoder.
Changeset [8218] also causes flatpages to trigger redirect on all URLs that don't end with a slash and return 404 response, regardless whether any page exists.
For example, if you have flatpage '/about/', flagpages are going to redirect '/about' -> '/about/', but also '/robots.txt' to '/robots.txt/' (if you don't have the file handled in urlconf), etc.
comment:20 by , 16 years ago
Cc: | added |
---|
comment:21 by , 14 years ago
Patch needs improvement: | set |
---|
I echo the concern noted above, that the patch in [8218] blindly redirects without testing to see whether the target of the redirect is valid (i.e., matches a URLconf or a FlatPage). At best, this adds extra request-response latency to 404 handling. At worst, it obscures the nature of the 404. Clients requesting /robots.txt should receive a 404 if that resource does not exist, and should not be redirected to a different resource (/robots.txt/).
For me, this problem manifests as a failing test case. I expect my REST API (which uses non-slash-terminated URLs) to return a 404 under certain conditions, but was surprised to find it returning a 302 -- due to FlatpageFallbackMiddleware blindly redirecting clients to a hypothetical slash-terminated resource instead of passing the 404 through to the client.
In short, FlatpageFallbackMiddleware should honor the APPEND_SLASH logic described in #3228, with one addition [shown in CAPS]:
"If APPEND_SLASH is set and the initial URL doesn't end with a slash, and it is not found in urlpatterns, a new URL is formed by appending a slash at the end. If this new URL is found in urlpatterns OR THE FLATPAGE TABLE, then an HTTP-redirect is returned to this new URL; otherwise the initial URL is processed as usual."
comment:22 by , 14 years ago
I've sent a pull request to fix the issue clay mentioned (and add some tests): https://github.com/django/django/pull/14
comment:23 by , 14 years ago
Cc: | added |
---|
comment:24 by , 14 years ago
Owner: | changed from | to
---|---|
Status: | reopened → new |
comment:25 by , 14 years ago
Owner: | changed from | to
---|
comment:26 by , 14 years ago
Owner: | changed from | to
---|
comment:27 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:28 by , 13 years ago
I've merged that pull request with upstream so it's up to date. Anyone want to take a look at it?
comment:29 by , 13 years ago
Component: | Core (Other) → contrib.flatpages |
---|
#6167 handles
APPEND_SLASH
(someone didn't search before adding a ticket :P). Rewording summary to just cover PREPEND_WWW