Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #34455 -- Restored i18n_patterns() respect of prefix_default_language argument when fallback language is used. #16735

Merged
merged 1 commit into from Apr 10, 2023

Conversation

sarahboyce
Copy link
Contributor

@felixxm
Copy link
Member

felixxm commented Apr 7, 2023

@oussjarrousse Does it work for you?

@@ -351,7 +351,8 @@ def regex(self):
@property
def language_prefix(self):
language_code = get_language() or settings.LANGUAGE_CODE
if language_code == settings.LANGUAGE_CODE and not self.prefix_default_language:
default_language = get_supported_language_variant(settings.LANGUAGE_CODE)
if language_code == default_language and not self.prefix_default_language:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is due to the fact that we're currently activating a fallback language not settings.LANGUAGE_CODE:

if not language:
language = self.get_fallback_language(request)

@claudep What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your question exactly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to emphasize that currently we're activating a fallback language e.g. en. In older versions, we activated settings.LANGUAGE_CODE, e.g. en-us. It is a side-effect of 94e7f47, not an intended change (as far as I'm aware). Now the question 😄 Do you think it's fine? or maybe we should try to avoid this side-effect instead of changing the language_prefix() 🤔 .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test would also pass by adding en-us to the global LANGUAGES setting. en-us is the default for LANGUAGE_CODE but it doesn't appear to be in any of the lists of supported languages (and it falls back to en)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the feeling that activating a language which is not in LANGUAGES is a bit weird, even if it's the historic behavior, so would be in favor of keeping the new behavior. However I'm not sure if it has other consequences that could be considered as backwards incompatible. If we keep that, we should also probably change the default LANGUAGE_CODE to en in global settings. Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think updating the default for LANGUAGE_CODE to en makes sense.

I guess previously we didn't validate that the LANGUAGE_CODE setting was within LANGUAGES, that en-us is the default is unfortunate but I guess people could also have used other things. 🤔

Some options could be:

  • Go with this current change and anyone who has this invalid value for LANGUAGE_CODE would not need to make an update.
  • We could treat en-us as a alias for en (so similar to this change but a bit stricter to only handle the en-us case).
  • Tell people to update the setting.

I'm really not familiar with depreciating things, maybe we should add a warning when your LANGUAGE_CODE is not within LANGUAGES?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks both, let's have it 👍

If we keep that, we should also probably change the default LANGUAGE_CODE to en in global settings. Does that make sense?

Yes, but we can do this separately as a cleanup in Django 5.0.

@felixxm felixxm changed the title Fixed #34455 -- Unified language prefix handling from LocalePrefixPattern and LocaleMiddleware. Fixed #34455 -- Restored i18n_patterns() respect of prefix_default_language argument when fallback language is used. Apr 10, 2023
…nguage argument when fallback language is used.

Regression in 94e7f47.

Thanks Oussama Jarrousse for the report.
@felixxm
Copy link
Member

felixxm commented Apr 10, 2023

@sarahboyce Thanks 👍 I added a release note.

@felixxm felixxm merged commit 3b47283 into django:main Apr 10, 2023
26 checks passed
@felixxm felixxm temporarily deployed to schedules April 11, 2023 02:45 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants