#32005 closed New feature (wontfix)
Allow disabling of auto-404-redirection in LocaleMiddleware
Reported by: | Alex Vandiver | Owned by: | nobody |
---|---|---|---|
Component: | Internationalization | Version: | 3.1 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
This is related to the last two comments on #17734. Specifically, if an application decides to return an explicit 404, there is no way to prevent the LocaleMiddleware from overriding this and trying the language redirect.
In those comments, it was about catch-all URL patterns. I'm running into something related, but slightly different -- we serve 404's for the `/` endpoint if the subdomain isn't valid, which the LocaleMiddleware unhelpfully redirects to (e.g.) /en/
which isn't any less of a 404.
Would folks be amenable to a patch which disabled the auto-404-redirect functionality in the middleware with a flag of some sort?
Change History (5)
comment:1 by , 4 years ago
Description: | modified (diff) |
---|---|
Type: | Uncategorized → New feature |
follow-up: 3 comment:2 by , 4 years ago
Looks like you've just introduced this issue here https://github.com/zulip/zulip/commit/536bd3188e9428993fd712ed2f0df7c160b6ad60 🤔
comment:3 by , 4 years ago
This is certainly tied to middleware ordering -- but I don't think that swapping the order up will fix it.
Replying to Carlton Gibson:
Looks like you've just introduced this issue here https://github.com/zulip/zulip/commit/536bd3188e9428993fd712ed2f0df7c160b6ad60 🤔
That's the commit which introduces the need for it, and and works around its lack; you can see it cites this ticket in so doing.
As noted in that commit message, the LocaleMiddleware needs to be around the middleware that throws the 404, in order for the 404 page to be able to be properly localized. If the LocaleMiddleware is inside it, then the 404 page actually gets the last localization that the thread served, in the previous request! You can see this live if you bounce reload on https://nonexistent.zulipchat.com/ for a bit.
comment:4 by , 4 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Yes, I see it. TBH, I don't think a change here is worth the complexity. It's pretty niche and you can easily subclass LocaleMiddleware
to skip the process_response
redirect, by setting an attribute on the response in your HostDomainMiddleware
and checking for it there.
comment:5 by , 4 years ago
Description: | modified (diff) |
---|
Fair enough. The only slight ugliness with subclassing is that one needs to repeat the logic that adds `Content-Language` and `Vary` headers -- skipping all of the process_response
on 404s wouldn't produce the right headers. Which actually means a reasonable amount of code duplication.
Hi Alex.
Thanks for the report. Perhaps need an extra coffee to think through if it's going to be worth the complexity but... — is this not a middleware ordering issue? i.e. If you put your
HostDomainMiddleware
beforeLocaleMiddleware
would the response returned inprocess_request
not get sent back to the client beforeLocaleMiddleware
ever got the chance to look at it?