Jlc/palm mig/language middleware - #127
Conversation
| from django.conf import settings | ||
| from django.http import Http404 | ||
| from django.http import Http404, parse_cookie | ||
| from django.utils.deprecation import MiddlewareMixin |
There was a problem hiding this comment.
why are you using deprecation ? it's not better to implement that in the new way ?
There was a problem hiding this comment.
Because openedx is using it?
https://github.com/openedx/edx-platform/blob/open-release/palm.master/openedx/core/djangoapps/lang_pref/middleware.py#L5
https://github.com/openedx/edx-platform/blob/open-release/palm.master/openedx/core/djangoapps/dark_lang/middleware.py#L15
There was a problem hiding this comment.
IMO that's is not a valid reason, probably that is a old middleware that openedx had to update when they made the django upgrade, however this is a new middleware that you're making from scratch
There was a problem hiding this comment.
@andrey-canon Actually, is something confusing in the nomenclature of django.
https://stackoverflow.com/questions/52913333/why-middleware-mixin-declared-in-django-utils-deprecation-py
Is like a help to create new middleware or to support old styles middleware.
https://docs.djangoproject.com/en/4.2/topics/http/middleware/#upgrading-pre-django-1-10-style-middleware
Some base middleware of django are inheriting from it.
https://github.com/django/django/blob/main/django/contrib/sites/middleware.py#L6
https://github.com/django/django/blob/main/django/contrib/auth/middleware.py#L23
There was a problem hiding this comment.
Some base middleware of django are inheriting from it.
This is the same argument that you exposed in the first comment but with django, my answer is the same those are old middlewares that they made compatible with the new middleware structure
my question would be do you have any reason to use the structure that django deprecated in the past, why do you think the deprecated version is better than the "new" django way ?
There was a problem hiding this comment.
No, but I think that Django makes this middleware-mixin with the idea of being compatible with both ways: MIDDLEWARE AND MIDDLEWARE_CLASSES. So I prefer the compatibility of both ways for the middleware.
There was a problem hiding this comment.
MIDDLEWARE_CLASSES was deprecated in Django 1.10 and probably you won't find any use of that in later versions
f8f0caa to
d47f188
Compare
This ensure to use the cookie language if the user send it.
This add the compatiblity for only he new django way of MIDDLEWARE.
d47f188 to
78d7582
Compare
| @@ -5,6 +5,8 @@ | |||
| classes: | |||
| ExtendedProfileFieldsMiddleware: Set extended_profile_fields in registration form. | |||
There was a problem hiding this comment.
could you please add the new middleware here
aab8199 to
dbfda6b
Compare
2 participants
Description
Migrate the language per cookie usage from edx-platform to plugin.
This was inspired in the
COOKIESmanipulation by django.https://github.com/django/django/blob/main/django/core/handlers/wsgi.py#L100-L102Testing instructions
Checkout you edx-platform to
open-release/palm.nelp.Install eox-nelp using this branch or using editable way.
eg
Screencast.from.26-01-24.17.01.30.webm
Before
Screencast.from.26-01-24.16.54.14.webm
After
Screencast.from.26-01-24.17.02.47.webm
Additional information
PRS related
original-pr with discussion: nelc/edx-platform#5
migration pr: eduNEXT/edunext-platform#672
Checklist for Merge