Opened 13 years ago
Closed 2 years ago
#18105 closed Cleanup/optimization (fixed)
Investigate possible misuse of Context
Reported by: | Aymeric Augustin | Owned by: | nobody |
---|---|---|---|
Component: | Template system | Version: | 1.4 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
This was reported 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.
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.
Attachments (2)
Change History (8)
comment:1 by , 13 years ago
Component: | Uncategorized → Template system |
---|---|
Type: | Uncategorized → Cleanup/optimization |
by , 13 years ago
Attachment: | 18105-1.diff added |
---|
Do not accept context instance as second render_to_response parameter
by , 13 years ago
Attachment: | 18105-2.diff added |
---|
Properly construct context when inclusion tag returns a context
comment:3 by , 13 years ago
Has patch: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
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.
comment:4 by , 10 years ago
Patch needs improvement: | set |
---|
comment:5 by , 10 years ago
733178830072caeca3c054a220808b4c557faec4 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.
comment:6 by , 2 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
As far as I'm aware the second part was fixed by e53495ba3352c2c0fdb6178f2b333c30cb6b5d46.
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).