Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#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 Alex Vandiver)

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 Carlton Gibson, 4 years ago

Description: modified (diff)
Type: UncategorizedNew feature

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?

comment:2 by Carlton Gibson, 4 years ago

Looks like you've just introduced this issue here https://github.com/zulip/zulip/commit/536bd3188e9428993fd712ed2f0df7c160b6ad60 🤔

in reply to:  2 comment:3 by Alex Vandiver, 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.

Last edited 4 years ago by Alex Vandiver (previous) (diff)

comment:4 by Carlton Gibson, 4 years ago

Resolution: wontfix
Status: newclosed

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 Alex Vandiver, 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.

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