#5789 -- Django LocaleMiddleware django_language should be _language #1771

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@Bouke
Contributor

Bouke commented Oct 18, 2013

See ticket 5789 for discussion. There has also been talk about migrating old session keys, but no decision so far.

@timgraham

View changes

docs/releases/1.7.txt
@@ -133,6 +133,10 @@ for the following, instead of backend specific behavior.
Minor features
~~~~~~~~~~~~~~
+* The user's selected language is now stored with the key ``_language`` in the

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Oct 18, 2013

Owner

This should be in the "backwards incompatible changes" section.

@timgraham

timgraham Oct 18, 2013

Owner

This should be in the "backwards incompatible changes" section.

@timgraham

View changes

django/utils/translation/trans_real.py
@@ -426,7 +426,7 @@ def get_language_from_request(request, check_path=False):
return lang_code
if hasattr(request, 'session'):
- lang_code = request.session.get('django_language', None)
+ lang_code = request.session.get('_language')

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Oct 18, 2013

Owner

couldn't we provide backwards compatibility by falling back to 'django_language' here if '_language' doesn't exist? we could remove that after 1 release.

@timgraham

timgraham Oct 18, 2013

Owner

couldn't we provide backwards compatibility by falling back to 'django_language' here if '_language' doesn't exist? we could remove that after 1 release.

@timgraham

View changes

docs/topics/i18n/translation.txt
+
+ .. versionchanged:: 1.7
+
+ In previous versions the key was named ``django_language``, but was

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Oct 18, 2013

Owner

no comma

@timgraham

timgraham Oct 18, 2013

Owner

no comma

@timgraham

View changes

django/middleware/locale.py
@@ -57,7 +57,7 @@ def process_response(self, request, response):
# Store language back into session if it is not present
if hasattr(request, 'session'):
- request.session.setdefault('django_language', language)
+ request.session.setdefault('_language', language)

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Oct 18, 2013

Owner

this could be modified similar to the below for backwards compatibility I think.

@timgraham

timgraham Oct 18, 2013

Owner

this could be modified similar to the below for backwards compatibility I think.

@Bouke

This comment has been minimized.

Show comment Hide comment
@Bouke

Bouke Oct 22, 2013

Contributor

I've added backwards compatibility with tests. If the changes look OK, I will do a rebase+squash.

Contributor

Bouke commented Oct 22, 2013

I've added backwards compatibility with tests. If the changes look OK, I will do a rebase+squash.

@timgraham

View changes

django/middleware/locale.py
@@ -56,8 +56,13 @@ def process_response(self, request, response):
return self.response_redirect_class(language_url)
# Store language back into session if it is not present
- if hasattr(request, 'session'):
- request.session.setdefault('django_language', language)
+ if hasattr(request, 'session') and not '_language' in request.session:

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Oct 22, 2013

Owner

personal preference: I think '_language' not in request.session reads a bit clearer.

@timgraham

timgraham Oct 22, 2013

Owner

personal preference: I think '_language' not in request.session reads a bit clearer.

@timgraham

View changes

django/middleware/locale.py
- if hasattr(request, 'session'):
- request.session.setdefault('django_language', language)
+ if hasattr(request, 'session') and not '_language' in request.session:
+ # backwards compatibility check (1.7) on django_language

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Oct 22, 2013

Owner

instead of "(1.7)", I'd suggest "(remove in 1.8)"

@timgraham

timgraham Oct 22, 2013

Owner

instead of "(1.7)", I'd suggest "(remove in 1.8)"

+ request.session['_language'] = request.session['django_language']
+ del request.session['django_language']
+ else:
+ request.session['_language'] = language

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Oct 22, 2013

Owner

possibly we could just leave setdefault here so it's easier to revert when the time comes? or maybe just add a comment about it (or create a trac ticket with instructions)

@timgraham

timgraham Oct 22, 2013

Owner

possibly we could just leave setdefault here so it's easier to revert when the time comes? or maybe just add a comment about it (or create a trac ticket with instructions)

This comment has been minimized.

Show comment Hide comment
@Bouke

Bouke Oct 22, 2013

Contributor

Reverting to setdefault has two disadvantages:

  • django_language is also read from session, even if not needed
  • django_language is not removed from the session

I will add a note about reverting.

@Bouke

Bouke Oct 22, 2013

Contributor

Reverting to setdefault has two disadvantages:

  • django_language is also read from session, even if not needed
  • django_language is not removed from the session

I will add a note about reverting.

@timgraham

View changes

docs/internals/deprecation.txt
@@ -410,6 +410,9 @@ these changes.
* The ``Model._meta.get_(add|change|delete)_permission`` methods will
be removed.
+* Session key ``django_language`` will no longer be read for backwards

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Oct 22, 2013

Owner

The session key

@timgraham

timgraham Oct 22, 2013

Owner

The session key

@timgraham

View changes

docs/releases/1.7.txt
+* The user's selected language is now stored with the key ``_language`` in the
+ session. Previously it was stored with the key ``django_language``, but keys
+ reserved for Django should start with an underscore. For backwards
+ compatibility ``django_language`` is still used in 1.7 and will be save for

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Oct 22, 2013

Owner

"is still read from in 1.7. Sessions will be migrated to the new _language key as they are written."

I'm not sure what "and will be save for use in 1.8" means?

@timgraham

timgraham Oct 22, 2013

Owner

"is still read from in 1.7. Sessions will be migrated to the new _language key as they are written."

I'm not sure what "and will be save for use in 1.8" means?

This comment has been minimized.

Show comment Hide comment
@Bouke

Bouke Oct 22, 2013

Contributor

What I'm trying to say is that users of Django should not yet use django_language session key, as it is still used in Django 1.7. When 1.8 drops backwards compatibility, the session key can be used by users.

@Bouke

Bouke Oct 22, 2013

Contributor

What I'm trying to say is that users of Django should not yet use django_language session key, as it is still used in Django 1.7. When 1.8 drops backwards compatibility, the session key can be used by users.

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Oct 22, 2013

Owner

Ah ok, you have a typo: "safe for use"

@timgraham

timgraham Oct 22, 2013

Owner

Ah ok, you have a typo: "safe for use"

@timgraham

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Oct 22, 2013

Owner

Looks good. Feel free to rebase and squash with the minor updates above.

Owner

timgraham commented Oct 22, 2013

Looks good. Feel free to rebase and squash with the minor updates above.

#5789 -- Django LocaleMiddleware django_language should be _language
Thanks to jdunck for the report and stugots for the initial patch.
@timgraham

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Oct 22, 2013

Owner

merged in 0d0f4f0 - thanks!

Owner

timgraham commented Oct 22, 2013

merged in 0d0f4f0 - thanks!

@timgraham timgraham closed this Oct 22, 2013

@Bouke Bouke deleted the Bouke:tickets/5789 branch Oct 22, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment