[Django] #18105: Investigate possible misuse of Context
Django
-----------------------------------------+------------------------
Reporter: aaugustin | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.4
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
This was reported [https://code.djangoproject.com/ticket/17229#comment:20
in the comments of #17229], but I'm moving it to its own ticket for
clarity.
----
Changeset r17894 produced several errors in the Django test suite. In
fact, it reveals issues where Context instances are initialized with a
Context argument instead of a simple dict.[[BR]]
Examples:
* In browser:django/trunk/django/contrib/auth/tests/urls.py, several
render_to_response calls have the Context instance as second argument,
which seems to be plain bugs (uncaught until now)
* In
browser:django/trunk/django/contrib/admin/templatetags/admin_modify.py,
the prepopulated_fields_js inclusion tag returns a Context instance
instead of a dict.
The question is now whether Context initialization should accept other
Context instance as the `__init__` dict_ parameter (and then this
changeset should be fixed) or if we should enforce dict_ as a dict and fix
places in Django code when it is not the case. I'd be in favour of the
latter, but this might need some more investigation.
--
Ticket URL: <https://code.djangoproject.com/ticket/18105>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: aaugustin | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.4
Component: Template system | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* component: Uncategorized => Template system
* type: Uncategorized => Cleanup/optimization
Comment:
After re-reading the docs about Context, I think things are clear: Context
takes a dict as optional init parameter. So for me, passing a Context to
another Context init is a mistake and should be fixed (see my patch on
#18103).
--
Ticket URL: <https://code.djangoproject.com/ticket/18105#comment:1>
Django
Reporter: aaugustin | Owner: nobody
Cleanup/optimization | Version: 1.4
Component: Template system | Resolution:
Severity: Normal | Triage Stage:
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
I think we need a deprecation path, but I'm in favor of this change.
--
Ticket URL: <https://code.djangoproject.com/ticket/18105#comment:2>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* has_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
I have separated in two distinct patches the problematic uses of Context
initialization from a context instance in Django code.
At this point and considering the correction you committed in r17896, it's
still possible to intialize a Context from another Context, even with
these patches.
--
Ticket URL: <https://code.djangoproject.com/ticket/18105#comment:3>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/18105#comment:4>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
[73317883] changed `render_to_response` so it wouldn't rewrap a `Context`
inadvertently passed in the `dictionary` argument in another `Context`.
That's the opposite of the route taken by first patch above and I'm
convinced it's a better approach given how widespread `render_to_response`
is and how easy forgetting `context_instance=` is.
The second patch still warrants consideration. I had a look and I don't
think the code inside `if isinstance(_dict, BaseContext)` is correct: it
adds values such as `autoescape` and `current_app` to the context when
they're intended to be attributes of the context object.
--
Ticket URL: <https://code.djangoproject.com/ticket/18105#comment:5>
Django
Reporter: Aymeric Augustin | Owner: nobody
Type: Cleanup/optimization | Status: closed
Component: Template system | Version: 1.4
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: new => closed
* resolution: => fixed
Comment:
As far as I'm aware the second part was fixed by
e53495ba3352c2c0fdb6178f2b333c30cb6b5d46.
--
Ticket URL: <https://code.djangoproject.com/ticket/18105#comment:6>