Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#27402 closed Bug (fixed)

When using i18n_patterns and prefix_default_language=False, 404 page redirects incorrectly

Reported by: Hovi Owned by: nobody
Component: Internationalization Version: 1.10
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hello,

I just migrated to 1.10.2 using new cool localization with prefix_default_language=False, replacing solid-i18n-urls:
https://github.com/st4lk/django-solid-i18n-urls
Only smaller issue is, that whenever I go to 404 page, I get redirected to /en/THE_OLD_URL/ (en is default language in my case).

My guess is bug in LocaleMiddleware, language_path variable is being constructed always using language code even in cases when there shouldn't be (default language).
I attached simple fix, that seems to work it out for me.

Attachments (2)

locale_404_bug.diff (706 bytes) - added by Hovi 4 years ago.
locale_404_bug_with_test.diff (2.1 KB) - added by Hovi 4 years ago.

Download all attachments as: .zip

Change History (14)

Changed 4 years ago by Hovi

Attachment: locale_404_bug.diff added

comment:1 Changed 4 years ago by Tim Graham

Can you add a test? (Just to be sure, #27063 doesn't solve the issue?)

comment:2 Changed 4 years ago by Hovi

It's definitely not same thing as #27063. I took a deeper look and it is a little more complicated.
When I just added test, it was passing anyway and it turned out problem is little more specific to my case (it is still bug though).
I have something like this in my url as last record without i18npatterns:

r'(?P<group1>.+)/(?P<group2>.+)/$

So what exactly happens here:
1, language_path is constructed as /en/non-existent-page instead of /non-existent-page
2, language_path passes is_valid_path(language_path,... because that url matches my regex.
3, that leads to redirect

Code after that constructs url for redirect (variable language_url) also doesn't take into account possibility default language without prefix, so that part may also need reviewing but I am not even sure if that can ever happen.

Attaching simple test.

Changed 4 years ago by Hovi

comment:3 Changed 4 years ago by Tim Graham

Has patch: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

I haven't looked into the details, but the new test isn't passing. Please send a pull request if you're able to improve the patch.

comment:4 Changed 4 years ago by Hovi

Yeah, new merged tests actually broke it by coincidentally using similar url pattern.

https://github.com/django/django/pull/7461

comment:5 Changed 4 years ago by Tim Graham

Patch needs improvement: unset

comment:6 Changed 4 years ago by François Freitag

Patch needs improvement: set

I've left a comment on the PR.

comment:7 Changed 4 years ago by Krzysztof Urbaniak

Patch needs improvement: unset

I've submitted pull request https://github.com/django/django/pull/7483 with a improved version of the patch.

comment:8 Changed 4 years ago by François Freitag

Triage Stage: AcceptedReady for checkin

The PR looks good to me. Thanks !

comment:9 Changed 3 years ago by Tim Graham

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

comment:10 Changed 3 years ago by François Freitag

Patch needs improvement: unset

comment:11 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In b8a815e9:

Fixed #27402 -- Fixed incorrect LocaleMiddleware redirects with prefix_default_language=False.

comment:12 Changed 3 years ago by Tim Graham <timograham@…>

In 81b5971b:

[1.10.x] Fixed #27402 -- Fixed incorrect LocaleMiddleware redirects with prefix_default_language=False.

Backport of b8a815e9dfea89034ede7ff786551f89af84a31b from master

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