[Django] #32005: Allow disabling of auto-404-redirection in LocaleMiddleware
Django
------------------------------------------------+------------------------
Reporter: Alex Vandiver | Owner: nobody
Type: Uncategorized | Status: new
Component: Internationalization | Version: 3.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
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](https://github.com/zulip/zulip/blob/master/zerver/middleware.py#L434),
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/32005>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Alex Vandiver | Owner: nobody
Component: | Version: 3.1
Internationalization |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* type: Uncategorized => New feature
Old description:
> 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](https://github.com/zulip/zulip/blob/master/zerver/middleware.py#L434),
> 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?
New description:
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
[https://github.com/zulip/zulip/blob/master/zerver/middleware.py#L434
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?
--
Comment:
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` before
`LocaleMiddleware` would the response returned in `process_request` not
get sent back to the client before `LocaleMiddleware` ever got the chance
to look at it?
--
Ticket URL: <https://code.djangoproject.com/ticket/32005#comment:1>
Django
Reporter: Alex Vandiver | Owner: nobody
Component: | Version: 3.1
Internationalization |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Looks like you've just introduced this issue here
https://github.com/zulip/zulip/commit/536bd3188e9428993fd712ed2f0df7c160b6ad60
🤔
--
Ticket URL: <https://code.djangoproject.com/ticket/32005#comment:2>
Django
Reporter: Alex Vandiver | Owner: nobody
Component: | Version: 3.1
Internationalization |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Alex Vandiver):
Replying to [comment:2 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
''before'' the middleware that throws the 404, in order for the 404 page
to be able to be properly localized. That's what now causes the
LocaleMiddleware to intercept the 404 and turn it into a 302.
--
Ticket URL: <https://code.djangoproject.com/ticket/32005#comment:3>
Django
Reporter: Alex Vandiver | Owner: nobody
Component: | Version: 3.1
Internationalization |
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => wontfix
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32005#comment:4>
Django
Reporter: Alex Vandiver | Owner: nobody
Component: | Version: 3.1
Internationalization |
Severity: Normal | Resolution: wontfix
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Old description:
> 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
> [https://github.com/zulip/zulip/blob/master/zerver/middleware.py#L434
> 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?
New description:
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
[https://github.com/zulip/zulip/blob/536bd3188e9428993fd712ed2f0df7c160b6ad60/zerver/middleware.py#L453
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?
--
Comment (by Alex Vandiver):
Fair enough. The only slight ugliness with subclassing is that one needs
to repeat
[https://github.com/django/django/blob/master/django/middleware/locale.py#L58-L60
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
[https://github.com/zulip/zulip/blob/536bd3188e9428993fd712ed2f0df7c160b6ad60/zerver/middleware.py#L378-L385
reasonable amount of code duplication].
--
Ticket URL: <https://code.djangoproject.com/ticket/32005#comment:5>